Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 4 commits into from
Oct 6, 2021
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5834156
Create DynamoDB tables on On-Demand billing mode by default.
shanmsac Aug 27, 2021
07c845b
Keep previous table creation function for backward compatibility.
shanmsac Aug 31, 2021
05a678f
Added unit tests.
shanmsac Sep 30, 2021
b27544e
Added more unit tests.
shanmsac Oct 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...on-kinesis-client/src/main/java/software/amazon/kinesis/leases/LeaseManagementConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ public class LeaseManagementConfig {

private Duration dynamoDbRequestTimeout = DEFAULT_REQUEST_TIMEOUT;

private BillingMode billingMode = BillingMode.PROVISIONED;
private BillingMode billingMode = BillingMode.PAY_PER_REQUEST;

/**
* Frequency (in millis) of the auditor job to scan for partial leases in the lease table.
Expand Down
18 changes: 16 additions & 2 deletions amazon-kinesis-client/src/main/java/software/amazon/kinesis/leases/LeaseRefresher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,31 @@ public interface LeaseRefresher {

/**
* Creates the table that will store leases. Succeeds if table already exists.
*
* Deprecated. Use createLeaseTableIfNotExists().
Copy link
Contributor

@joshua-kim joshua-kim Sep 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to @link.

*
* @param readCapacity
* @param writeCapacity
*
* @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
*/
@Deprecated
boolean createLeaseTableIfNotExists(Long readCapacity, Long writeCapacity)
throws ProvisionedThroughputException, DependencyException;

/**
* Creates the table that will store leases. Succeeds if table already exists.
*
* @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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya, kept this and marked as deprecated.

boolean createLeaseTableIfNotExists()
throws ProvisionedThroughputException, DependencyException;

/**
Expand Down
5 changes: 2 additions & 3 deletions ...lient/src/main/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseCoordinator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,10 +211,9 @@ public void run() {
@Override
public void initialize() throws ProvisionedThroughputException, DependencyException, IllegalStateException {
final boolean newTableCreated =
leaseRefresher.createLeaseTableIfNotExists(initialLeaseTableReadCapacity, initialLeaseTableWriteCapacity);
Copy link
Contributor

@joshua-kim joshua-kim Oct 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

leaseRefresher.createLeaseTableIfNotExists();
if (newTableCreated) {
log.info("Created new lease table for coordinator with initial read capacity of {} and write capacity of {}.",
initialLeaseTableReadCapacity, initialLeaseTableWriteCapacity);
log.info("Created new lease table for coordinator with pay per request billing mode.");
}
// Need to wait for table in active state.
final long secondsBetweenPolls = 10L;
Expand Down
2 changes: 1 addition & 1 deletion ...src/main/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseManagementFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ public DynamoDBLeaseManagementFactory(final KinesisAsyncClient kinesisClient, fi
ignoreUnexpectedChildShards, shardSyncIntervalMillis, consistentReads, listShardsBackoffTimeMillis,
maxListShardsRetryAttempts, maxCacheMissesBeforeReload, listShardsCacheAllowedAgeInSeconds,
cacheMissWarningModulus, initialLeaseTableReadCapacity, initialLeaseTableWriteCapacity,
hierarchicalShardSyncer, tableCreatorCallback, dynamoDbRequestTimeout, BillingMode.PROVISIONED);
hierarchicalShardSyncer, tableCreatorCallback, dynamoDbRequestTimeout, BillingMode.PAY_PER_REQUEST);
}

/**
Expand Down
41 changes: 30 additions & 11 deletions ...-client/src/main/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseRefresher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public DynamoDBLeaseRefresher(final String table, final DynamoDbAsyncClient dyna
public DynamoDBLeaseRefresher(final String table, final DynamoDbAsyncClient dynamoDBClient,
final LeaseSerializer serializer, final boolean consistentReads,
@NonNull final TableCreatorCallback tableCreatorCallback, Duration dynamoDbRequestTimeout) {
this(table, dynamoDBClient, serializer, consistentReads, tableCreatorCallback, dynamoDbRequestTimeout, BillingMode.PROVISIONED);
this(table, dynamoDBClient, serializer, consistentReads, tableCreatorCallback, dynamoDbRequestTimeout, BillingMode.PAY_PER_REQUEST);
}

/**
Expand Down Expand Up @@ -162,16 +162,7 @@ public DynamoDBLeaseRefresher(final String table, final DynamoDbAsyncClient dyna
@Override
public boolean createLeaseTableIfNotExists(@NonNull final Long readCapacity, @NonNull final Long writeCapacity)
throws ProvisionedThroughputException, DependencyException {
try {
if (tableStatus() != null) {
return newTableCreated;
}
} catch (DependencyException de) {
//
// Something went wrong with DynamoDB
//
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, updated interface's javadoc to include this.

ProvisionedThroughput throughput = ProvisionedThroughput.builder().readCapacityUnits(readCapacity)
.writeCapacityUnits(writeCapacity).build();
final CreateTableRequest request;
Expand All @@ -185,6 +176,34 @@ public boolean createLeaseTableIfNotExists(@NonNull final Long readCapacity, @No
.build();
}

return createTableIfNotExists(request);
}

/**
* {@inheritDoc}
*/
@Override
public boolean createLeaseTableIfNotExists()
throws ProvisionedThroughputException, DependencyException {
final CreateTableRequest request = CreateTableRequest.builder().tableName(table).keySchema(serializer.getKeySchema())
.attributeDefinitions(serializer.getAttributeDefinitions())
.billingMode(billingMode).build();

return createTableIfNotExists(request);
}

private boolean createTableIfNotExists(CreateTableRequest request)
throws ProvisionedThroughputException, DependencyException {
try {
if (tableStatus() != null) {
return newTableCreated;
}
} catch (DependencyException de) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

//
// Something went wrong with DynamoDB
//
log.error("Failed to get table status for {}", table, de);
}

final AWSExceptionManager exceptionManager = createExceptionManager();
exceptionManager.add(ResourceInUseException.class, t -> t);
Expand Down
11 changes: 10 additions & 1 deletion ...-client/src/test/java/software/amazon/kinesis/leases/ExceptionThrowingLeaseRefresher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,22 @@ private void throwExceptions(String methodName, ExceptionThrowingLeaseRefresherM

@Override
public boolean createLeaseTableIfNotExists(Long readCapacity, Long writeCapacity)
throws ProvisionedThroughputException, DependencyException {
throws ProvisionedThroughputException, DependencyException {
throwExceptions("createLeaseTableIfNotExists",
ExceptionThrowingLeaseRefresherMethods.CREATELEASETABLEIFNOTEXISTS);

return leaseRefresher.createLeaseTableIfNotExists(readCapacity, writeCapacity);
}

@Override
public boolean createLeaseTableIfNotExists()
throws ProvisionedThroughputException, DependencyException {
throwExceptions("createLeaseTableIfNotExists",
ExceptionThrowingLeaseRefresherMethods.CREATELEASETABLEIFNOTEXISTS);

return leaseRefresher.createLeaseTableIfNotExists();
}

@Override
public boolean leaseTableExists() throws DependencyException {
throwExceptions("leaseTableExists", ExceptionThrowingLeaseRefresherMethods.LEASETABLEEXISTS);
Expand Down
3 changes: 1 addition & 2 deletions ...inesis-client/src/test/java/software/amazon/kinesis/leases/LeaseCoordinatorExerciser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,7 @@ public static void main(String[] args) throws InterruptedException, DependencyEx
LeaseRefresher leaseRefresher = new DynamoDBLeaseRefresher("nagl_ShardProgress", dynamoDBClient,
new DynamoDBLeaseSerializer(), true, TableCreatorCallback.NOOP_TABLE_CREATOR_CALLBACK);

if (leaseRefresher.createLeaseTableIfNotExists(INITIAL_LEASE_TABLE_READ_CAPACITY,
INITIAL_LEASE_TABLE_WRITE_CAPACITY)) {
if (leaseRefresher.createLeaseTableIfNotExists()) {
log.info("Waiting for newly created lease table");
if (!leaseRefresher.waitUntilLeaseTableExists(10, 300)) {
log.error("Table was not created in time");
Expand Down
1 change: 0 additions & 1 deletion ...est/java/software/amazon/kinesis/leases/LeaseIntegrationBillingModePayPerRequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,3 @@ protected DynamoDBLeaseRefresher getLeaseRefresher() {
tableCreatorCallback, LeaseManagementConfig.DEFAULT_REQUEST_TIMEOUT, BillingMode.PAY_PER_REQUEST);
}
}

2 changes: 1 addition & 1 deletion amazon-kinesis-client/src/test/java/software/amazon/kinesis/leases/LeaseIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ protected void starting(Description description) {

protected DynamoDBLeaseRefresher getLeaseRefresher() {
return new DynamoDBLeaseRefresher(tableName, ddbClient, leaseSerializer, true,
tableCreatorCallback, LeaseManagementConfig.DEFAULT_REQUEST_TIMEOUT, BillingMode.PROVISIONED);
tableCreatorCallback, LeaseManagementConfig.DEFAULT_REQUEST_TIMEOUT, BillingMode.PAY_PER_REQUEST);
}

}
Expand Down
43 changes: 37 additions & 6 deletions ...ent/src/test/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseRefresherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;

import org.junit.Assert;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
Expand Down Expand Up @@ -265,7 +266,40 @@ public void testLeaseTableExistsTimesOut() throws Exception {
}

@Test
public void testCreateLeaseTableTimesOut() throws Exception {
public void testCreateLeaseTableProvisionedBillingModeIfNotExists() throws Exception {
leaseRefresher = new DynamoDBLeaseRefresher(TABLE_NAME, dynamoDbClient, leaseSerializer, CONSISTENT_READS,
tableCreatorCallback, LeaseManagementConfig.DEFAULT_REQUEST_TIMEOUT, BillingMode.PROVISIONED);

when(dynamoDbClient.describeTable(any(DescribeTableRequest.class))).thenReturn(mockDescribeTableFuture);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

when(mockDescribeTableFuture.get(anyLong(), any()))
Copy link
Contributor

@joshua-kim joshua-kim Oct 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.thenThrow(ResourceNotFoundException.builder().message("Table doesn't exist").build());

when(dynamoDbClient.createTable(any(CreateTableRequest.class))).thenReturn(mockCreateTableFuture);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with earlier comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

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

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

Copy link
Contributor

@joshua-kim joshua-kim Oct 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we verify interactions w/ the ddb client?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, also for other dependencies.

Assert.assertTrue(result);
}

@Test
public void testCreateLeaseTableIfNotExists() throws Exception {
when(dynamoDbClient.describeTable(any(DescribeTableRequest.class))).thenReturn(mockDescribeTableFuture);
when(mockDescribeTableFuture.get(anyLong(), any()))
.thenThrow(ResourceNotFoundException.builder().message("Table doesn't exist").build());

when(dynamoDbClient.createTable(any(CreateTableRequest.class))).thenReturn(mockCreateTableFuture);
when(mockCreateTableFuture.get(anyLong(), any())).thenReturn(null);

final boolean result = leaseRefresher.createLeaseTableIfNotExists();

Assert.assertTrue(result);
}

Copy link
Contributor

@joshua-kim joshua-kim Oct 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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

  1. If we throw a ResourceNotFoundException, we should expect to return true.
  2. If we throw a LimitExceededException, we should propagate up a ProvisionedThroughputException.
  3. If we throw a DynamoDBException we should throw a DependencyException.
  4. If we throw a TimeoutException we should throw a DependencyException.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Test
public void testCreateLeaseTableProvisionedBillingModeTimesOut() throws Exception {
leaseRefresher = new DynamoDBLeaseRefresher(TABLE_NAME, dynamoDbClient, leaseSerializer, CONSISTENT_READS,
tableCreatorCallback, LeaseManagementConfig.DEFAULT_REQUEST_TIMEOUT, BillingMode.PROVISIONED);
TimeoutException te = setRuleForDependencyTimeout();

when(dynamoDbClient.describeTable(any(DescribeTableRequest.class))).thenReturn(mockDescribeTableFuture);
Expand All @@ -279,10 +313,7 @@ public void testCreateLeaseTableTimesOut() throws Exception {
}

@Test
public void testCreateLeaseTableBillingMode() throws Exception {
leaseRefresher = new DynamoDBLeaseRefresher(TABLE_NAME, dynamoDbClient, leaseSerializer, CONSISTENT_READS,
tableCreatorCallback, LeaseManagementConfig.DEFAULT_REQUEST_TIMEOUT, BillingMode.PAY_PER_REQUEST);

public void testCreateLeaseTableTimesOut() throws Exception {
TimeoutException te = setRuleForDependencyTimeout();

when(dynamoDbClient.describeTable(any(DescribeTableRequest.class))).thenReturn(mockDescribeTableFuture);
Expand All @@ -292,7 +323,7 @@ public void testCreateLeaseTableBillingMode() throws Exception {
when(dynamoDbClient.createTable(any(CreateTableRequest.class))).thenReturn(mockCreateTableFuture);
when(mockCreateTableFuture.get(anyLong(), any())).thenThrow(te);

verifyCancel(mockCreateTableFuture, () -> leaseRefresher.createLeaseTableIfNotExists(10L, 10L));
verifyCancel(mockCreateTableFuture, () -> leaseRefresher.createLeaseTableIfNotExists());
}

@FunctionalInterface
Expand Down