Create DynamoDB tables on On-Demand billing mode by default. #854

shanmsac · 2021-08-27T22:13:36Z

This will enable KCL to create the DynamoDB tables - Lease and ShardProgress with the On-Demand billing mode instead of provisioned billing mode.
Updated the LeaseRefresher interface function createLeaseTableIfNotExists() to remove the provision mode parameters.
Removed the integration test "DynamoDBLeaseRenewerBillingModePayPerRequestIntegrationTest.java" as PayPerRequest mode is tested by default integration test.

Signed-off-by: Sachin Sundar P S shanmsac@amazon.com

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

This will enable KCL to create the DynamoDB tables - Lease and ShardProgress with the On-Demand billing mode instead of provisioned billing mode.

joshua-kim · 2021-08-30T19:02:41Z

amazon-kinesis-client/src/main/java/software/amazon/kinesis/leases/LeaseRefresher.java

     * @return true if we created a new table (table didn't exist before)
     * 
     * @throws ProvisionedThroughputException if we cannot create the lease table due to per-AWS-account capacity
     *         restrictions.
     * @throws DependencyException if DynamoDB createTable fails in an unexpected way
     */
-    boolean createLeaseTableIfNotExists(Long readCapacity, Long writeCapacity)


This is a public interface customers might be using; let's not mess with these signatures. We can make createLeaseTableIfNotExists() a separate signature to preserve backwards-compatible behavior.

ya, kept this and marked as deprecated.

joshua-kim · 2021-08-30T19:13:40Z

...c/test/java/software/amazon/kinesis/leases/LeaseIntegrationBillingModePayPerRequestTest.java

-import software.amazon.kinesis.leases.dynamodb.TableCreatorCallback;
-
-@Slf4j
-public class LeaseIntegrationBillingModePayPerRequestTest extends LeaseIntegrationTest {


Lets keep the existing integ tests

since keeping the old function signature, will keep the integ tests

joshua-kim

Left one comment to not remove public interface method

shanmsac · 2021-08-31T21:15:57Z

Updated as per comments

joshua-kim · 2021-09-09T18:00:57Z

...sis-client/src/main/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseRefresher.java

@@ -162,6 +161,16 @@ public DynamoDBLeaseRefresher(final String table, final DynamoDbAsyncClient dyna
    @Override
    public boolean createLeaseTableIfNotExists(@NonNull final Long readCapacity, @NonNull final Long writeCapacity)
            throws ProvisionedThroughputException, DependencyException {
+        // DynamoDB is now created in PayPerRequest billing mode by default. Keeping this for backward compatibility.
+        return createLeaseTableIfNotExists();


Let's not change the old code path - this will change the behavior for any customer classes that use this.

addressed this.

joshua-kim · 2021-09-09T18:01:10Z

...sis-client/src/test/java/software/amazon/kinesis/leases/ExceptionThrowingLeaseRefresher.java

@@ -110,11 +110,17 @@ private void throwExceptions(String methodName, ExceptionThrowingLeaseRefresherM

    @Override
    public boolean createLeaseTableIfNotExists(Long readCapacity, Long writeCapacity)
+            throws ProvisionedThroughputException, DependencyException {
+        return createLeaseTableIfNotExists();


See earlier comment

joshua-kim · 2021-09-09T18:02:59Z

How did we test this? Can we add unit tests?

shanmsac · 2021-09-30T05:01:01Z

added unit tests for both table creation methods

joshua-kim · 2021-09-30T22:20:55Z

amazon-kinesis-client/src/main/java/software/amazon/kinesis/leases/LeaseRefresher.java

@@ -29,17 +29,31 @@

    /**
     * Creates the table that will store leases. Succeeds if table already exists.
-     * 
+     * Deprecated. Use createLeaseTableIfNotExists().


You can use @link or javadoc annotation here to point people over to the new method, it makes it easier to find the correct method since it indexes in ides + links in documentation generators

Updated to @link.

joshua-kim · 2021-10-01T04:32:12Z

...s-client/src/main/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseCoordinator.java

@@ -211,10 +211,9 @@ public void run() {
    @Override
    public void initialize() throws ProvisionedThroughputException, DependencyException, IllegalStateException {
        final boolean newTableCreated =
-                leaseRefresher.createLeaseTableIfNotExists(initialLeaseTableReadCapacity, initialLeaseTableWriteCapacity);


There should be a unit test in DynamoDBLeaseCoordinatorTest to make sure that LeaseRefresher#createLeaseIfNotExists() gets called in DynamoDBLeaseCoordinator#initialize() as part of validating this change.

There is no DynamoDBLeaseCoordinatorTest or any unit test for this class. Added unit test for initialize() to test this.

joshua-kim · 2021-10-01T04:47:47Z

...sis-client/src/main/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseRefresher.java

-            //
-            log.error("Failed to get table status for {}", table, de);
-        }
+        // DynamoDB is now created in PayPerRequest billing mode by default. Keeping this for backward compatibility.


Leaving this up to your preference, but I personally don't think this comment is needed since it's mentioned the interface's javadoc.

Ya, updated interface's javadoc to include this.

joshua-kim · 2021-10-01T04:52:29Z

...client/src/test/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseRefresherTest.java

+                tableCreatorCallback, LeaseManagementConfig.DEFAULT_REQUEST_TIMEOUT, BillingMode.PROVISIONED);
+
+        when(dynamoDbClient.describeTable(any(DescribeTableRequest.class))).thenReturn(mockDescribeTableFuture);
+        when(mockDescribeTableFuture.get(anyLong(), any()))


Why any() here? Can we use the specific class we're expecting (if it's not possible to use the exact value, applicable in other spots in this PR as well).

Ah, these came from the previous tests. Updated to use actual class objects.

joshua-kim · 2021-10-01T04:54:13Z

...client/src/test/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseRefresherTest.java

+        leaseRefresher = new DynamoDBLeaseRefresher(TABLE_NAME, dynamoDbClient, leaseSerializer, CONSISTENT_READS,
+                tableCreatorCallback, LeaseManagementConfig.DEFAULT_REQUEST_TIMEOUT, BillingMode.PROVISIONED);
+
+        when(dynamoDbClient.describeTable(any(DescribeTableRequest.class))).thenReturn(mockDescribeTableFuture);


I don't think we need to use any(DescribeTableRequest.class) since the table name is constant across tests, we can mock against a concrete object like DescribeTableRequest.builder().tableName(TABLE_NAME).build().

yes, updated this as part of removing the any() where ever possible

joshua-kim · 2021-10-01T04:58:06Z

...sis-client/src/main/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseRefresher.java

+            if (tableStatus() != null) {
+                return newTableCreated;
+            }
+        } catch (DependencyException de) {


I know this is the existing behavior but I'm wondering why we don't throw this exception here. Seems like it makes more sense to fail here if we're not able to pull the lease table's status and let the caller decide how to proceed.

I wondered the same while rewriting this function why the exception was consumed. I think it was done as CreateTable call will fail with ResourceInUseException and return false(table already exists) or create the table, so finally its the same behavior for caller. So, I'm keeping this behavior for now.

joshua-kim · 2021-10-01T05:01:05Z

...client/src/test/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseRefresherTest.java

+        when(mockDescribeTableFuture.get(anyLong(), any()))
+                .thenThrow(ResourceNotFoundException.builder().message("Table doesn't exist").build());
+
+        when(dynamoDbClient.createTable(any(CreateTableRequest.class))).thenReturn(mockCreateTableFuture);


Same with earlier comment

joshua-kim · 2021-10-01T05:05:00Z

...client/src/test/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseRefresherTest.java

+        when(mockCreateTableFuture.get(anyLong(), any())).thenReturn(null);
+
+        final boolean result = leaseRefresher.createLeaseTableIfNotExists(10L, 10L);
+


Can we verify interactions w/ the ddb client?

Added, also for other dependencies.

joshua-kim · 2021-10-01T05:17:49Z

...client/src/test/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseRefresherTest.java

+
+        Assert.assertTrue(result);
+    }
+


We've only tested the cases where the table doesn't exist (i.e ddbClient.describeTable() throws a ResourceNotFoundException. Let's add a few other edge cases which can happen (I noticed a few exceptions are thrown in the bootstrap path):

DescribeTable

If we throw a DependencyException, we should either continue trying to create the table (or throw the exception if we decide to propagate it up as per my earlier comment. I don't have a strong opinion on this since it's existing behavior, so feel free to take either approach).

CreateTable

If we throw a ResourceNotFoundException, we should expect to return true.

If we throw a LimitExceededException, we should propagate up a ProvisionedThroughputException.

If we throw a DynamoDBException we should throw a DependencyException.

If we throw a TimeoutException we should throw a DependencyException.

As with the earlier comment, we should also be verifying interactions between dependencies.

For DescribeTable, I have kept the existing behavior and added a tests for all above.

joshua-kim

Left a few comments on the unit tests

joshua-kim

LGTM. Thanks for the PR!

Create DynamoDB tables on On-Demand billing mode by default.

5834156

This will enable KCL to create the DynamoDB tables - Lease and ShardProgress with the On-Demand billing mode instead of provisioned billing mode.

joshua-kim reviewed Aug 30, 2021

View reviewed changes

joshua-kim suggested changes Aug 30, 2021

View reviewed changes

Keep previous table creation function for backward compatibility.

07c845b

shanmsac requested a review from joshua-kim August 31, 2021 21:10

joshua-kim reviewed Sep 9, 2021

View reviewed changes

Added unit tests.

05a678f

joshua-kim reviewed Sep 30, 2021

View reviewed changes

joshua-kim reviewed Oct 1, 2021

View reviewed changes

joshua-kim suggested changes Oct 1, 2021

View reviewed changes

Added more unit tests.

b27544e

shanmsac requested a review from joshua-kim October 6, 2021 22:52

joshua-kim approved these changes Oct 6, 2021

View reviewed changes

joshua-kim merged commit e81969a into awslabs:master Oct 6, 2021

shanmsac deleted the ddb-on-demand branch October 6, 2021 23:06

shanmsac added this to the v2.3.7 milestone Oct 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create DynamoDB tables on On-Demand billing mode by default. #854

Create DynamoDB tables on On-Demand billing mode by default. #854

shanmsac commented Aug 27, 2021

joshua-kim Aug 30, 2021

shanmsac Aug 31, 2021

joshua-kim Aug 30, 2021 •

edited

shanmsac Aug 31, 2021

joshua-kim left a comment

shanmsac commented Aug 31, 2021

joshua-kim Sep 9, 2021

shanmsac Sep 30, 2021

joshua-kim Sep 9, 2021

shanmsac Sep 30, 2021

joshua-kim commented Sep 9, 2021

shanmsac commented Sep 30, 2021

joshua-kim Sep 30, 2021 •

edited

shanmsac Oct 6, 2021

joshua-kim Oct 1, 2021 •

edited

shanmsac Oct 6, 2021

joshua-kim Oct 1, 2021

shanmsac Oct 6, 2021

joshua-kim Oct 1, 2021 •

edited

shanmsac Oct 6, 2021

joshua-kim Oct 1, 2021

shanmsac Oct 6, 2021

joshua-kim Oct 1, 2021

shanmsac Oct 6, 2021

joshua-kim Oct 1, 2021

shanmsac Oct 6, 2021

joshua-kim Oct 1, 2021 •

edited

shanmsac Oct 6, 2021

joshua-kim Oct 1, 2021 •

edited

shanmsac Oct 6, 2021

joshua-kim left a comment

joshua-kim left a comment

		when(mockCreateTableFuture.get(anyLong(), any())).thenReturn(null);

		final boolean result = leaseRefresher.createLeaseTableIfNotExists(10L, 10L);

Create DynamoDB tables on On-Demand billing mode by default. #854

Create DynamoDB tables on On-Demand billing mode by default. #854

Conversation

shanmsac commented Aug 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshua-kim Aug 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshua-kim left a comment

Choose a reason for hiding this comment

shanmsac commented Aug 31, 2021

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

joshua-kim commented Sep 9, 2021

shanmsac commented Sep 30, 2021

joshua-kim Sep 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshua-kim Oct 1, 2021 • 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

joshua-kim Oct 1, 2021 • 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

joshua-kim Oct 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshua-kim Oct 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshua-kim left a comment

Choose a reason for hiding this comment

joshua-kim left a comment

Choose a reason for hiding this comment

joshua-kim Aug 30, 2021 •

edited

joshua-kim Sep 30, 2021 •

edited

joshua-kim Oct 1, 2021 •

edited

joshua-kim Oct 1, 2021 •

edited

joshua-kim Oct 1, 2021 •

edited

joshua-kim Oct 1, 2021 •

edited