Replace DistributedStorage with SingleCrudTransactionManager on data loader import storage mode
#3125
Conversation
Summary of ChangesHello @inv-jishnu, 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 refactors the data loader's import mechanism to utilize Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully replaces DistributedStorage with SingleCrudOperationTransactionManager for the data loader's import storage mode, which is a positive step toward unifying the data access APIs. The changes are applied consistently across the CLI, core logic, and tests. My review includes a couple of suggestions for improvement. Firstly, there's a notable amount of code duplication in ScalarDbDao from adding new overloaded methods; refactoring this could enhance maintainability. Secondly, I've identified an inconsistency in exception handling within a new method in the same class. Addressing these points will further improve the code's quality and robustness.
| public Optional<Result> get( | ||
| String namespace, | ||
| String table, | ||
| Key partitionKey, | ||
| Key clusteringKey, | ||
| SingleCrudOperationTransactionManager manager) | ||
| throws ScalarDbDaoException { | ||
|
|
||
| // Retrieving the key data for logging | ||
| String loggingKey = keysToString(partitionKey, clusteringKey); | ||
|
|
||
| try { | ||
| Get get = createGetWith(namespace, table, partitionKey, clusteringKey); | ||
| return manager.get(get); | ||
| } catch (CrudException e) { | ||
| throw new ScalarDbDaoException("error GET " + loggingKey, e); | ||
| } | ||
| } |
There was a problem hiding this comment.
This pull request introduces several new overloaded methods for SingleCrudOperationTransactionManager and DistributedTransactionManager (get, put, scan, createScanner). These new methods are very similar to existing ones, leading to significant code duplication. For instance, this new get method is almost identical to the one for DistributedTransaction.
Since SingleCrudOperationTransactionManager, DistributedTransactionManager, and DistributedTransaction all implement or can be treated as CrudOperable, you could refactor this to use a single generic method that accepts a CrudOperable argument. This would greatly reduce code duplication and improve the maintainability of this class.
A generic method might look something like this:
public Optional<Result> get(
String namespace,
String table,
Key partitionKey,
Key clusteringKey,
CrudOperable operable)
throws ScalarDbDaoException {
// ...
}This approach could be applied to put, scan, and createScanner methods as well.
There was a problem hiding this comment.
Some of the contents will be removed in another PR. I will also consider this and make necessary changes.
data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/dao/ScalarDbDao.java
Show resolved
Hide resolved
|
Closing this PR as this change is no longer needed. |
1 participant
Description
In this PR I have replaced the usage of
DIstributedStoragewithSingleCrudTransactionManagerin scalardb data loader when storage mode is selected as import mode. This PR only removes distributed storage from import section but not completely. The other occurrences will be removed as well in later PRs.Related issues and/or PRs
NA
Changes made
DistributedStoragewithSingleCrudTransactionManagerChecklist
Additional notes (optional)
I will create other PRs for related changes to table metadata service, and export service
Release notes
Replaced
DistributedStoragewithSingleCrudTransactionManageron data loader import storage mode