-
Notifications
You must be signed in to change notification settings - Fork 3
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.
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
use writer UUID + sequence number as a request ID for transactional writes #94
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼 yes, good catch. Need to include a token for retries.
val firstEvent = events.head; | ||
val token = s"${firstEvent.writerUuid}-${firstEvent.seqNr.toString}" | ||
|
||
val req = TransactWriteItemsRequest | ||
.builder() | ||
.clientRequestToken(token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like client request token has a max length of 36. So only enough for the UUID in default format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... then the idempotence would be a problem... so I guess taking the last 36 characters of ${writerUuid}-${seqNr}
would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we encode the UUID more efficiently. Say parse it from string and rewrite as base64. Then it would only take up 22 characters.
bb.asLongBuffer() | ||
.put(uuid.getMostSignificantBits) | ||
.put(uuid.getLeastSignificantBits) | ||
.put(seqNr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the maximally compact encoding (e.g. nearly all seqNrs will have quite a few zero high-bytes), but 24 bytes will base64 encode into 32 characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼 I think this is good. We get everything needed, and always within the character limit.
bb.asLongBuffer() | ||
.put(uuid.getMostSignificantBits) | ||
.put(uuid.getLeastSignificantBits) | ||
.put(seqNr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼 I think this is good. We get everything needed, and always within the character limit.
We've observed a write of multiple events failing with a
software.amazon.awssdk.services.dynamodb.model.TransactionInProgressException: Transaction from previous request is still in progress
, indicating that two concurrently executing transactional writes have the same request token.The plugin has the AWS SDK generate the token (by not specifying one), so the underlying issue seems to be in the SDK. This instead uses the writer UUID (unique per event-sourced actor instantiation) with the sequence number of the first event in the batch appended, to ensure uniqueness (restart of the actor will generate a new UUID, successive persists by the same actor will increment the sequence number component).