fix(eventhubs): route batch partition key via message annotations - #7277
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 7 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
The partition-key fix looks correct to me. In particular:
The five failing Event Hubs x64 jobs appear to have the same MSVC warnings-as-errors failure at
auto const remainingEventCount
= static_cast<uint32_t>(eventCount - receivedEvents.size());
auto batchOfEvents = receiver.ReceiveEvents(remainingEventCount);I verified this change locally against PR head |
|
Thanks for catching that. It is the same defect I hit, and the fix is already on the branch as The Event Hubs pipeline is now green on all 41 jobs, including the five x64 jobs that failed before. Your uAMQP build is useful coverage here, because I built against the Rust AMQP stack and did not exercise uAMQP. I also reverted each part of the fix in turn to make sure the new tests catch the regression. Moving the envelope annotation back to The live test still has not run against a real Event Hub, so service-side routing stays unproven. That is the last gap before this leaves draft. The red |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 7 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Fixes Event Hubs batch partition-key routing by ensuring the batch envelope places x-opt-partition-key in AMQP message-annotations (not delivery-annotations) and that the envelope is built from the annotated first message, matching Event Hubs service routing behavior.
Changes:
- Route batch partition key via
MessageAnnotationsand ensure the batch envelope is created from the annotated message copy. - Add offline regression tests validating envelope/inner-message annotations, annotation absence when unset, and replacement of caller-provided annotations.
- Add a LIVE-only test that validates a keyed batch lands on a single partition and that received events report the key; update docs and changelog accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/eventhubs/azure-messaging-eventhubs/test/ut/producer_client_test.cpp | Adds LIVE-only regression test to validate partition-key batch routing to a single partition. |
| sdk/eventhubs/azure-messaging-eventhubs/test/ut/event_data_test.cpp | Adds offline tests asserting partition-key annotation placement and replacement behavior for batches. |
| sdk/eventhubs/azure-messaging-eventhubs/src/event_data_batch.cpp | Fixes envelope and inner-message annotation placement and ensures envelope is built from annotated message. |
| sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/event_data_batch.hpp | Updates docs for partition-key behavior and clarifies TryAdd copy/annotation semantics; adjusts private envelope builder signature. |
| sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md | Documents the bug fix and related behavioral notes. |
EventDataBatch::ToAmqpMessage wrote the x-opt-partition-key value to the AMQP delivery-annotations section of the batch envelope. OASIS AMQP 1.0 section 3.2.2 limits delivery-annotations to a single hop, so no intermediary forwards them. Section 3.2.3 requires intermediaries to propagate message-annotations. The Event Hubs service therefore ignored the key, and a keyed batch spread over all partitions. TryAddAmqpMessage also built the batch envelope from the original message instead of the annotated copy, so the envelope inherited no partition key annotation. The partition key now goes in the message-annotations section, on the batch envelope and on each message in the batch. This matches the Go SDK, which sets the annotation in both places. The code now assigns the annotation instead of calling emplace. The partition key of the batch is the routing key, so it must replace a partition key annotation that the caller set on a raw AMQP message. Fixes #7257
MSVC treats warning C4267 as an error in this repository. The call `ReceiveEvents(eventCount - receivedEvents.size())` widened the uint32_t `eventCount` to size_t, and `ReceiveEvents` takes a uint32_t. All five Win2022 eventhubs jobs failed to build. Clang and GCC do not set that flag, so the Ubuntu and macOS jobs passed. Store the count in a uint32_t local first. The loop guard keeps the difference below `eventCount`, so the cast cannot lose data.
… producers The test found the target partition by the growth of the sequence number of each partition. Another producer that writes to the same Event Hub can add as many events as this batch holds to a different partition. The test then saw two partitions with the whole batch and failed, even though the routing was correct. Each event body now starts with the unique key of the run. The test reads every partition from the sequence number that it had before the send and keeps only the events with that marker. So the count ignores every other producer. The check on the partition key stays independent, because the marker in the body, not the partition key, finds the events. Also correct the PartitionKey doc comment. It said that the service applies the partition key to the batch and to every event. The client does that. The service only reads the annotation and routes on it.
b948372 to
f913684
Compare
Summary
The C++ Event Hubs library ignored the partition key on every batch send.
EventDataBatch::ToAmqpMessagewrote thex-opt-partition-keyvalue to the AMQP delivery-annotations section of the batch envelope. The Event Hubs service reads only the message-annotations section for routing. A batch with a partition key spread over all partitions, which is the same result as sending no partition key.Fixes #7257.
Motivation
OASIS AMQP 1.0 section 3.2.2 states that delivery-annotations "convey information from the sending peer to the receiving peer", and they stop at that hop. Section 3.2.3 states that message-annotations hold properties "aimed at the infrastructure", and that "Intermediaries MUST propagate the annotations unless the annotations are explicitly augmented or modified". The code put a routing key in the one section that no intermediary must forward. See the OASIS AMQP 1.0 messaging specification.
A second defect made the first one impossible to work around.
TryAddAmqpMessagebuilt the batch envelope from the original message rather than the annotated copymessageToSend, so the envelope inherited no partition key annotation to fall back on.The service returns no error for the malformed batch. Each consumed event also reports the correct partition key, because the inner messages carried the annotation correctly. A caller therefore read a correct
PartitionKeyon an event that sat on an arbitrary partition. Per-key ordering and partition affinity were broken for every caller that set a partition key on a batch.C++ was the only Event Hubs SDK that used delivery-annotations. The Go SDK sets the annotation on the envelope and on each message, which is the shape this change adopts.
Changes
ToAmqpMessagewrites the partition key toMessageAnnotationsinstead ofDeliveryAnnotations.TryAddAmqpMessagebuilds the batch envelope from the annotated copymessageToSend.CreateBatchEnvelopenow takes anAmqpMessage const&. The method is private and inline, and the package is beta, so this changes no public API.emplace. The partition key of the batch is the routing key, so it must replace a partition key annotation that the caller set on a raw AMQP message. The message that the caller supplies does not change.event_data_test.cppcover the envelope annotation, the absence of an annotation when no key is set, and the replacement of a caller-set annotation.SendBatchWithPartitionKey_LIVEONLY_, asserts that a keyed batch lands on exactly one partition and that each event carries the key.TryAddapplies the generated message ID and the partition key to a copy.Test plan
Built with the Rust AMQP stack (the default) and ran the offline suite.
Each test fails when the matching part of the fix is reverted, which is the property that makes the tests worth adding:
MessageAnnotationsback toDeliveryAnnotationsPartitionKeyIsInMessageAnnotationsCreateBatchEnvelope(messageToSend)back to the original messagePartitionKeyIsInMessageAnnotationsemplaceBatchPartitionKeyReplacesCallerAnnotationResults:
CheckpointStoreTest.TestCheckpointsfails before and after this change because theEVENTHUB_CONSUMER_GROUPenvironment variable is absent. It is unrelated to this change.clang-formatreports no difference on all four source files.cspellreports zero issues._LIVEONLY_token makes the test framework skip the live test in playback mode.Validation not yet done
The live test has not run against a real Event Hub, because this environment has no Event Hubs credentials. Issue #7257 records that the pre-fix behavior is a round-robin spread across every partition, so the live test must fail before this change and pass after it. That run is still outstanding.