-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Merged
yunhaoling
merged 30 commits into
Azure:feature/eventhub/idempotent-producer
from
yunhaoling:yuling/eh/idempotent-producer
Mar 4, 2021
Merged
Changes from 24 commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
98d7c03
sync idempotent producer constructor
yunhaoling 45b5622
sync idempotent producer prototype
yunhaoling 173252e
imporve constants
yunhaoling 68da29e
async impl
yunhaoling 556685e
remove duplicate code of validation on outgoing eventdata
yunhaoling 282dbe2
fix bug, add basic test
yunhaoling 4438a7f
fix mypy and pylint
yunhaoling 4ddf3fe
fix implementation bug
yunhaoling 4a0b979
review feedback
yunhaoling 653b4ca
validate partition configs
yunhaoling 37bbdfe
add tests
yunhaoling 25e6951
add changelog and samples
yunhaoling dc8c561
update readme
yunhaoling 3b07456
update shared-requirements
yunhaoling 8f9e47a
merge master
yunhaoling 8f5cf9f
update tests yml to test unreleased uamqp v1.2.15
yunhaoling 74d0f6a
fix tests
yunhaoling c0fd953
more test fix
yunhaoling ef77168
more test fix
yunhaoling 41026c9
addressing comments
yunhaoling e1fb000
add more docs
yunhaoling a89b04a
fix tests
yunhaoling 199631d
fix tox warning
yunhaoling 2bbe734
fix pylint
yunhaoling d47e6a2
fix pylint
yunhaoling 005e6cb
remove the change in tests.yml
yunhaoling aa6b0e4
revert non-existing links first
yunhaoling f3535ff
update setup.py
yunhaoling 5b8e65c
Merge remote-tracking branch 'central/master' into yuling/eh/idempote…
yunhaoling 2b12883
fix pylint
yunhaoling File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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