Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create namespace metadata when importing tables #1260

Merged
merged 3 commits into from
Nov 17, 2023
Merged

Create namespace metadata when importing tables #1260

merged 3 commits into from
Nov 17, 2023

Conversation

Torch3333
Copy link
Contributor

@Torch3333 Torch3333 commented Nov 1, 2023 •
edited
Loading

Description

In the table importing feature, we need to create namespace metadata if they do not exist when importing a table.

Related issues and/or PRs

Changes made

  • Updates the JdbcAdmin and ConsensuCommitAdmin importTable() method to create the metadata of the imported table namespace.
  • Updates the signature to admin.importTable(String namespace, String table, Map<String, String> options): the options parameter is added. This parameter is required to pass specific options when creating namespace metadata for the Cassandra and Dynamo Admin. Even though the import feature is not supported for these storages as of now, we update the signature to be future-proof.

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes.
  • Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
  • Tests (unit, integration, etc.) have been added for the changes.
  • My changes generate no new warnings.
  • Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

Release notes

N/A

@Torch3333 Torch3333 self-assigned this Nov 1, 2023
@Torch3333 Torch3333 force-pushed the add_namespace_metadata_when_importing_table branch 3 times, most recently from b02222a to 6ac0faa Compare November 7, 2023 00:10
@Torch3333 Torch3333 force-pushed the add_namespace_metadata_when_importing_table branch from 6ac0faa to 8f1495f Compare November 7, 2023 21:12
Torch3333
Torch3333 commented Nov 7, 2023
View reviewed changes
core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java
Comment on lines +546 to 553
public void importTable(String namespace, String table, Map<String, String> options)
throws ExecutionException {
try (Connection connection = dataSource.getConnection()) {
TableMetadata tableMetadata = getImportTableMetadata(namespace, table);
createNamespacesTableIfNotExists(connection);
upsertIntoNamespacesTable(connection, namespace);
addTableMetadata(connection, namespace, table, tableMetadata, true, false);
} catch (SQLException | ExecutionException e) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the JdbcAdmin, Metadata for the namespace is created when importing a table.

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java
@@ -231,8 +231,8 @@ public void importTable(String namespace, String table) throws ExecutionExceptio
}

// add ScalarDB metadata
admin.repairTable(
namespace, table, buildTransactionTableMetadata(tableMetadata), ImmutableMap.of());
admin.repairNamespace(namespace, options);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the ConsensusCommitAdmin, we create the namespace metadata using repair

...t/src/main/java/com/scalar/db/api/DistributedStorageAdminImportTableIntegrationTestBase.java
@@ -79,7 +79,6 @@ protected abstract Map<String, TableMetadata> createExistingDatabaseWithAllDataT
@Test
public void importTable_ShouldWorkProperly() throws Exception {
// Arrange
admin.repairNamespace(getNamespace(), getCreationOptions());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove several admin.repairNamespace() or admin.createNamespace() call from the DistributedStorageAdminImportTableIntegrationTestBase.java and DistributedTransactionAdminImportTableIntegrationTestBase.java that were put as a temporary measure for these tests to run.

...test/java/com/scalar/db/transaction/jdbc/JdbcTransactionAdminImportTableIntegrationTest.java
@@ -1,60 +0,0 @@
package com.scalar.db.transaction.jdbc;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JDBC transaction admin will be deleted in this release so we can remove this test class.

@Torch3333 Torch3333 marked this pull request as ready for review November 7, 2023 21:39
komamitsu
komamitsu approved these changes Nov 9, 2023
View reviewed changes
Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

brfrn169
brfrn169 reviewed Nov 9, 2023
View reviewed changes
schema-loader/src/main/java/com/scalar/db/schemaloader/SchemaOperator.java Outdated
@@ -413,9 +414,9 @@ public void importTables(List<ImportTableSchema> tableSchemaList) throws SchemaL
String table = tableSchema.getTable();
try {
if (tableSchema.isTransactionTable()) {
transactionAdmin.get().importTable(namespace, table);
transactionAdmin.get().importTable(namespace, table, Collections.emptyMap());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add Map<String, String> options to ImportTableSchema, and pass it to importTable() as the third argument. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.
Similarly to TableSchema, we could define custom options for each table in the schema file or pass options globally via CLI argument that will be applied to each table.
I realized that setting a given option via the CLI and also in the schema file will throw an exception because of this (the same option key will be added twice in the list builder and this is not allowed by default). I don't know if this behavior is intended eventually. If an option is set globally as CLI argument and in the schema file, using the value specified in the schema file is more user-friendly in my opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an option is set globally as CLI argument and in the schema file, using the value specified in the schema file is more user-friendly in my opinion.

I agree. That's more user-friendly behavior.

Copy link
Contributor Author

@Torch3333 Torch3333 Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the options handling for the Schema Loader --import command in b6e16cf and it follows this principle.

If an option is set globally as CLI argument and in the schema file, using the value specified in the schema file is more user-friendly in my opinion.

I will update the options handling for the other commands besides --import to follow this principle (all using the TableSchema class) in another PR.
Thanks for your comment and PTAL when you get the chance.

jnmt
jnmt approved these changes Nov 9, 2023
View reviewed changes
Copy link
Contributor

@jnmt jnmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

feeblefakie
feeblefakie approved these changes Nov 11, 2023
View reviewed changes
Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

@Torch3333 Torch3333 force-pushed the add_namespace_metadata_when_importing_table branch from 6bc2078 to 7e7b95b Compare November 15, 2023 22:33
@Torch3333 Torch3333 force-pushed the add_namespace_metadata_when_importing_table branch from 7e7b95b to b6e16cf Compare November 15, 2023 23:20
@Torch3333 Torch3333 requested a review from brfrn169 November 15, 2023 23:51
brfrn169
brfrn169 approved these changes Nov 16, 2023
View reviewed changes
Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

@brfrn169 brfrn169 merged commit 012f941 into master Nov 17, 2023
23 checks passed
@brfrn169 brfrn169 deleted the add_namespace_metadata_when_importing_table branch November 17, 2023 02:10
@jnmt jnmt mentioned this pull request Nov 22, 2023
Merged
@brfrn169 brfrn169 mentioned this pull request Nov 29, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnmt jnmt jnmt approved these changes

@feeblefakie feeblefakie feeblefakie approved these changes

@brfrn169 brfrn169 brfrn169 approved these changes

@komamitsu komamitsu komamitsu approved these changes

Assignees

@Torch3333 Torch3333

Labels
improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Torch3333 @komamitsu @brfrn169 @feeblefakie @jnmt