Implementation of Dynamo Admin module #301

thongdk8 · 2021-09-07T07:53:37Z

This PR contains the implementation of the Dynamo Admin module and tests for it.

… dynamo-admin

brfrn169

Thank you! I did the first review for this and left some comments/suggestions. Please check them when you have time!

I will review it again later. Thanks.

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

core/src/main/java/com/scalar/db/storage/dynamo/DynamoTableMetadataManager.java

thongdk8 · 2021-09-08T01:50:05Z

Thank you! I did the first review for this and left some comments/suggestions. Please check them when you have time!

I will review it again later. Thanks.

Thank you. I'll mark this PR as ready when I finish the tests.

… dynamo-admin

brfrn169

Thank you! I took a look at this again and left some comments/suggestions. Please take a look!

Maybe, I think I will take a look at it once again later. Thanks.

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

core/src/main/java/com/scalar/db/storage/dynamo/DynamoTableMetadataManager.java

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

feeblefakie

Thank you!
Let me start off by leaving comments on the superficial parts of the code.
I will look into it later.

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

feeblefakie · 2021-09-10T02:47:52Z

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

+      for (String column : metadata.getClusteringKeyNames()) {
+        if (metadata.getColumnDataType(column) == DataType.BOOLEAN) {
+          throw new ExecutionException(
+              "BOOLEAN type is not supported for a clustering key or a secondary index in DynamoDB");


This limitation is still there? I did a quick search but I can't find it.

It still is a limitation. There is no attribute type likes ScalarAttributeType.BOOL came from SDK.

feeblefakie · 2021-09-10T02:52:33Z

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

-    throw new UnsupportedOperationException("implement later");
+  public void createNamespace(String namespace, Map<String, String> options) {
+    // In Dynamo DB storage, namespace will be added to table name as prefix along with dot
+    // separator.
  }

  @Override
  public void createTable(


I think this method is too big.
The code seems to have several logical units (for each component to build), so can you make them turn to methods and call them to make this method slim?

@thongdk8 Somehow it is resolved without checked but was it done (doesn't look like it though) or was it difficult/giving much benefit so not done or it was unintentionally resolved by another change?

Thank you. I did split the method to make it slimmer in 166ec54.
This was marked as resolved because I did apply it, I mentioned here. All the suggestions that were marked as resolved were applied.

I meant, please reply back to all the comments respectively. Otherwise, it's pretty hard to identify since there are many comments.

I see. I'll do it from now on. Sorry about that.

brfrn169

Thank you! Just left some comments. Please take a look when you have time!

I found one different behavior from the original schema loader. Just in case, can you please re-check if there are any other different behaviors in this new implementation? And if you find them, please correct them. Thank you.

core/src/main/java/com/scalar/db/storage/dynamo/DynamoTableMetadataManager.java

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

… dynamo-admin

brfrn169

Thank you! I left one question and some minor suggestions. Please check them when you have time!

BTW, did you re-check if there are any other different behaviors from the original schema loader in this new implementation? And was it good?

core/src/main/java/com/scalar/db/storage/dynamo/DynamoTableMetadataManager.java

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

thongdk8 · 2021-09-15T09:10:27Z

BTW, did you re-check if there are any other different behaviors from the original schema loader in this new implementation? And was it good?

Thank you. I did check the behaviors, they are fine.

brfrn169

LGTM except what I commented. Thank you!

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

Co-authored-by: Toshihiro Suzuki <[email protected]>

brfrn169

LGTM! Thank you!

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

feeblefakie

Left additional comments and suggestions.

core/src/main/java/com/scalar/db/storage/dynamo/DynamoTableMetadataManager.java

feeblefakie · 2021-09-16T05:44:15Z

core/src/main/java/com/scalar/db/storage/dynamo/DynamoTableMetadataManager.java

+      throw new StorageRuntimeException("creating meta data table failed");
+    }
+
+    while (true) {


This block is redundant with the block in Admin.
(CREATING_WAITING_TIME is also redundantly defined)
It would be great if the redundancy is removed.

Yes, indeed. I created a static function for waiting table creation. Applied in 391f30b

feeblefakie · 2021-09-16T05:50:06Z

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

+  private static final String PARTITION_KEY = "concatenatedPartitionKey";
+  private static final String CLUSTERING_KEY = "concatenatedClusteringKey";
+  private static final String GLOBAL_INDEX_NAME_PREFIX = "global_index";
+  private static final int CREATING_WAITING_TIME = 3000;


The variable names are not consistent.
It is is ms then should it end with _MS or something to be consistent with _SEC?

I would suggest to have the same time unit and adjust for each case like below.

CREATE_WAIT_DURATION_MILLIS COOL_DOWN_DURATION_MILLIS

Cause the cooldown time is accepted in seconds so I change variables' name with postfix _SECS. Apply in 391f30b

… dynamo-admin

brfrn169

@thongdk8 Sorry, I missed one thing. Just left some suggestions. Please check them!

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

brfrn169

LGTM! Thank you for your patience.

feeblefakie · 2021-09-16T09:10:42Z

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

+        .build();
+  }
+
+  protected static void waitForTableCreation(DynamoDbClient client, String tableFullName) {


Can it be package private instead of protected?
(Any reason for protected?)

It was used in DynamoTableMetadataManager when creating the metadata table so I made it protected.

So, it can be package-private, right?
I've checked the code and DynamoTableMetadataManager in the same package is the only one that uses this method except for this Admin class, so please make it package-private.
Restricted scope is usually better.

Thank you. I removed the protected identifier 90037fd

feeblefakie

Looking good except for the comments I additionally made.
Thank you!

feeblefakie · 2021-09-16T11:22:31Z

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

+        .build();
+  }
+
+  protected static void waitForTableCreation(DynamoDbClient client, String tableFullName) {


So, it can be package-private, right?
I've checked the code and DynamoTableMetadataManager in the same package is the only one that uses this method except for this Admin class, so please make it package-private.
Restricted scope is usually better.

feeblefakie · 2021-09-16T11:33:24Z

core/src/main/java/com/scalar/db/storage/dynamo/DynamoTableMetadataManager.java

@@ -1,22 +1,44 @@
 package com.scalar.db.storage.dynamo;

+import static com.scalar.db.storage.dynamo.DynamoAdmin.waitForTableCreation;


Let's not use static import for this.

Updated in 90037fd

feeblefakie · 2021-09-16T11:33:48Z

core/src/main/java/com/scalar/db/storage/dynamo/DynamoTableMetadataManager.java

@@ -25,11 +47,14 @@
 */


It's basically rewritten, so please put your name on it!

Updated in 90037fd

feeblefakie

LGTM! Thank you!

thongdk8 added 6 commits September 2, 2021 17:47

create table with auto scaling enable

7c1ee41

continuos backup controling

f959e35

implement admin functions, add unit tests

b9d7922

format

a7cd656

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

c4a15fc

… dynamo-admin

correct full table name

e32e6b5

thongdk8 requested review from brfrn169 and feeblefakie September 7, 2021 07:54

thongdk8 self-assigned this Sep 7, 2021

thongdk8 added the enhancement New feature or request label Sep 7, 2021

brfrn169 reviewed Sep 8, 2021

View reviewed changes

correct table creation request, add integration test

2f5e335

thongdk8 requested a review from brfrn169 September 8, 2021 09:40

thongdk8 changed the title ~~[WIP] Implementation of Dynamo Admin module~~ Implementation of Dynamo Admin module Sep 8, 2021

thongdk8 marked this pull request as ready for review September 8, 2021 09:40

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

1513a9a

… dynamo-admin

brfrn169 reviewed Sep 9, 2021

View reviewed changes

thongdk8 added 3 commits September 9, 2021 15:54

revise

83b7020

correct delete request

3de4141

fix test

aa8701e

feeblefakie reviewed Sep 10, 2021

View reviewed changes

revise

166ec54

thongdk8 requested review from brfrn169 and feeblefakie September 10, 2021 09:12

brfrn169 reviewed Sep 14, 2021

View reviewed changes

core/src/main/java/com/scalar/db/storage/dynamo/DynamoTableMetadataManager.java Outdated Show resolved Hide resolved

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java Show resolved Hide resolved

thongdk8 added 3 commits September 15, 2021 14:17

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

20c29e8

… dynamo-admin

disable auto scaling before drop

4af745c

remove redundant exeption

fc56815

brfrn169 reviewed Sep 15, 2021

View reviewed changes

revise

f82e714

brfrn169 reviewed Sep 15, 2021

View reviewed changes

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java Outdated Show resolved Hide resolved

Update core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java

88af4c3

Co-authored-by: Toshihiro Suzuki <[email protected]>

brfrn169 previously approved these changes Sep 16, 2021

View reviewed changes

feeblefakie reviewed Sep 16, 2021

View reviewed changes

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java Show resolved Hide resolved

feeblefakie reviewed Sep 16, 2021

View reviewed changes

thongdk8 added 2 commits September 16, 2021 15:19

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

3471196

… dynamo-admin

revise

391f30b

thongdk8 dismissed brfrn169’s stale review via 391f30b September 16, 2021 06:37

format

355b9b2

thongdk8 requested a review from feeblefakie September 16, 2021 06:48

brfrn169 reviewed Sep 16, 2021

View reviewed changes

revise

19938a0

brfrn169 reviewed Sep 16, 2021

View reviewed changes

core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java Outdated Show resolved Hide resolved

rename ru

1d82dc8

brfrn169 previously approved these changes Sep 16, 2021

View reviewed changes

feeblefakie reviewed Sep 16, 2021

View reviewed changes

revise

90037fd

thongdk8 dismissed brfrn169’s stale review via 90037fd September 16, 2021 12:30

feeblefakie approved these changes Sep 16, 2021

View reviewed changes

feeblefakie merged commit c9a7290 into master Sep 16, 2021

feeblefakie deleted the dynamo-admin branch September 16, 2021 12:43

brfrn169 pushed a commit that referenced this pull request Sep 17, 2021

Implementation of Dynamo Admin module (#301)

3df4aa2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of Dynamo Admin module #301

Implementation of Dynamo Admin module #301

thongdk8 commented Sep 7, 2021

brfrn169 left a comment

thongdk8 commented Sep 8, 2021

brfrn169 left a comment •

edited

Loading

feeblefakie left a comment

feeblefakie Sep 10, 2021

thongdk8 Sep 10, 2021 •

edited

Loading

feeblefakie Sep 10, 2021

feeblefakie Sep 16, 2021 •

edited

Loading

thongdk8 Sep 16, 2021

feeblefakie Sep 16, 2021 •

edited

Loading

thongdk8 Sep 16, 2021

brfrn169 left a comment •

edited

Loading

brfrn169 left a comment •

edited

Loading

thongdk8 commented Sep 15, 2021

brfrn169 left a comment

brfrn169 left a comment

feeblefakie left a comment

feeblefakie Sep 16, 2021

thongdk8 Sep 16, 2021

feeblefakie Sep 16, 2021

thongdk8 Sep 16, 2021

brfrn169 left a comment

brfrn169 left a comment

feeblefakie Sep 16, 2021

thongdk8 Sep 16, 2021 •

edited

Loading

feeblefakie Sep 16, 2021 •

edited

Loading

thongdk8 Sep 16, 2021

feeblefakie left a comment

feeblefakie Sep 16, 2021 •

edited

Loading

feeblefakie Sep 16, 2021

thongdk8 Sep 16, 2021

feeblefakie Sep 16, 2021

thongdk8 Sep 16, 2021

feeblefakie left a comment

		@@ -1,22 +1,44 @@
		package com.scalar.db.storage.dynamo;

		import static com.scalar.db.storage.dynamo.DynamoAdmin.waitForTableCreation;

Implementation of Dynamo Admin module #301

Implementation of Dynamo Admin module #301

Conversation

thongdk8 commented Sep 7, 2021

brfrn169 left a comment

Choose a reason for hiding this comment

thongdk8 commented Sep 8, 2021

brfrn169 left a comment • edited Loading

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thongdk8 Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feeblefakie Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feeblefakie Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 left a comment • edited Loading

Choose a reason for hiding this comment

brfrn169 left a comment • edited Loading

Choose a reason for hiding this comment

thongdk8 commented Sep 15, 2021

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

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 left a comment

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thongdk8 Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

feeblefakie Sep 16, 2021 • edited Loading

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

feeblefakie Sep 16, 2021 • edited Loading

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

feeblefakie left a comment

Choose a reason for hiding this comment

brfrn169 left a comment •

edited

Loading

thongdk8 Sep 10, 2021 •

edited

Loading

feeblefakie Sep 16, 2021 •

edited

Loading

feeblefakie Sep 16, 2021 •

edited

Loading

brfrn169 left a comment •

edited

Loading

brfrn169 left a comment •

edited

Loading

thongdk8 Sep 16, 2021 •

edited

Loading

feeblefakie Sep 16, 2021 •

edited

Loading

feeblefakie Sep 16, 2021 •

edited

Loading