Fix the DynamoDB metadata table upsert operation #2769
Conversation
…en table is not yet available.
There was a problem hiding this comment.
Pull Request Overview
This PR adds retry logic to handle ResourceNotFoundException during metadata upsert operations when a newly created DynamoDB table isn’t immediately available.
- Introduces
DEFAULT_MAX_RETRY_COUNTconstant for retry limits. - Wraps
putItemcalls inupsertIntoNamespacesTableandupsertTableMetadatawith retry loops.
Comments suppressed due to low confidence (2)
core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java:242
- New retry behavior for
ResourceNotFoundExceptionisn’t covered by existing tests. Add unit or integration tests that simulate table-creation delays to verify the retry and timeout logic.
int retryCount = 0;
core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java:253
- [nitpick] The error message for retry exhaustion is identical to the general
Exceptioncatch. Consider differentiating the message to clarify that the retries were exhausted versus other failures.
throw new ExecutionException("Inserting the " + namespace + " namespace into the namespaces table failed", e);
| } catch (Exception e) { | ||
| throw new ExecutionException( | ||
| "Inserting the " + namespace + " namespace into the namespaces table failed", e); | ||
| int retryCount = 0; |
There was a problem hiding this comment.
[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.
| throw new ExecutionException( | ||
| "Inserting the " + namespace + " namespace into the namespaces table failed", e); | ||
| } | ||
| Uninterruptibles.sleepUninterruptibly(waitingDurationSecs, TimeUnit.SECONDS); |
There was a problem hiding this comment.
[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.
| Uninterruptibles.sleepUninterruptibly(waitingDurationSecs, TimeUnit.SECONDS); | |
| long backoffDelay = calculateExponentialBackoffWithJitter(retryCount); | |
| Uninterruptibles.sleepUninterruptibly(backoffDelay, TimeUnit.MILLISECONDS); |
|
@KodaiD I think it would be great if this PR includes new unit tests for the change. Also, this is just an idea, but using https://github.com/failsafe-lib/failsafe or https://github.com/resilience4j/resilience4j (a bit heavy?) might be helpful for this kind of retry. This is not a requirement but just FYI. |
@komamitsu Thank you for your review! I added unit test in 67c5e94. PTAL!
Thank you for your comment on this as well! DynamoAdmin contains multiple existing retry logics in addition to the one I added, so I simply made changes to align with the style of those existing ones. |
brfrn169
left a comment
brfrn169
left a comment
There was a problem hiding this comment.
Left a comment. Other than that, LGTM!
core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java
Outdated
Show resolved
Hide resolved
5 participants
Description
This PR resolves a
ResourceNotFoundExceptionencountered during upsert operations onDynamoAdminmetadata tables.The issue stems from attempts to insert data immediately following table creation, where the table may not be fully ready even if the
DescribeTableresponse isTableStatus.ACTIVE. AWS states that there are cases where table status propagation can take time.This PR adds a retry mechanism to wait until the table becomes available.
Related issues and/or PRs
N/A
Changes made
ResourceNotFoundExceptionduring upsert operations:upsertTableMetadataupsertIntoNamespacesTableChecklist
Additional notes (optional)
N/A
Release notes
N/A