Allow tags to be added when lease table is created #1065

RyanFrench · 2023-03-16T13:07:41Z

Issue #, if available: #154

Description of changes: Add capability to add tags when creating the lease table

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

stair-aws

Thanks for the contribution! PR mostly LGTM, and verified mvn clean test against a rebase. A few small requested changes, and question.

stair-aws · 2023-03-22T14:35:25Z

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

@@ -190,6 +194,8 @@ public class LeaseManagementConfig {

    private BillingMode billingMode = BillingMode.PAY_PER_REQUEST;

+    private Collection<Tag> tags = DefaultSdkAutoConstructList.getInstance();


FMI: why DefaultSdkAutoConstructList (DSACL) instead of Collections.emptyList() (CeL)? Since DSACL is backed by a CeL, what is the benefit?

Would you kindly add Javadoc to this new member variable?

I used DSACL as that was the default value associated with the tags in the CreateTable request. I'm not overly familiar with the java SDK, or java in general, but I figured if no tags were specified then we would be essentially setting the value to in the CreateTable to the default value already there, and would avoid complicated branching scenarios.

Happy to change to a CeL instead if preferred.

Thanks for the context! Given the SDK dive into TagListCopier, DSACL seems the better option if the SDK sniffs for the SdkAutoConstructList marker interface in other places.

stair-aws · 2023-03-22T14:39:10Z

...nt/src/main/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseManagementFactory.java

+     * @param tags
+     */
+    @Deprecated
+    public DynamoDBLeaseManagementFactory(final KinesisAsyncClient kinesisClient, final String streamName,


N.B. These large parameters lists are an anti-pattern. @see http://principles-wiki.net/anti-patterns:long_parameter_list

We (AWS) should hold ourselves to higher standards and refactor this mess (which is, unfortunately, systemic in KCL).

No change necessary by PR author.

...nt/src/main/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseManagementFactory.java

stair-aws · 2023-03-22T14:54:00Z

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

        } else {
            request = CreateTableRequest.builder().tableName(table).keySchema(serializer.getKeySchema())
                    .attributeDefinitions(serializer.getAttributeDefinitions()).provisionedThroughput(throughput)
+                    .tags(tags)


FYI: validated TagListCopier protects against nulls:

static List<Tag> copy(Collection<? extends Tag> tagListParam) { List<Tag> list; if (tagListParam == null || tagListParam instanceof SdkAutoConstructList) { list = DefaultSdkAutoConstructList.getInstance(); } else { List<Tag> modifiableList = new ArrayList<>(); tagListParam.forEach(entry -> { modifiableList.add(entry); }); list = Collections.unmodifiableList(modifiableList); } return list;

(This might also answer my question w.r.t. DSACL.)

stair-aws · 2023-03-22T15:09:18Z

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

@@ -168,10 +192,13 @@ public boolean createLeaseTableIfNotExists(@NonNull final Long readCapacity, @No
        if(BillingMode.PAY_PER_REQUEST.equals(billingMode)){
            request = CreateTableRequest.builder().tableName(table).keySchema(serializer.getKeySchema())
                    .attributeDefinitions(serializer.getAttributeDefinitions())
-                    .billingMode(billingMode).build();
+                    .billingMode(billingMode)
+                    .tags(tags)


Don't repeat yourself (i.e., here, line ~201, line ~217).

A suggested refactor is to create a method that provides the "base" builder. Example:

private CreateTableRequest.Builder createTableRequestBuilder() { final CreateTableRequest.Builder builder = CTR.builder() .tableName(...) .tags(tags) ... /* and other shared elements */; if (BillingMode.PAY_PER_REQUEST...) { builder.billingMode(billingMode); } return builder; }

Then this method simplifies to something like:

final CTR.B builder = createTRQB(); if (!BillingMode.PAY_PER_REQUEST...) { ProvisionedThroughput throughput = ... builder.provisionedThroughput(throughput); } return createTableIfNotExists(builder.build());

stair-aws · 2023-03-22T15:16:34Z

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

@@ -313,6 +317,40 @@ public void testCreateLeaseTableProvisionedBillingModeIfNotExists() throws Excep
        Assert.assertTrue(result);
    }

+    @Test
+    public void testCreateLeaseTableWithTagsIfNotExists() throws Exception {
+        tags = Arrays.asList(Tag.builder().key("foo").value("bar").build());


IDE warning: s/Arrays.asList/Collections.singletonList/

stair-aws · 2023-03-22T15:18:56Z

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

+                tableCreatorCallback, LeaseManagementConfig.DEFAULT_REQUEST_TIMEOUT, BillingMode.PROVISIONED, tags);
+
+        when(dynamoDbClient.describeTable(describeTableRequest)).thenReturn(mockDescribeTableFuture);
+        when(mockDescribeTableFuture.get(eq(LeaseManagementConfig.DEFAULT_REQUEST_TIMEOUT.toMillis()), eq(TimeUnit.MILLISECONDS)))


When all parameters are "raw values", eq is unnecessary and can/should be removed.

edit: and lines ~340, 348~350

stair-aws · 2023-03-22T15:21:50Z

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

+                .tags(tags)
+                .build();
+        when(dynamoDbClient.createTable(createTableRequest)).thenReturn(mockCreateTableFuture);
+        when(mockCreateTableFuture.get(eq(LeaseManagementConfig.DEFAULT_REQUEST_TIMEOUT.toMillis()), eq(TimeUnit.MILLISECONDS)))


thenReturn(null) is the default behavior for Mockito mock objects. This statement is unnecessary and can be removed.

I've just re-run the tests with this removed and it's now throwing exceptions around incomplete mocking. Adding it back in fixes the test.

Strange. "Works on my machine" w/o this line (and I'm sync'd to 6783e1e+rebase). It's an insignificant concern, so let's leave as-is for this PR.

stair-aws · 2023-03-22T15:26:28Z

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

+
+        final boolean result = leaseRefresher.createLeaseTableIfNotExists(10L, 10L);
+
+        verify(dynamoDbClient, times(1)).describeTable(describeTableRequest);


FYI: times(1) is the default VerificationMode and is therefore redundant/unnecessary.

This verification block is duplicated several times in this class. Unless you choose to take ownership, that is something we (AWS) can deduplicate in a future commit.

RyanFrench · 2023-03-22T16:34:30Z

Thanks for taking a look and the guidance! I'm not a java dev by any means, so appreciate there's quite a bit in there I didn't know.

I believe I've addressed all of the changes requested and added my reasoning about using DSACL above.

stair-aws

LGTM (w/ one small request)!

To double-check: did you execute this in an AWS account and validate the DDB tags appear and will satisfy your ask in #154 ?

stair-aws · 2023-03-22T18:08:43Z

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

@@ -787,6 +802,16 @@ protected DependencyException convertAndRethrowExceptions(String operation, Stri
        }
    }

+    private CreateTableRequest.Builder createTableRequestBuilder() {
+    final CreateTableRequest.Builder builder = CreateTableRequest.builder().tableName(table).keySchema(serializer.getKeySchema())


The indentation for code in this method is incorrect. (Looks like most lines should be indented +1 level?)

Would you kindly fix so the indentation matches the convention of other methods?

(FWIW, I normally regard whitespace as a nitpick. However, seeing } at position 1 is jarring when it isn't end-of-class.)

stair-aws

PR LGTM. I'll take this for a "test-drive" to validate before merging. Thanks for taking ownership and contributing this PR!

RyanFrench · 2023-03-22T19:20:39Z

I did execute this in an AWS account and could see tags being added to the table correctly. I also tested to make sure that creating without tags still worked. I can add some integration tests for this the same as the others in the project, however, I seem to be hitting intermittent failures when running them from PeriodicShardSyncManagerTest.testFor1000DifferentValidReshardHierarchyTreeWithSomeInProgressParentsTheHashRangesAreAlwaysComplete

stair-aws

@RyanFrench Would you kindly apply the following patch to your branch:

diff --git a/amazon-kinesis-client/src/main/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseManagementFactory.java b/amazon-kinesis-client/src/main/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseManagementFactory.java
index b06561a..8b60f6d 100644
--- a/amazon-kinesis-client/src/main/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseManagementFactory.java
+++ b/amazon-kinesis-client/src/main/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseManagementFactory.java
@@ -648,7 +648,7 @@ public class DynamoDBLeaseManagementFactory implements LeaseManagementFactory {
     @Override
     public DynamoDBLeaseRefresher createLeaseRefresher() {
         return new DynamoDBLeaseRefresher(tableName, dynamoDBClient, leaseSerializer, consistentReads,
-                tableCreatorCallback, dynamoDbRequestTimeout, billingMode);
+                tableCreatorCallback, dynamoDbRequestTimeout, billingMode, tags);
     }
 
     @Override

With this patch, I verified that a virgin KCL application created the DDB lease table w/ the provided tags.

Nice work!

RyanFrench · 2023-03-23T17:22:45Z

Done, and thank you for verifying and the help getting this over the line!

Allow tags to be added when lease table is created

8ee80f9

stair-aws added the v2.x Issues related to the 2.x version label Mar 20, 2023

stair-aws suggested changes Mar 22, 2023

View reviewed changes

stair-aws added the enhancement label Mar 22, 2023

stair-aws self-assigned this Mar 22, 2023

RyanFrench added 4 commits March 22, 2023 16:15

Add javadoc comment to new member variable

cca20eb

DRY up creation of the CreateTable Request builder

890a5c8

Fix compiler error

a769516

Remove unnecessary eq functions

6783e1e

RyanFrench force-pushed the master branch from 5928170 to 6783e1e Compare March 22, 2023 16:53

stair-aws suggested changes Mar 22, 2023

View reviewed changes

Fix indentation

d50cf00

stair-aws approved these changes Mar 22, 2023

View reviewed changes

stair-aws suggested changes Mar 23, 2023

View reviewed changes

Add patch

3fe2c50

stair-aws approved these changes Mar 23, 2023

View reviewed changes

stair-aws merged commit 2ecd1c4 into awslabs:master Mar 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow tags to be added when lease table is created #1065

Allow tags to be added when lease table is created #1065

RyanFrench commented Mar 16, 2023

stair-aws left a comment

stair-aws Mar 22, 2023

RyanFrench Mar 22, 2023

stair-aws Mar 22, 2023

stair-aws Mar 22, 2023

stair-aws Mar 22, 2023

stair-aws Mar 22, 2023

stair-aws Mar 22, 2023

stair-aws Mar 22, 2023

stair-aws Mar 22, 2023

RyanFrench Mar 22, 2023

stair-aws Mar 22, 2023

stair-aws Mar 22, 2023

RyanFrench commented Mar 22, 2023

stair-aws left a comment

stair-aws Mar 22, 2023

RyanFrench Mar 22, 2023

stair-aws left a comment

RyanFrench commented Mar 22, 2023

stair-aws left a comment

RyanFrench commented Mar 23, 2023

		@@ -190,6 +194,8 @@ public class LeaseManagementConfig {

		private BillingMode billingMode = BillingMode.PAY_PER_REQUEST;

		private Collection<Tag> tags = DefaultSdkAutoConstructList.getInstance();


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

		verify(dynamoDbClient, times(1)).describeTable(describeTableRequest);

Allow tags to be added when lease table is created #1065

Allow tags to be added when lease table is created #1065

Conversation

RyanFrench commented Mar 16, 2023

stair-aws 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

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

RyanFrench commented Mar 22, 2023

stair-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stair-aws left a comment

Choose a reason for hiding this comment

RyanFrench commented Mar 22, 2023

stair-aws left a comment

Choose a reason for hiding this comment

RyanFrench commented Mar 23, 2023