Add create, truncate and delete table implementation to JDBCAdmin #270

Torch3333 · 2021-08-10T09:26:28Z

This completes the implementation of the JDBCStorageAdmin and refactor integration test to rely on it.
cf. https://scalar-labs.atlassian.net/browse/DLT-9371

…ate_truncate_delete_to_jdbc_admin # Conflicts: # core/src/integration-test/java/com/scalar/db/storage/jdbc/test/TestEnv.java

…ate_truncate_delete_to_jdbc_admin

Torch3333 · 2021-08-11T04:26:06Z

core/src/integration-test/java/com/scalar/db/storage/jdbc/test/TestEnv.java

-import java.util.Set;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
-import javax.sql.DataSource;
 import org.apache.commons.dbcp2.BasicDataSource;

 @SuppressFBWarnings("SQL_NONCONSTANT_STRING_PASSED_TO_EXECUTE")
 public class TestEnv implements Closeable {


A lof of changes in the PR were due to the refactoring of this integration test class to now rely on the JdbcDatabaseAdmin to insert test table.

Torch3333 · 2021-08-11T04:27:46Z

core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabaseAdmin.java

 import java.util.Map;
 import java.util.Optional;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import javax.annotation.concurrent.ThreadSafe;
 import org.apache.commons.dbcp2.BasicDataSource;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

 @ThreadSafe
 public class JdbcDatabaseAdmin implements DistributedStorageAdmin {


Most of the important changes are here.

Torch3333 · 2021-08-11T04:37:03Z

core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabaseAdmin.java

  public void dropTable(String namespace, String table) throws ExecutionException {
-    throw new UnsupportedOperationException("implement later");
+    dropTableInternal(namespace, table);
+    metadataManager.deleteTableMetadata(namespace, table);
+    if (metadataManager.getTableNames(namespace).isEmpty()) {
+      dropSchema(namespace);
+    }
  }


I added the new method TableMetadataManager.getTablesNames() because I needed a way to know if there was any tables associated with the given namespace. I figured this was more proper to rely on the MetadataManager data than run specific sql queries to check if a namespace contains some tables.

brfrn169

LGTM overall! Thank you!

I left some comments and suggestions. Please check them when you get a chance!

core/src/main/java/com/scalar/db/api/DistributedStorageAdmin.java

core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabaseAdmin.java

core/src/main/java/com/scalar/db/util/Utility.java

core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabaseAdmin.java

…the end of JdbcTableMetadataManager.deleteMetadataTable()

brfrn169

Thank you! I just left several comments. Please take a look!

core/src/integration-test/java/com/scalar/db/storage/jdbc/test/TestEnv.java

core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabaseAdmin.java

core/src/main/java/com/scalar/db/storage/jdbc/JdbcTableMetadataManager.java

brfrn169

LGTM! Thank you!

feeblefakie

Overall looking good! Thank you!
Left some questions and a minor suggestion.

core/src/main/java/com/scalar/db/storage/common/TableMetadataManager.java

core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabaseAdmin.java

feeblefakie · 2021-08-13T04:48:43Z

core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabaseAdmin.java

+        createTableInternal(connection, namespace, table, metadata);
+        createIndex(connection, namespace, table, metadata);
+      } catch (SQLException e) {
+        connection.rollback();


Just a question.
Are we supposed to call rollback for DDL?
Here, it's called, but it seems like it is not called in other places like below.
https://github.com/scalar-labs/scalardb/pull/270/files#diff-26e58dccc9f536e4794af12538f00972936415bb04ba1b0d5d1f6b903f55e2d5R169

Are there any particular criteria when to call it and when to not?

Are we supposed to call rollback for DDL?

I assumed rollback was supported for all kind of statements but it's not the case for DDL. Oracle and Mysql do not support DDL statement transaction rollback while Postgresql and Sqlserver do.

Here, it's called, but it seems like it is not called in other places like below.
https://github.com/scalar-labs/scalardb/pull/270/files#diff-26e58dccc9f536e4794af12538f00972936415bb04ba1b0d5d1f6b903f55e2d5R169

Are there any particular criteria when to call it and when to not?

connection.rollback() should only be called if connection.setAutoCommit(false) was called beforehand.
By default, auto-commit is true so every statement executed will be auto-committed right after. If an errors occurs in this mode, it will be roll-backed transparently.

Got it. Thank you!

feeblefakie

LGTM! Thank you!

brfrn169

LGTM! Thank you!

Torch3333 · 2021-08-17T05:31:16Z

Please wait before merging this, I want to clarify something first with @brfrn169

…ked by passing the namespacePrefix concatenated to the namespace

…:scalar-labs/scalardb into add_create_truncate_delete_to_jdbc_admin

Torch3333 · 2021-08-17T05:56:18Z

@brfrn169
@feeblefakie
I found out that was an inconsistency in the code where one of the JDBCTableMetadataManager method was called by concatenating "namespacePrefix + namespace" while only the "namespace" value should be passed.
Sorry for this, please have a look again 🙇

core/src/main/java/com/scalar/db/storage/jdbc/JdbcTableMetadataManager.java

…aManager.java Simplify method usage Co-authored-by: Toshihiro Suzuki <[email protected]>

brfrn169

LGTM! Thank you!

Torch3333 · 2021-08-17T07:15:41Z

Circle-ci integration-test-for-jdbc-oracle is failing. I am investigating it.

brfrn169 · 2021-08-17T07:21:30Z

@Torch3333 I re-ran the Oracle integration test and it was successful but it seems like it took more time than usual. I will run it again.

brfrn169 · 2021-08-17T08:10:25Z

It looks like the last Oracle integration finished as the usual time 👍

feeblefakie

LGTM! Thank you!

Vincent Guilpain added 6 commits August 4, 2021 16:39

Add create, truncate, delete to cassandraAdmin

2cb37e9

Update integration test to use JdbcAdmin

0d3dc3f

Merge branch 'master' of github.com:scalar-labs/scalardb into add_cre…

949a2a6

…ate_truncate_delete_to_jdbc_admin # Conflicts: # core/src/integration-test/java/com/scalar/db/storage/jdbc/test/TestEnv.java

Fix spotbugs errors

6744dbb

Add getTables unit test

707b94c

Add missing unit test for JdbcDatabaseAdmin

9de84c6

Torch3333 added the enhancement New feature or request label Aug 10, 2021

Torch3333 self-assigned this Aug 10, 2021

Vincent Guilpain added 2 commits August 11, 2021 10:39

Merge branch 'master' of github.com:scalar-labs/scalardb into add_cre…

b287a7f

…ate_truncate_delete_to_jdbc_admin

Refactor server integration test to use the updated TestEnv

73c63e1

Torch3333 commented Aug 11, 2021

View reviewed changes

Torch3333 changed the title ~~WIP Add create, truncate and delete table implementation to JDBCAdmin~~ Add create, truncate and delete table implementation to JDBCAdmin Aug 11, 2021

Torch3333 marked this pull request as ready for review August 11, 2021 04:40

Torch3333 requested review from brfrn169 and feeblefakie August 11, 2021 04:40

brfrn169 reviewed Aug 11, 2021

View reviewed changes

Vincent Guilpain added 5 commits August 11, 2021 17:46

Replace schema by namespace in the method parameters

ff7ec91

Delete the metadata table if it does not contain metadata anymore at …

013e27d

…the end of JdbcTableMetadataManager.deleteMetadataTable()

Do not sort indexes for creation

f56c520

Add blank line

b38babc

Suppress spotbugs false positive

6ec1540

Torch3333 requested a review from brfrn169 August 12, 2021 08:06

brfrn169 reviewed Aug 12, 2021

View reviewed changes

Remove unused Datasource class attribute

737298b

Torch3333 requested a review from brfrn169 August 13, 2021 02:04

brfrn169 previously approved these changes Aug 13, 2021

View reviewed changes

feeblefakie reviewed Aug 13, 2021

View reviewed changes

Vincent Guilpain added 2 commits August 17, 2021 10:32

Fix javadoc typo

2142f24

Rollback failed connection.commit() calls

f84db9a

Torch3333 dismissed brfrn169’s stale review via f84db9a August 17, 2021 01:41

Torch3333 requested a review from feeblefakie August 17, 2021 01:41

feeblefakie previously approved these changes Aug 17, 2021

View reviewed changes

Merge branch 'master' into add_create_truncate_delete_to_jdbc_admin

060235b

brfrn169 previously approved these changes Aug 17, 2021

View reviewed changes

Merge branch 'master' into add_create_truncate_delete_to_jdbc_admin

feadc35

Vincent Guilpain added 2 commits August 17, 2021 14:48

Fix inconsistency of JdbcTableMetadataManager.getTableMetadata() invo…

e71d3bb

…ked by passing the namespacePrefix concatenated to the namespace

Merge branch 'add_create_truncate_delete_to_jdbc_admin' of github.com…

c6c997b

…:scalar-labs/scalardb into add_create_truncate_delete_to_jdbc_admin

Torch3333 dismissed stale reviews from brfrn169 and feeblefakie via c6c997b August 17, 2021 05:49

Torch3333 requested a review from brfrn169 August 17, 2021 05:51

brfrn169 reviewed Aug 17, 2021

View reviewed changes

core/src/main/java/com/scalar/db/storage/jdbc/JdbcTableMetadataManager.java Outdated Show resolved Hide resolved

Update core/src/main/java/com/scalar/db/storage/jdbc/JdbcTableMetadat…

14c3301

…aManager.java Simplify method usage Co-authored-by: Toshihiro Suzuki <[email protected]>

brfrn169 approved these changes Aug 17, 2021

View reviewed changes

feeblefakie approved these changes Aug 18, 2021

View reviewed changes

Merge branch 'master' into add_create_truncate_delete_to_jdbc_admin

3041d78

brfrn169 merged commit cd92e6b into master Aug 18, 2021

brfrn169 deleted the add_create_truncate_delete_to_jdbc_admin branch August 18, 2021 06:20

brfrn169 pushed a commit that referenced this pull request Aug 25, 2021

Add create, truncate and delete table implementation to JDBCAdmin (#270)

57a683a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add create, truncate and delete table implementation to JDBCAdmin #270

Add create, truncate and delete table implementation to JDBCAdmin #270

Torch3333 commented Aug 10, 2021 •

edited

Loading

Torch3333 Aug 11, 2021 •

edited

Loading

Torch3333 Aug 11, 2021 •

edited

Loading

Torch3333 Aug 11, 2021

brfrn169 left a comment

brfrn169 left a comment

brfrn169 left a comment

feeblefakie left a comment

feeblefakie Aug 13, 2021

Torch3333 Aug 13, 2021 •

edited

Loading

feeblefakie Aug 13, 2021

feeblefakie left a comment

brfrn169 left a comment

Torch3333 commented Aug 17, 2021

Torch3333 commented Aug 17, 2021

brfrn169 left a comment

Torch3333 commented Aug 17, 2021

brfrn169 commented Aug 17, 2021 •

edited

Loading

brfrn169 commented Aug 17, 2021

feeblefakie left a comment

Add create, truncate and delete table implementation to JDBCAdmin #270

Add create, truncate and delete table implementation to JDBCAdmin #270

Conversation

Torch3333 commented Aug 10, 2021 • edited Loading

Torch3333 Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Torch3333 Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Torch3333 Aug 11, 2021

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

feeblefakie Aug 13, 2021

Choose a reason for hiding this comment

Torch3333 Aug 13, 2021 • edited Loading

Choose a reason for hiding this comment

feeblefakie Aug 13, 2021

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

Torch3333 commented Aug 17, 2021

Torch3333 commented Aug 17, 2021

brfrn169 left a comment

Choose a reason for hiding this comment

Torch3333 commented Aug 17, 2021

brfrn169 commented Aug 17, 2021 • edited Loading

brfrn169 commented Aug 17, 2021

feeblefakie left a comment

Choose a reason for hiding this comment

Torch3333 commented Aug 10, 2021 •

edited

Loading

Torch3333 Aug 11, 2021 •

edited

Loading

Torch3333 Aug 11, 2021 •

edited

Loading

Torch3333 Aug 13, 2021 •

edited

Loading

brfrn169 commented Aug 17, 2021 •

edited

Loading