[EventHubs] Idempotent producer #16756
Conversation
ghost
added
the
yunhaoling
requested review from
annatisch,
rakshith91 and
swathipil
as code owners
| @@ -392,6 +417,20 @@ def size_in_bytes(self): | |||
| """ | |||
| return self._size | |||
|
|
|||
| @property | |||
| def starting_published_sequence_number(self): | |||
There was a problem hiding this comment.
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.
IIRC batches are transactional right? There's no partial failure here?
There was a problem hiding this comment.
yes, batches are transactional, either all events in the batch get sent or failed.
There was a problem hiding this comment.
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 with EventData.
@jsquire , would you be able to provide some context on the "starting_" prefix being chosen instead of just starting_published_sequence_number?
There was a problem hiding this comment.
"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.
I have created a separate issue for further discussion on the namings: #16994
There was a problem hiding this comment.
Yeah we can keep as-is for preview and settle before GA
There was a problem hiding this comment.
the preview may stay for a while (or even be pulled out) so we should have time to discuss among languages :P
sdk/eventhub/azure-eventhub/azure/eventhub/aio/_producer_client_async.py
Show resolved
Hide resolved
sdk/eventhub/azure-eventhub/azure/eventhub/aio/_producer_client_async.py
Show resolved
Hide resolved
sdk/eventhub/azure-eventhub/tests/livetest/asynctests/test_send_async.py
Show resolved
Hide resolved
|
/azp run python - eventhub - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - eventhub - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - eventhub - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - eventhub - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - eventhub - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - eventhub - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -135,7 +135,8 @@ async def on_event(partition_context, event): | |||
| ed = EventData("Event Number {}".format(i)) | |||
| ed.properties = app_prop | |||
| event_list.append(ed) | |||
| senders[0].send(event_list) | |||
|
|
|||
| senders[0].send(EventDataBatch._from_batch(event_list)) | |||
There was a problem hiding this comment.
I find it funny that in our tests (both EH and SB), we use these private shortcuts to create a batch...
I wonder how many customers do the same :P
There was a problem hiding this comment.
yea.. I remember there was once a discussion on enabling the Batch object to accept a list, however the pitfall here is when trying to adding a list into batch which exceeds the batch size, then users still have to loop over the list and add event one by one to avoid exceeding the size limit.
sdk/eventhub/azure-eventhub/tests/livetest/synctests/test_consumer_client.py
Show resolved
Hide resolved
yunhaoling
changed the base branch from
master
to
feature/eventhub/idempotent-producer
|
/azp run python - eventhub - tests |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
livetest passed which could be found in the |
* sync idempotent producer constructor * sync idempotent producer prototype * imporve constants * async impl * remove duplicate code of validation on outgoing eventdata * fix bug, add basic test * fix mypy and pylint * fix implementation bug * review feedback * validate partition configs * add tests * add changelog and samples * update readme * update shared-requirements * update tests yml to test unreleased uamqp v1.2.15 * fix tests * more test fix * more test fix * addressing comments * add more docs * fix tests * fix tox warning * fix pylint * fix pylint * remove the change in tests.yml * revert non-existing links first * update setup.py * fix pylint
* sync idempotent producer constructor * sync idempotent producer prototype * imporve constants * async impl * remove duplicate code of validation on outgoing eventdata * fix bug, add basic test * fix mypy and pylint * fix implementation bug * review feedback * validate partition configs * add tests * add changelog and samples * update readme * update shared-requirements * update tests yml to test unreleased uamqp v1.2.15 * fix tests * more test fix * more test fix * addressing comments * add more docs * fix tests * fix tox warning * fix pylint * fix pylint * remove the change in tests.yml * revert non-existing links first * update setup.py * fix pylint
gist: https://gist.github.com/yunhaoling/c17ddd3c1bfbe5c4ba156a8eb0e54e7d
issue: #12427
pending uamqp v1.2.15 release: Azure/azure-uamqp-python#210