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

Add create, truncate and delete table implementation to Cassandra Admin #278

Merged
merged 9 commits into from
Aug 24, 2021
Merged

Add create, truncate and delete table implementation to Cassandra Admin #278

merged 9 commits into from
Aug 24, 2021

Conversation

Torch3333
Copy link
Contributor

@Torch3333 Torch3333 commented Aug 18, 2021 •
edited
Loading

This completes the implementation for the CassandraAdmin and the CassandraTableMetadataManager.
Please have a look when you get a chance.

Vincent Guilpain added 3 commits August 13, 2021 15:25
Add create, truncate, delete table to CassandraAdmin
685c15f
Set namespace prefix when instantiating cassandra metadata manager
f7a7c42
Merge branch 'master' of github.com:scalar-labs/scalardb into add_cre…
7f3dec2 
…ate_truncate_delete_implementation_to_cassandra_admin

# Conflicts:
#	core/src/main/java/com/scalar/db/api/DistributedStorageAdmin.java
#	core/src/main/java/com/scalar/db/storage/cassandra/CassandraTableMetadataManager.java
@Torch3333 Torch3333 self-assigned this Aug 18, 2021
@Torch3333 Torch3333 added the enhancement New feature or request label Aug 18, 2021
@Torch3333 Torch3333 force-pushed the add_create_truncate_delete_implementation_to_cassandra_admin branch from f316f58 to 97196fe Compare August 18, 2021 08:37
Torch3333
Torch3333 commented Aug 18, 2021
View reviewed changes
...-test/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitIntegrationTestBase.java
@@ -62,6 +78,61 @@
// assume that recovery is a spied object
private RecoveryHandler recovery;

protected static void createTables() throws ExecutionException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once thing I omitted to say in the JDBCAdmin PR is that I am refactoring the ConsensusCommitIntegrationTestXXX classes temporarily to make use of the few XXXAdmin implementations that are already completed.
Once all the XXXAdmin implementations will be completed, I should be able to simplify things further.

@Torch3333 Torch3333 changed the title WIP Add create truncate delete implementation to cassandra admin WIP Add create, truncate and delete table implementation to cassandra admin Aug 18, 2021
@Torch3333 Torch3333 changed the title WIP Add create, truncate and delete table implementation to cassandra admin WIP Add create, truncate and delete table implementation to Cassandra Admin Aug 18, 2021
@Torch3333 Torch3333 changed the title WIP Add create, truncate and delete table implementation to Cassandra Admin Add create, truncate and delete table implementation to Cassandra Admin Aug 18, 2021
@Torch3333 Torch3333 marked this pull request as ready for review August 18, 2021 09:01
brfrn169
brfrn169 reviewed Aug 18, 2021
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.

Thank you! Overall LGTM! Just left some comments and suggestions. Please check them!

...src/integration-test/java/com/scalar/db/storage/cassandra/CassandraAdminIntegrationTest.java Outdated Show resolved Hide resolved
core/src/integration-test/java/com/scalar/db/storage/cassandra/CassandraIntegrationTest.java Outdated Show resolved Hide resolved
core/src/main/java/com/scalar/db/io/DataType.java Outdated Show resolved Hide resolved
core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java Outdated Show resolved Hide resolved
core/src/main/java/com/scalar/db/storage/cassandra/CassandraTableMetadataManager.java Outdated Show resolved Hide resolved
core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java Outdated Show resolved Hide resolved
core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java Outdated Show resolved Hide resolved
core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java Outdated Show resolved Hide resolved
@Torch3333 Torch3333 requested a review from brfrn169 August 19, 2021 02:22
brfrn169
brfrn169 reviewed Aug 19, 2021
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.

Just left a question and very minor suggestions. Please check them! Thanks.

core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java Outdated Show resolved Hide resolved
core/src/main/java/com/scalar/db/storage/cassandra/CassandraTableMetadataManager.java Outdated Show resolved Hide resolved
core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java Show resolved Hide resolved
brfrn169
brfrn169 previously approved these changes Aug 19, 2021
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!

feeblefakie
feeblefakie reviewed Aug 23, 2021
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.

Very sorry for the late review.
Overall looking good! Thank you!
Left some comments and questions.

core/src/main/java/com/scalar/db/io/DataType.java Outdated Show resolved Hide resolved
core/src/main/java/com/scalar/db/storage/cassandra/CassandraTableMetadataManager.java Outdated Show resolved Hide resolved
core/src/main/java/com/scalar/db/storage/cassandra/CassandraTableMetadataManager.java Outdated Show resolved Hide resolved
...src/integration-test/java/com/scalar/db/storage/cassandra/CassandraAdminIntegrationTest.java Show resolved Hide resolved
core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java
}

@Override
public void createTable(
String namespace, String table, TableMetadata metadata, Map<String, String> options)
throws ExecutionException {
throw new UnsupportedOperationException("implement later");
String fullKeyspace = fullKeyspace(namespace);
createKeyspaceIfNotExists(fullKeyspace, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if createTable should create a keyspace if it doesn't exist.
Users try to create a table with options (usually) specifically for table creation, not options for keyspace, so in such a case, it will create a keyspace with the default options most of the time? (but we don't know users want the default options for the keyspace)

I think simply throwing an exception might be safer and more intuitive.
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.

@feeblefakie @brfrn169
I see. If you are available, let's discuss this point after the daily meeting today.

core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java Outdated Show resolved Hide resolved
core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java Outdated Show resolved Hide resolved
@Torch3333
Copy link
Contributor Author

As @brfrn169 suggested, let's merge this first then refactor the DistributedStorageAdmin in subsequent PRs.

brfrn169
brfrn169 approved these changes Aug 24, 2021
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!

feeblefakie
feeblefakie approved these changes Aug 24, 2021
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!

@feeblefakie feeblefakie merged commit 7b7c27b into master Aug 24, 2021
@feeblefakie feeblefakie deleted the add_create_truncate_delete_implementation_to_cassandra_admin branch August 24, 2021 09:09
brfrn169 pushed a commit that referenced this pull request Aug 25, 2021
@Torch3333 @brfrn169
Add create, truncate and delete table implementation to Cassandra Adm…
65715fe 
…in (#278)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feeblefakie feeblefakie feeblefakie approved these changes

@brfrn169 brfrn169 brfrn169 approved these changes

Assignees

@Torch3333 Torch3333

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Torch3333 @brfrn169 @feeblefakie