Revisit importTable() in Consensus Commit #3102
Conversation
Summary of ChangesHello @brfrn169, 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 significantly refactors the 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 removes the getImportTableMetadata and addRawColumnToTable methods from the DistributedStorageAdmin interface and related classes, as well as disables corresponding tests for Cassandra and DynamoDB, and modifies the importTable method in ConsensusCommitAdmin. The review focuses on the correctness of removing these methods and the impact on the overall system.
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java
Show resolved
Hide resolved
.../src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManager.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the import table functionality by removing deprecated getImportTableMetadata and addRawColumnToTable methods from the DistributedStorageAdmin interface and its implementations, simplifying the import process to use importTable directly followed by addNewColumnToTable for metadata columns.
Key changes:
- Removed
getImportTableMetadataandaddRawColumnToTablemethods from the admin API interfaces and all implementations - Updated
ConsensusCommitAdmin.importTableto calladmin.importTable()first, then add columns usingaddNewColumnToTableinstead ofaddRawColumnToTable - Refactored batch operation read-only checks to use
instanceof Selectioninstead of checking forGetorScanindividually - Removed associated tests for the deleted methods
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
core/src/main/java/com/scalar/db/api/DistributedStorageAdmin.java |
Removed getImportTableMetadata and addRawColumnToTable method declarations from interface |
core/src/main/java/com/scalar/db/common/CommonDistributedStorageAdmin.java |
Removed wrapper implementations for deleted methods |
core/src/main/java/com/scalar/db/service/AdminService.java |
Removed delegation implementations for deleted methods |
core/src/main/java/com/scalar/db/storage/multistorage/MultiStorageAdmin.java |
Removed routing implementations for deleted methods |
core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java |
Removed addRawColumnToTable implementation and changed getImportTableMetadata visibility to package-private |
core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java |
Removed unsupported operation stubs for deleted methods |
core/src/main/java/com/scalar/db/storage/cosmos/CosmosAdmin.java |
Removed unsupported operation stubs for deleted methods |
core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java |
Removed unsupported operation stubs for deleted methods |
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java |
Refactored importTable to call admin.importTable() first, then use addNewColumnToTable instead of addRawColumnToTable |
core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java |
Simplified batch read-only check to use instanceof Selection |
core/src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManager.java |
Simplified batch read-only check to use instanceof Selection |
core/src/test/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdminTestBase.java |
Updated test to verify importTable is called before adding columns, changed assertions to use addNewColumnToTable |
core/src/test/java/com/scalar/db/storage/multistorage/MultiStorageAdminTest.java |
Removed test for deleted getImportTableMetadata method |
core/src/test/java/com/scalar/db/storage/jdbc/JdbcAdminTest.java |
Removed tests for deleted addRawColumnToTable method |
core/src/test/java/com/scalar/db/storage/dynamo/DynamoAdminTestBase.java |
Removed unsupported operation tests for deleted methods, renumbered remaining test variables |
core/src/test/java/com/scalar/db/storage/cosmos/CosmosAdminTest.java |
Removed unsupported operation tests for deleted methods, renumbered remaining test variables |
core/src/test/java/com/scalar/db/storage/cassandra/CassandraAdminTest.java |
Removed unsupported operation tests for deleted methods, renumbered remaining test variables |
integration-test/src/main/java/com/scalar/db/api/DistributedStorageAdminPermissionIntegrationTestBase.java |
Removed integration tests for deleted methods |
core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoAdminPermissionIntegrationTest.java |
Removed disabled test overrides for deleted methods |
core/src/integration-test/java/com/scalar/db/storage/cassandra/CassandraAdminPermissionIntegrationTest.java |
Removed disabled test overrides for deleted methods |
core/src/integration-test/java/com/scalar/db/storage/jdbc/JdbcSchemaLoaderImportIntegrationTest.java |
Added missing @AfterAll annotation to afterAll method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java
Show resolved
Hide resolved
There was a problem hiding this comment.
The main changes are in ConsensusCommitAdmin.
This refactor updates the logic to call admin.importTable() first, and then add columns using addNewColumnToTable() instead of addRawColumnToTable().
| /** | ||
| * Get import table metadata in the ScalarDB format. | ||
| * | ||
| * @param namespace namespace name of import table | ||
| * @param table import table name | ||
| * @throws IllegalArgumentException if the table does not exist | ||
| * @throws IllegalStateException if the table does not meet the requirement of ScalarDB table | ||
| * @throws ExecutionException if the operation fails | ||
| * @return import table metadata in the ScalarDB format | ||
| */ | ||
| default TableMetadata getImportTableMetadata(String namespace, String table) | ||
| throws ExecutionException { | ||
| return getImportTableMetadata(namespace, table, Collections.emptyMap()); | ||
| } | ||
|
|
||
| /** | ||
| * Get import table metadata in the ScalarDB format. | ||
| * | ||
| * @param namespace namespace name of import table | ||
| * @param table import table name | ||
| * @param overrideColumnsType a map of column data type by column name. Only set the column for | ||
| * which you want to override the default data type mapping. | ||
| * @throws IllegalArgumentException if the table does not exist | ||
| * @throws IllegalStateException if the table does not meet the requirement of ScalarDB table | ||
| * @throws ExecutionException if the operation fails | ||
| * @return import table metadata in the ScalarDB format | ||
| */ | ||
| TableMetadata getImportTableMetadata( | ||
| String namespace, String table, Map<String, DataType> overrideColumnsType) | ||
| throws ExecutionException; | ||
|
|
||
| /** | ||
| * Add a column in the table without updating the metadata table in ScalarDB. | ||
| * | ||
| * @param namespace namespace name of import table | ||
| * @param table import table name | ||
| * @param columnName name of the column to be added | ||
| * @param columnType type of the column to be added | ||
| * @throws IllegalArgumentException if the table does not exist | ||
| * @throws ExecutionException if the operation fails | ||
| */ | ||
| void addRawColumnToTable(String namespace, String table, String columnName, DataType columnType) | ||
| throws ExecutionException; | ||
|
|
There was a problem hiding this comment.
After the refactoring in ConsensusCommitAdmin, these methods are no longer needed.
| Map<String, String> options, | ||
| Map<String, DataType> overrideColumnsType) | ||
| throws ExecutionException { | ||
| rdbEngine.throwIfImportNotSupported(); |
There was a problem hiding this comment.
This is not directly related to the refactoring, but I noticed that the behavior of adding metadata seems to be different between 4.x and 3.x. 3.x uses repairTable(), so it deletes table metadata first and then adds the new metadata. In contrast, 4.x does not delete the existing metadata. It diverged at #1125.
@Torch3333, do you remember the reason why you chose the overwriteMetadata = false? Both should be the same if there is no specific reason.
try (Connection connection = dataSource.getConnection()) {
TableMetadata tableMetadata = getImportTableMetadata(namespace, table, overrideColumnsType);
createNamespacesTableIfNotExists(connection);
upsertIntoNamespacesTable(connection, namespace);
addTableMetadata(connection, namespace, table, tableMetadata, true, false);
}
5 participants
Description
This PR revisits the logic of the
importTable()method in Consensus Commit.Related issues and/or PRs
N/A
Changes made
ConsensusCommitAdmin.importTable()method to calladmin.importTable()first, then add columns usingaddNewColumnToTable()instead ofaddRawColumnToTable().getImportTableMetadata()andaddRawColumnToTable()methods from the admin API interfaces and all implementationsChecklist
Additional notes (optional)
N/A
Release notes
N/A