Unify admin behavior #856

brfrn169 · 2023-04-26T03:03:17Z

This PR unifies the behavior of the admin implementations (e.g. an error is thrown when dropping a namespace but the namespace has some tables). I'll add inline comments for the details. Please take a look!

brfrn169 · 2023-04-26T03:04:32Z

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

@@ -15,6 +15,7 @@ public interface Admin {
   *


Documented the unified admin behaviors.

brfrn169 · 2023-04-26T03:04:59Z

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

@@ -14,6 +14,8 @@ public interface DistributedTransactionAdmin extends Admin {
   * Creates coordinator namespace and tables.


Ditto. Documented the unified admin behaviors.

brfrn169 · 2023-04-26T03:06:16Z

core/src/main/java/com/scalar/db/common/CheckedDistributedStorageAdmin.java

+import java.util.Set;
+import javax.annotation.Nullable;
+
+public class CheckedDistributedStorageAdmin implements DistributedStorageAdmin {


This CheckedDistributedStorageAdmin is the main component in this PR.

This class wraps the DistributedStorageAdmin implementations, and adds some checks to them.

brfrn169 · 2023-04-26T03:07:21Z

core/src/main/java/com/scalar/db/common/CheckedDistributedStorageAdmin.java

+  /**
+   * Whether to check if the namespace exists or not. Set false when the storage does not support
+   * namespaces.
+   */
+  private final boolean checkNamespace;


Since we have several implementations that don't support namespaces (DynamoDB, SQLite), we set it to false for them.

Maybe we can remove it after supporting the namespace listing feature. (CC: @Torch3333 )

Maybe we can remove it after supporting the namespace listing feature.

Agreed.

brfrn169 · 2023-04-26T04:35:29Z

core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java

  }

  @Override
  public void addNewColumnToTable(
      String namespace, String table, String columnName, DataType columnType)
      throws ExecutionException {
    try {
-      if (getTableMetadata(namespace, table).getColumnNames().contains(columnName)) {


Since we have checks in CheckedDistributedStorageAdmin, removed these checks from the admin implementations.

brfrn169 · 2023-04-26T04:37:15Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java

@@ -54,18 +54,30 @@ public ConsensusCommitAdmin(DatabaseConfig databaseConfig) {

  @Override
  public void createCoordinatorTables(Map<String, String> options) throws ExecutionException {
+    if (coordinatorTablesExist()) {


Also, added some checks to ConsensusCommitAdmin.

Torch3333 · 2023-04-26T07:09:07Z

core/src/main/java/com/scalar/db/common/CheckedDistributedStorageAdmin.java

+      throw new IllegalArgumentException("Namespace does not exist: " + namespace);
+    }
+    if (tableExists(namespace, table)) {
+      throw new IllegalArgumentException("Table already exists: " + namespace + "." + table);


Suggested change

throw new IllegalArgumentException("Table already exists: " + namespace + "." + table);

throw new IllegalArgumentException("Table already exists: " + ScalarDbUtils.getFullTableName(namespace, table));

Should the utility function be used all the time?

Fixed in fcb2338. Thanks.

Torch3333 · 2023-04-26T07:09:23Z

core/src/main/java/com/scalar/db/common/CheckedDistributedStorageAdmin.java

+  @Override
+  public void dropTable(String namespace, String table) throws ExecutionException {
+    if (!tableExists(namespace, table)) {
+      throw new IllegalArgumentException("Table does not exist: " + namespace + "." + table);


Suggested change

throw new IllegalArgumentException("Table does not exist: " + namespace + "." + table);

throw new IllegalArgumentException("Table does not exist: " + ScalarDbUtils.getFullTableName(namespace, table));

Likewise.

Fixed in fcb2338. Thanks.

Torch3333 · 2023-04-26T07:09:45Z

core/src/main/java/com/scalar/db/common/CheckedDistributedStorageAdmin.java

+  @Override
+  public void truncateTable(String namespace, String table) throws ExecutionException {
+    if (!tableExists(namespace, table)) {
+      throw new IllegalArgumentException("Table does not exist: " + namespace + "." + table);


Suggested change

throw new IllegalArgumentException("Table does not exist: " + namespace + "." + table);

throw new IllegalArgumentException("Table does not exist: " + ScalarDbUtils.getFullTableName(namespace, table));

Likewise

Fixed in fcb2338. Thanks.

Torch3333 · 2023-04-26T07:10:06Z

core/src/main/java/com/scalar/db/common/CheckedDistributedStorageAdmin.java

+  public void dropIndex(String namespace, String table, String columnName)
+      throws ExecutionException {
+    if (!tableExists(namespace, table)) {
+      throw new IllegalArgumentException("Table does not exist: " + namespace + "." + table);


Suggested change

throw new IllegalArgumentException("Table does not exist: " + namespace + "." + table);

throw new IllegalArgumentException("Table does not exist: " + ScalarDbUtils.getFullTableName(namespace, table));

Likewise.

Fixed in fcb2338. Thanks.

Torch3333 · 2023-04-26T07:10:53Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java

@@ -165,6 +177,9 @@ public void addNewColumnToTable(
      String namespace, String table, String columnName, DataType columnType)
      throws ExecutionException {
    TableMetadata tableMetadata = getTableMetadata(namespace, table);
+    if (tableMetadata == null) {
+      throw new IllegalArgumentException("Table does not exist: " + namespace + "." + table);


Suggested change

throw new IllegalArgumentException("Table does not exist: " + namespace + "." + table);

throw new IllegalArgumentException("Table does not exist: " + ScalarDbUtils.getFullTableName(namespace, table));

Likewise.

Fixed in fbe7195. Thanks.

Torch3333 · 2023-04-26T07:23:15Z

core/src/main/java/com/scalar/db/common/CheckedDistributedStorageAdmin.java

+  /**
+   * Whether to check if the namespace exists or not. Set false when the storage does not support
+   * namespaces.
+   */
+  private final boolean checkNamespace;


Maybe we can remove it after supporting the namespace listing feature.

Agreed.

Torch3333 · 2023-04-26T07:42:56Z

core/src/main/java/com/scalar/db/common/CheckedDistributedStorageAdmin.java

+      String namespace, String table, TableMetadata metadata, Map<String, String> options)
+      throws ExecutionException {
+    if (checkNamespace && !namespaceExists(namespace)) {
+      throw new IllegalArgumentException("Namespace does not exist: " + namespace);


When the underlying admin is the DynamoAdmin used with a namespace prefix, the value of the namespace argument used in this class here is not prefixed. The error message may be confusing since it will mention the unprefixed namespace.
For clarity, mentioning the prefixed namespace in the error message throughout this class would be great. What do you think ?

I have the same concern about using the unprefixed namespace in the error message in the ConsensusCommitAdmin

I think since users pass the logical name (the unprefixed name) to the APIs, I feel it's consistent and natural for the error messages to use the logical name. In CheckedDistributedStorageAdmin, the IllegalArgumentException always has the logical name. However, when an error occurs, the message of the exception also uses the logical name, but the cause of the exception should have the physical name. So we can use it for debugging.

What do you think?

Indeed, this makes sense. Let's keep it as is. Thank you.

komamitsu · 2023-04-26T09:17:39Z

core/src/main/java/com/scalar/db/common/CheckedDistributedStorageAdmin.java

+  }
+
+  @Override
+  public void dropTable(String namespace, String table) throws ExecutionException {


These xxxxTable() and xxxxIndex() should check namespace as well as createTable()? I think either is fine as long as they're consistent.

In the operations that assumes the the target table exists, we don't have the namespace check. In that sense, they should be consistent. And the table check contains the namespace check, so I think that should be okay.

komamitsu · 2023-04-26T09:26:41Z

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

@@ -77,7 +77,7 @@ public String dropNamespaceSql(String namespace) {
  @Override
  public void dropNamespaceTranslateSQLException(SQLException e, String namespace)
      throws ExecutionException {
-    throw new ExecutionException(String.format("error dropping the schema %s", namespace), e);
+    throw new ExecutionException("dropping the schema failed: %s" + namespace, e);


%s should be removed?

Ah my bad. Good catch! Thanks. Fixed in 87166ae.

komamitsu · 2023-04-26T09:26:58Z

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

@@ -86,7 +86,7 @@ public String dropNamespaceSql(String namespace) {
  @Override
  public void dropNamespaceTranslateSQLException(SQLException e, String namespace)
      throws ExecutionException {
-    throw new ExecutionException(String.format("error dropping the schema %s", namespace), e);
+    throw new ExecutionException("dropping the schema failed: %s" + namespace, e);


Same as above

Fixed in 87166ae. Thanks.

komamitsu · 2023-04-26T09:27:15Z

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

@@ -78,7 +78,7 @@ public String dropNamespaceSql(String namespace) {
  @Override
  public void dropNamespaceTranslateSQLException(SQLException e, String namespace)
      throws ExecutionException {
-    throw new ExecutionException(String.format("error dropping the schema %s", namespace), e);
+    throw new ExecutionException("dropping the schema failed: %s" + namespace, e);


Same as above

Fixed in 87166ae. Thanks.

feeblefakie

LGTM! Thank you!

komamitsu

LGTM! 👍

Torch3333

LGTM, thank you!

kota2and3kan

LGTM! Thank you!
(Sorry for the late review...)

For your reference, I was able to get the error as expected as follows (i.e., the bug I found is fixed).

Create schemas/tables on MySQL

0: scalardb>
0: scalardb> CREATE COORDINATOR TABLES;
No rows affected (1.027 seconds)
0: scalardb>
0: scalardb>
0: scalardb> CREATE NAMESPACE s0;
No rows affected (0.03 seconds)
0: scalardb>
0: scalardb>
0: scalardb> CREATE NAMESPACE s1;
No rows affected (0.044 seconds)
0: scalardb>
0: scalardb>
0: scalardb> CREATE TABLE s0.t0 (c1 INT PRIMARY KEY, c2 INT);
No rows affected (0.426 seconds)
0: scalardb>
0: scalardb>
0: scalardb> CREATE TABLE s1.t1 (c1 INT PRIMARY KEY, c2 INT);
No rows affected (0.302 seconds)
0: scalardb>

Check schemas/tables on MySQL

mysql> show databases;
+--------------------+
| Database           |
+--------------------+
| coordinator        |
| information_schema |
| my_database        |
| mysql              |
| performance_schema |
| s0                 |
| s1                 |
| scalardb           |
| sys                |
+--------------------+
9 rows in set (0.00 sec)

mysql>
mysql> show tables from s0;
+--------------+
| Tables_in_s0 |
+--------------+
| t0           |
+--------------+
1 row in set (0.00 sec)

mysql>
mysql> show tables from s1;
+--------------+
| Tables_in_s1 |
+--------------+
| t1           |
+--------------+
1 row in set (0.00 sec)

Check table metadata

mysql> select * from scalardb.metadata;
+-------------------+------------------------+-----------+-----------+------------------+---------+------------------+
| full_table_name   | column_name            | data_type | key_type  | clustering_order | indexed | ordinal_position |
+-------------------+------------------------+-----------+-----------+------------------+---------+------------------+
| coordinator.state | tx_created_at          | BIGINT    | NULL      | NULL             |       0 |                3 |
| coordinator.state | tx_id                  | TEXT      | PARTITION | NULL             |       0 |                1 |
| coordinator.state | tx_state               | INT       | NULL      | NULL             |       0 |                2 |
| s0.t0             | before_c2              | INT       | NULL      | NULL             |       0 |               13 |
| s0.t0             | before_tx_committed_at | BIGINT    | NULL      | NULL             |       0 |               12 |
| s0.t0             | before_tx_id           | TEXT      | NULL      | NULL             |       0 |                8 |
| s0.t0             | before_tx_prepared_at  | BIGINT    | NULL      | NULL             |       0 |               11 |
| s0.t0             | before_tx_state        | INT       | NULL      | NULL             |       0 |                9 |
| s0.t0             | before_tx_version      | INT       | NULL      | NULL             |       0 |               10 |
| s0.t0             | c1                     | INT       | PARTITION | NULL             |       0 |                1 |
| s0.t0             | c2                     | INT       | NULL      | NULL             |       0 |                2 |
| s0.t0             | tx_committed_at        | BIGINT    | NULL      | NULL             |       0 |                7 |
| s0.t0             | tx_id                  | TEXT      | NULL      | NULL             |       0 |                3 |
| s0.t0             | tx_prepared_at         | BIGINT    | NULL      | NULL             |       0 |                6 |
| s0.t0             | tx_state               | INT       | NULL      | NULL             |       0 |                4 |
| s0.t0             | tx_version             | INT       | NULL      | NULL             |       0 |                5 |
| s1.t1             | before_c2              | INT       | NULL      | NULL             |       0 |               13 |
| s1.t1             | before_tx_committed_at | BIGINT    | NULL      | NULL             |       0 |               12 |
| s1.t1             | before_tx_id           | TEXT      | NULL      | NULL             |       0 |                8 |
| s1.t1             | before_tx_prepared_at  | BIGINT    | NULL      | NULL             |       0 |               11 |
| s1.t1             | before_tx_state        | INT       | NULL      | NULL             |       0 |                9 |
| s1.t1             | before_tx_version      | INT       | NULL      | NULL             |       0 |               10 |
| s1.t1             | c1                     | INT       | PARTITION | NULL             |       0 |                1 |
| s1.t1             | c2                     | INT       | NULL      | NULL             |       0 |                2 |
| s1.t1             | tx_committed_at        | BIGINT    | NULL      | NULL             |       0 |                7 |
| s1.t1             | tx_id                  | TEXT      | NULL      | NULL             |       0 |                3 |
| s1.t1             | tx_prepared_at         | BIGINT    | NULL      | NULL             |       0 |                6 |
| s1.t1             | tx_state               | INT       | NULL      | NULL             |       0 |                4 |
| s1.t1             | tx_version             | INT       | NULL      | NULL             |       0 |                5 |
+-------------------+------------------------+-----------+-----------+------------------+---------+------------------+
29 rows in set (0.00 sec)

Drop namespaces with/without CASCADE option

0: scalardb> DROP NAMESPACE s0;
Error: Invalid query (INVALID_ARGUMENT: Namespace is not empty: s0, [t0]) (state=42000,code=501)
0: scalardb>
0: scalardb> DROP NAMESPACE s1 CASCADE;
No rows affected (0.149 seconds)
0: scalardb>

When I run the DROP SCHEMA without CASCADE option, ScalarDB returns error as expected.

Check schemas/tables on MySQL

mysql> show databases;
+--------------------+
| Database           |
+--------------------+
| coordinator        |
| information_schema |
| my_database        |
| mysql              |
| performance_schema |
| s0                 |
| scalardb           |
| sys                |
+--------------------+
8 rows in set (0.00 sec)

mysql>
mysql> show tables from s0;
+--------------+
| Tables_in_s0 |
+--------------+
| t0           |
+--------------+
1 row in set (0.00 sec)

Check table metadata

mysql> select * from scalardb.metadata;
+-------------------+------------------------+-----------+-----------+------------------+---------+------------------+
| full_table_name   | column_name            | data_type | key_type  | clustering_order | indexed | ordinal_position |
+-------------------+------------------------+-----------+-----------+------------------+---------+------------------+
| coordinator.state | tx_created_at          | BIGINT    | NULL      | NULL             |       0 |                3 |
| coordinator.state | tx_id                  | TEXT      | PARTITION | NULL             |       0 |                1 |
| coordinator.state | tx_state               | INT       | NULL      | NULL             |       0 |                2 |
| s0.t0             | before_c2              | INT       | NULL      | NULL             |       0 |               13 |
| s0.t0             | before_tx_committed_at | BIGINT    | NULL      | NULL             |       0 |               12 |
| s0.t0             | before_tx_id           | TEXT      | NULL      | NULL             |       0 |                8 |
| s0.t0             | before_tx_prepared_at  | BIGINT    | NULL      | NULL             |       0 |               11 |
| s0.t0             | before_tx_state        | INT       | NULL      | NULL             |       0 |                9 |
| s0.t0             | before_tx_version      | INT       | NULL      | NULL             |       0 |               10 |
| s0.t0             | c1                     | INT       | PARTITION | NULL             |       0 |                1 |
| s0.t0             | c2                     | INT       | NULL      | NULL             |       0 |                2 |
| s0.t0             | tx_committed_at        | BIGINT    | NULL      | NULL             |       0 |                7 |
| s0.t0             | tx_id                  | TEXT      | NULL      | NULL             |       0 |                3 |
| s0.t0             | tx_prepared_at         | BIGINT    | NULL      | NULL             |       0 |                6 |
| s0.t0             | tx_state               | INT       | NULL      | NULL             |       0 |                4 |
| s0.t0             | tx_version             | INT       | NULL      | NULL             |       0 |                5 |
+-------------------+------------------------+-----------+-----------+------------------+---------+------------------+
16 rows in set (0.00 sec)

brfrn169 force-pushed the unify-admin-behaviors branch from e22d2fd to 4c46db7 Compare April 26, 2023 04:30

brfrn169 self-assigned this Apr 26, 2023

brfrn169 added the improvement label Apr 26, 2023

brfrn169 commented Apr 26, 2023

View reviewed changes

brfrn169 changed the title ~~Unify admin behaviors~~ Unify admin behavior Apr 26, 2023

Unify admin behavior

32f4cc4

brfrn169 force-pushed the unify-admin-behaviors branch from 4c46db7 to 32f4cc4 Compare April 26, 2023 04:40

brfrn169 requested review from komamitsu, feeblefakie, Torch3333 and kota2and3kan April 26, 2023 04:41

Torch3333 reviewed Apr 26, 2023

View reviewed changes

brfrn169 added 2 commits April 26, 2023 18:01

Fixes based on the feedback

fcb2338

Fix based on the feedback

fbe7195

komamitsu reviewed Apr 26, 2023

View reviewed changes

Fixes based on the feedback

87166ae

brfrn169 requested review from komamitsu and Torch3333 April 26, 2023 10:29

feeblefakie approved these changes Apr 26, 2023

View reviewed changes

komamitsu approved these changes Apr 27, 2023

View reviewed changes

Torch3333 approved these changes Apr 27, 2023

View reviewed changes

kota2and3kan approved these changes Apr 27, 2023

View reviewed changes

Merge branch 'master' into unify-admin-behaviors

f89270f

brfrn169 merged commit 7c2cb33 into master Apr 27, 2023
12 checks passed

brfrn169 deleted the unify-admin-behaviors branch April 27, 2023 08:53

brfrn169 added this to Done in 3.9.0 Apr 27, 2023

brfrn169 added a commit that referenced this pull request Apr 27, 2023

Unify admin behavior (#856)

489a65f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify admin behavior #856

Unify admin behavior #856

brfrn169 commented Apr 26, 2023 •

edited

brfrn169 Apr 26, 2023

brfrn169 Apr 26, 2023

brfrn169 Apr 26, 2023

brfrn169 Apr 26, 2023

Torch3333 Apr 26, 2023

brfrn169 Apr 26, 2023

brfrn169 Apr 26, 2023

Torch3333 Apr 26, 2023

brfrn169 Apr 26, 2023

Torch3333 Apr 26, 2023

brfrn169 Apr 26, 2023

Torch3333 Apr 26, 2023

brfrn169 Apr 26, 2023

Torch3333 Apr 26, 2023

brfrn169 Apr 26, 2023

Torch3333 Apr 26, 2023

brfrn169 Apr 26, 2023

Torch3333 Apr 26, 2023

Torch3333 Apr 26, 2023

brfrn169 Apr 26, 2023 •

edited

Torch3333 Apr 27, 2023

komamitsu Apr 26, 2023

brfrn169 Apr 26, 2023

komamitsu Apr 26, 2023

brfrn169 Apr 26, 2023

komamitsu Apr 26, 2023

brfrn169 Apr 26, 2023 •

edited

komamitsu Apr 26, 2023

brfrn169 Apr 26, 2023

feeblefakie left a comment

komamitsu left a comment

Torch3333 left a comment

kota2and3kan left a comment

		@@ -14,6 +14,8 @@ public interface DistributedTransactionAdmin extends Admin {
		* Creates coordinator namespace and tables.

	throw new IllegalArgumentException("Table already exists: " + namespace + "." + table);
	throw new IllegalArgumentException("Table already exists: " + ScalarDbUtils.getFullTableName(namespace, table));

Unify admin behavior #856

Unify admin behavior #856

Conversation

brfrn169 commented Apr 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 Apr 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 Apr 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

komamitsu left a comment

Choose a reason for hiding this comment

Torch3333 left a comment

Choose a reason for hiding this comment

kota2and3kan left a comment

Choose a reason for hiding this comment

brfrn169 commented Apr 26, 2023 •

edited

brfrn169 Apr 26, 2023 •

edited

brfrn169 Apr 26, 2023 •

edited