feat: support single db batch operation #8268
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @nazarhussain, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new batch operation for database repositories, enabling atomic put and delete operations within a single transaction. This enhancement is a crucial step towards implementing atomic writes across multiple database repositories, addressing a part of issue #8244.
Highlights
- Unified Batch Operations: Implemented a new batch method across the database interfaces (DatabaseController, Repository, PrefixedRepository) and their LevelDbController implementation, allowing put and delete operations to be bundled and executed atomically.
- New DbBatch Type: Introduced a DbBatch type to formally define the structure for combined put and delete operations, improving type safety and clarity.
- Refactored persistDiff Function: The persistDiff utility in opPool.ts was updated to leverage the new atomic batch operation, replacing separate batchPut and batchDelete calls for improved data consistency.
- Comprehensive Testing: A new unit test was added to level.test.ts to thoroughly validate the functionality of the new batch operation, ensuring its reliability.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces support for atomic batch operations (put and delete) on a single database repository. This is achieved by adding a new batch method to the database controller and repository layers, which is then used to refactor persistDiff in opPool.ts for more efficient and atomic database updates. The changes are well-structured and include corresponding type definitions and tests. My review includes a few suggestions to improve code conciseness using functional patterns and points out a testing configuration that should be corrected before merging.
Comment on lines
+144
to
+158
| async batch(prefix: P, batch: DbBatch<I, T>): Promise<void> { | ||
| const batchWithKeys = []; | ||
| for (const b of batch) { | ||
| if (b.type === "del") { | ||
| batchWithKeys.push({type: b.type, key: this.wrapKey(this.encodeKeyRaw(prefix, b.key))}); | ||
| } else { | ||
| batchWithKeys.push({ | ||
| type: b.type, | ||
| key: this.wrapKey(this.encodeKeyRaw(prefix, b.key)), | ||
| value: this.encodeValue(b.value), | ||
| }); | ||
| } | ||
| } | ||
| await this.db.batch(batchWithKeys, this.dbReqOpts); | ||
| } |
There was a problem hiding this comment.
The for loop with if/else can be simplified by using the map function on the batch array. This would make the code more concise and functional. Also, it's good practice to type batchWithKeys.
async batch(prefix: P, batch: DbBatch<I, T>): Promise<void> {
const batchWithKeys: DbBatch<Uint8Array, Uint8Array> = batch.map((b) => {
const key = this.wrapKey(this.encodeKeyRaw(prefix, b.key));
if (b.type === "del") {
return {type: "del", key};
} else {
return {type: "put", key, value: this.encodeValue(b.value)};
}
});
await this.db.batch(batchWithKeys, this.dbReqOpts);
}There was a problem hiding this comment.
We prefer using for off instead of map iterators because of performance.
Comment on lines
+160
to
+166
| async batchBinary(prefix: P, batch: DbBatch<I, Uint8Array>): Promise<void> { | ||
| const batchWithKeys = []; | ||
| for (const b of batch) { | ||
| batchWithKeys.push({...b, key: this.wrapKey(this.encodeKeyRaw(prefix, b.key))}); | ||
| } | ||
| await this.db.batch(batchWithKeys, this.dbReqOpts); | ||
| } |
There was a problem hiding this comment.
This for loop can be simplified by using the map function on the batch array, which is more concise. It's also good practice to type batchWithKeys.
async batchBinary(prefix: P, batch: DbBatch<I, Uint8Array>): Promise<void> {
const batchWithKeys: DbBatch<Uint8Array, Uint8Array> = batch.map((b) => ({
...b,
key: this.wrapKey(this.encodeKeyRaw(prefix, b.key)),
}));
await this.db.batch(batchWithKeys, this.dbReqOpts);
}There was a problem hiding this comment.
We prefer using for off instead of map iterators because of performance.
Comment on lines
+133
to
+143
| async batch(batch: DbBatch<I, T>): Promise<void> { | ||
| const batchWithKeys: DbBatch<Uint8Array, Uint8Array> = []; | ||
| for (const b of batch) { | ||
| if (b.type === "del") { | ||
| batchWithKeys.push({...b, key: this.encodeKey(b.key)}); | ||
| } else { | ||
| batchWithKeys.push({...b, key: this.encodeKey(b.key), value: this.encodeValue(b.value)}); | ||
| } | ||
| } | ||
| await this.db.batch(batchWithKeys, this.dbReqOpts); | ||
| } |
There was a problem hiding this comment.
The for loop with if/else can be simplified by using the map function on the batch array. This would make the code more concise, functional, and arguably more readable by explicitly constructing the new objects rather than using spread syntax with property overrides.
async batch(batch: DbBatch<I, T>): Promise<void> {
const batchWithKeys: DbBatch<Uint8Array, Uint8Array> = batch.map((b) => {
if (b.type === "del") {
return {type: "del", key: this.encodeKey(b.key)};
} else {
return {type: "put", key: this.encodeKey(b.key), value: this.encodeValue(b.value)};
}
});
await this.db.batch(batchWithKeys, this.dbReqOpts);
}There was a problem hiding this comment.
We prefer using for off instead of map iterators because of performance.
Comment on lines
+145
to
+151
| async batchBinary(batch: DbBatch<I, Uint8Array>): Promise<void> { | ||
| const batchWithKeys: DbBatch<Uint8Array, Uint8Array> = []; | ||
| for (const b of batch) { | ||
| batchWithKeys.push({...b, key: this.encodeKey(b.key)}); | ||
| } | ||
| await this.db.batch(batchWithKeys, this.dbReqOpts); | ||
| } |
There was a problem hiding this comment.
This for loop can be simplified by using the map function on the batch array, which is more concise and follows a functional programming style.
async batchBinary(batch: DbBatch<I, Uint8Array>): Promise<void> {
const batchWithKeys: DbBatch<Uint8Array, Uint8Array> = batch.map((b) => ({
...b,
key: this.encodeKey(b.key),
}));
await this.db.batch(batchWithKeys, this.dbReqOpts);
}There was a problem hiding this comment.
We prefer using for off instead of map iterators because of performance.
Performance Report🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
|
merge conflicts |
|
do we really need this? I have never encountered this as an issue, so it's seems purely theoretical |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Support single db operation to delete/put in a single repository. Will partially covers #8244 and enable next step for the across db repositories atomic write.
Description
putanddeletein a single batchSteps to test or reproduce