Skip to content

Fix the DynamoDB metadata table upsert operation #2769

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

Merged
merged 4 commits into from
Jun 18, 2025
Merged

Fix the DynamoDB metadata table upsert operation #2769

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c04965
Fix the DynamoDB metadata table upsert operation to handle retries wh…
KodaiD Jun 16, 2025
67c5e94
Add unit tests
KodaiD Jun 17, 2025
0ee5ca2
Apply the suggestion
KodaiD Jun 17, 2025
1ae0e64
Merge branch 'master' into fix-dynamo-metadata-upsert
feeblefakie Jun 18, 2025
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
61 changes: 43 additions & 18 deletions core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ public class DynamoAdmin implements DistributedStorageAdmin {
public static final String DEFAULT_NO_BACKUP = "false";
public static final String DEFAULT_REQUEST_UNIT = "10";
private static final int DEFAULT_WAITING_DURATION_SECS = 3;
@VisibleForTesting static final int DEFAULT_MAX_RETRY_COUNT = 10;

@VisibleForTesting static final String PARTITION_KEY = "concatenatedPartitionKey";
@VisibleForTesting static final String CLUSTERING_KEY = "concatenatedClusteringKey";
Expand Down Expand Up @@ -238,15 +239,26 @@ public void createNamespace(String nonPrefixedNamespace, Map<String, String> opt
private void upsertIntoNamespacesTable(Namespace namespace) throws ExecutionException {
Map<String, AttributeValue> itemValues = new HashMap<>();
itemValues.put(NAMESPACES_ATTR_NAME, AttributeValue.builder().s(namespace.prefixed()).build());
try {
client.putItem(
PutItemRequest.builder()
.tableName(ScalarDbUtils.getFullTableName(metadataNamespace, NAMESPACES_TABLE))
.item(itemValues)
.build());
} catch (Exception e) {
throw new ExecutionException(
"Inserting the " + namespace + " namespace into the namespaces table failed", e);
int retryCount = 0;
Copy link

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

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

[nitpick] The retry logic in upsertIntoNamespacesTable and upsertTableMetadata is duplicated. Consider extracting it into a shared helper method to reduce code duplication and improve maintainability.

Copilot uses AI. Check for mistakes.
while (true) {
try {
client.putItem(
PutItemRequest.builder()
.tableName(ScalarDbUtils.getFullTableName(metadataNamespace, NAMESPACES_TABLE))
.item(itemValues)
.build());
return;
} catch (ResourceNotFoundException e) {
if (retryCount >= DEFAULT_MAX_RETRY_COUNT) {
throw new ExecutionException(
"Inserting the " + namespace + " namespace into the namespaces table failed", e);
}
Uninterruptibles.sleepUninterruptibly(waitingDurationSecs, TimeUnit.SECONDS);
Copy link

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

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

[nitpick] Using a fixed delay may lead to unnecessary wait times or throttling. Consider implementing exponential backoff (with jitter) to more efficiently handle table readiness.

Suggested change
Uninterruptibles.sleepUninterruptibly(waitingDurationSecs, TimeUnit.SECONDS);
long backoffDelay = calculateExponentialBackoffWithJitter(retryCount);
Uninterruptibles.sleepUninterruptibly(backoffDelay, TimeUnit.MILLISECONDS);

Copilot uses AI. Check for mistakes.
retryCount++;
} catch (Exception e) {
throw new ExecutionException(
"Inserting the " + namespace + " namespace into the namespaces table failed", e);
}
}
}

Expand Down Expand Up @@ -470,15 +482,28 @@ private void upsertTableMetadata(Namespace namespace, String table, TableMetadat
METADATA_ATTR_SECONDARY_INDEX,
AttributeValue.builder().ss(metadata.getSecondaryIndexNames()).build());
}
try {
client.putItem(
PutItemRequest.builder()
.tableName(ScalarDbUtils.getFullTableName(metadataNamespace, METADATA_TABLE))
.item(itemValues)
.build());
} catch (Exception e) {
throw new ExecutionException(
"Adding the metadata for the " + getFullTableName(namespace, table) + " table failed", e);
int retryCount = 0;
while (true) {
try {
client.putItem(
PutItemRequest.builder()
.tableName(ScalarDbUtils.getFullTableName(metadataNamespace, METADATA_TABLE))
.item(itemValues)
.build());
return;
} catch (ResourceNotFoundException e) {
if (retryCount >= DEFAULT_MAX_RETRY_COUNT) {
throw new ExecutionException(
"Adding the metadata for the " + getFullTableName(namespace, table) + " table failed",
e);
}
Uninterruptibles.sleepUninterruptibly(waitingDurationSecs, TimeUnit.SECONDS);
retryCount++;
} catch (Exception e) {
throw new ExecutionException(
"Adding the metadata for the " + getFullTableName(namespace, table) + " table failed",
e);
}
}
}

Expand Down
45 changes: 45 additions & 0 deletions core/src/test/java/com/scalar/db/storage/dynamo/DynamoAdminTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,39 @@ public void createTable_WhenMetadataTableExists_ShouldCreateOnlyTable()
.isInstanceOf(IllegalArgumentException.class);
}

@Test
public void createTable_WhenActualMetadataTableCreationIsDelayed_ShouldFailAfterRetries() {
// Arrange
when(client.describeTable(any(DescribeTableRequest.class))).thenReturn(tableIsActiveResponse);
when(client.describeContinuousBackups(any(DescribeContinuousBackupsRequest.class)))
.thenReturn(backupIsEnabledResponse);
when(client.putItem(any(PutItemRequest.class))).thenThrow(ResourceNotFoundException.class);
TableMetadata metadata =
TableMetadata.newBuilder()
.addPartitionKey("c1")
.addClusteringKey("c2", Order.DESC)
.addClusteringKey("c3", Order.ASC)
.addColumn("c1", DataType.TEXT)
.addColumn("c2", DataType.BIGINT)
.addColumn("c3", DataType.BOOLEAN)
.addColumn("c4", DataType.INT)
.addColumn("c5", DataType.BLOB)
.addColumn("c6", DataType.DOUBLE)
.addColumn("c7", DataType.FLOAT)
.addColumn("c8", DataType.DATE)
.addColumn("c9", DataType.TIME)
.addColumn("c10", DataType.TIMESTAMP)
.addColumn("c11", DataType.TIMESTAMPTZ)
.addSecondaryIndex("c4")
.build();

// Act Assert
assertThatThrownBy(() -> admin.createTable(NAMESPACE, TABLE, metadata))
.isInstanceOf(ExecutionException.class);
verify(client, times(DynamoAdmin.DEFAULT_MAX_RETRY_COUNT + 1))
.putItem(any(PutItemRequest.class));
}

@Test
public void dropTable_WithNoMetadataLeft_ShouldDropTableAndDeleteMetadata()
throws ExecutionException {
Expand Down Expand Up @@ -1436,6 +1469,18 @@ public void createNamespace_WithExistingNamespacesTable_ShouldAddNamespace()
.build());
}

@Test
public void createNamespace_WhenActualNamespaceTableCreationIsDelayed_ShouldFailAfterRetries() {
// Arrange
when(client.putItem(any(PutItemRequest.class))).thenThrow(ResourceNotFoundException.class);

// Act Assert
assertThatThrownBy(() -> admin.createNamespace(NAMESPACE, Collections.emptyMap()))
.isInstanceOf(ExecutionException.class);
verify(client, times(DynamoAdmin.DEFAULT_MAX_RETRY_COUNT + 1))
.putItem(any(PutItemRequest.class));
}

@Test
public void namespaceExists_WithExistingNamespace_ShouldReturnTrue() throws ExecutionException {
// Arrange
Expand Down