Add retry logic for notification updates on concurrent entity modification - #5222
Conversation
0647c4e to
6dd1d69
Compare
|
Thanks for working on this. The bounded retry looks reasonable as a tactical fix for the existing notification endpoint. I think this PR is also another concrete example for the ongoing dev@ discussion about consistent changes and the persistence boundary. This code has to decide locally which conflict is retryable, how often to retry, which state to reload and reconstruct, and which validation remains valid across attempts. The I do not expect this PR or its author to solve that broader design, so this is not a blocker for the tactical fix. However, I would avoid treating this endpoint-local loop as a retry pattern to copy. The common follow-up belongs in the dev@ discussion: the persistence contract should distinguish one backend atomic attempt from the logical operation that reloads and revalidates state before retrying, and should expose sufficiently precise conflict and outcome semantics to do that consistently across backends. |
|
Thanks for the review @snazy and for the context on the dev@ discussion. That framing makes sense. Working through this, the loop ended up making four decisions that are not really the endpoint's to make: which return status counts as a retryable conflict, how many attempts are reasonable, what state to reload, and which validations still hold on a retry. The last one was the least obvious. I kept the location checks, FileIO load, and metadata parse outside the loop because their results do not change across attempts, but that is a judgment I made locally rather than something the persistence contract told me. The distinction you draw between a single atomic backend attempt and the logical operation that reloads and revalidates above it matches what I ran into here. Agreed on not treating this as a pattern to copy into other paths. I will follow both threads. |
|
The
|
6dd1d69 to
f776268
Compare
When an UPDATE notification is processed in
sendNotificationForTableLike, the entity may have been concurrently modified by another request between the initial read and the call toupdateEntityPropertiesIfNotChanged. This causes aCommitConflictExceptionthat propagates directly to the caller with no recovery, even though the update could succeed on a simple retry.This PR adds a bounded retry loop (up to 3 attempts) for the UPDATE path in notification handling. On a
CommitConflictException, the retry logic:loadEntityto get the latest versionIf the re-read entity has already processed a newer notification, the retry correctly throws
AlreadyExistsExceptioninstead of blindly overwriting. If all attempts are exhausted, theoriginal
CommitConflictExceptionpropagates as before.Expensive validations (location checks, FileIO loading, metadata parsing) stay outside the retry
loop since their results do not change between attempts.
This resolves the TODO comment that was already present at line 3096 of
LocalIcebergCatalog.java.The CREATE path retry is left for a follow-up.
Fixes #4658
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)Tests added
testNotificationUpdateRetriesOnConcurrentModification: Creates a table via CREATE notification,then sends an UPDATE notification with a spy that returns
TARGET_ENTITY_CONCURRENTLY_MODIFIEDonthe first attempt and delegates to the real method on the second. Verifies the notification
succeeds and that exactly 2 attempts were made.
testNotificationUpdateGivesUpAfterMaxRetries: Creates a table via CREATE notification, thensends an UPDATE notification with a spy that always returns
TARGET_ENTITY_CONCURRENTLY_MODIFIED. Verifies thatCommitConflictExceptionis thrown afterexhausting all retry attempts.
Both tests pass on the Relational and NoSqlInMem implementations.