Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[EventHubs] Idempotent producer #16756
[EventHubs] Idempotent producer #16756
Changes from 14 commits
98d7c03
45b5622
173252e
68da29e
556685e
282dbe2
4438a7f
4ddf3fe
4a0b979
653b4ca
37bbdfe
25e6951
dc8c561
3b07456
8f9e47a
8f5cf9f
74d0f6a
c0fd953
ef77168
41026c9
e1fb000
a89b04a
199631d
2bbe734
d47e6a2
005e6cb
aa6b0e4
f3535ff
5b8e65c
2b12883
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 still feel this needs a rename....
And I think my preference is to go with
published_sequence_number
have the documentation indicate that this refers to the first in the batch.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.
IIRC batches are transactional right? There's no partial failure here?
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, batches are transactional, either all events in the batch get sent or failed.
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 don't have a strong preference on the name -- personally I think the "starting_" prefix adds some clarity for a batch object while with just
published_sequence_number
would give us the benefit of consistency withEventData
.@jsquire , would you be able to provide some context on the "starting_" prefix being chosen instead of just
starting_published_sequence_number
?cc: @chradek @conniey
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.
"Starting" was chosen to denote that it was the sequence number applied to the first event in the batch. The batch itself is not assigned a sequence number, nor is it considered a single item by the service.
To determine the next sequence number that the service expects, one has to take the batch's starting sequence number and increment it by the number of events that were in the batch. This caused confusion for our beta users that expected the "LastPublishedSequenceNumber" returned as part of the partition publishing properties did not align with the "PublishedSequenceNumber" that we assigned to the batch. It was agreed that the prefix "Starting" helped as a mnemonic.
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 have created a separate issue for further discussion on the namings: #16994
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.
Yeah we can keep as-is for preview and settle before GA
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.
the preview may stay for a while (or even be pulled out) so we should have time to discuss among languages :P