Extend EventData with AmqpAnnotatedMessage by v-hongli1 · Pull Request #22314 · Azure/azure-sdk-for-java

v-hongli1 · 2021-06-16T04:36:56Z

Fix issue Extend EventData with AmqpAnnotatedMessage #21717
Changed files:
1. sdk/eventhubs/azure-messaging-eventhubs/migration-guide.md
2. sdk/eventhubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventData.java
3. sdk/eventhubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventDataBatch.java
4. sdk/eventhubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventHubMessageSerializer.java
5. sdk/eventhubs/azure-messaging-eventhubs/src/test/java/com/azure/messaging/eventhubs/BackCompatTest.java
6. sdk/eventhubs/azure-messaging-eventhubs/src/test/java/com/azure/messaging/eventhubs/EventDataTest.java
7. sdk/eventhubs/azure-messaging-eventhubs/src/test/java/com/azure/messaging/eventhubs/EventHubMessageSerializerTest.java
8. sdk/eventhubs/azure-messaging-eventhubs/src/test/java/com/azure/messaging/eventhubs/EventHubPartitionAsyncConsumerTest.java
9. sdk/eventhubs/azure-messaging-eventhubs/src/test/java/com/azure/messaging/eventhubs/InteropAmqpPropertiesTest.java
10. sdk/eventhubs/azure-messaging-eventhubs/src/test/java/com/azure/messaging/eventhubs/TestUtils.java

@jongio, @conniey for notification

ghost · 2021-06-16T04:37:02Z

Thank you for your contribution hongli750210! We will review the pull request and get back to you soon.

YijunXieMS

EH EventData and SB Message have similar stuffs but also many different properties. We need to remove what is not need for EventData.
The changes on the systemProperties looks pretty risky. Is it possible to keep existing stuffs and only expose new APIs?

...enthubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventData.java

jsquire · 2021-06-28T17:43:31Z

...enthubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventData.java

This should be the message id. We're not exposing user id as a top-level property.

...enthubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventData.java

conniey

In general, please do not remove or add properties that are not described by the issue: #21717

...enthubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventData.java

sdk/eventhubs/azure-messaging-eventhubs/migration-guide.md

...enthubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventData.java

...ssaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventHubMessageSerializer.java

…202106161009

…g 202106161337

…AnnotatedMessage by lihong 202106161337

…AmqpAnnotatedMessage by lihong 202106161625

…r issue#21717 Extend EventData with AmqpAnnotatedMessage by lihong 202106161656

…r issue#21717 Extend EventData with AmqpAnnotatedMessage by lihong 202106161801

…r issue#21717 Extend EventData with AmqpAnnotatedMessage by lihong 202106161820

…c) for issue#21717 Extend EventData with AmqpAnnotatedMessage by lihong 202106301016

…c) for issue#21717 Extend EventData with AmqpAnnotatedMessage by lihong 202106301030

check-enforcer · 2021-07-01T18:44:03Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run java - [service] - ci

...enthubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventData.java

...bs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventDataBatch.java

...ssaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventHubMessageSerializer.java

conniey · 2021-07-07T00:53:19Z

...enthubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventData.java

     */
    public EventData(byte[] body) {
-        this(BinaryData.fromBytes(Objects.requireNonNull(body, "'body' cannot be null.")));
+        this.systemProperties = new SystemProperties();


Since AmqpAnnotatedMessage exposes byte[] as the body, I felt like it was best to store it as such rather than BinaryData that we were using before to stop a lot of the conversions to and from BinaryData.

@conniey, @yijun: This looks like SystemProperties is a copy of data that also appears on the AmqpAnnotatedMessage instance. Since the AmqpAnnotatedMessage instance is fully mutable, if callers change the data there, then the EventData projection is no longer accurate and you've got drift....

Am I overlooking something?

YijunXieMS

LGTM

...enthubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventData.java

…operties itself.

conniey · 2021-07-09T07:04:13Z

/azp run java - eventhubs - tests

azure-pipelines · 2021-07-09T07:04:28Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost added Event Hubs customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Jun 16, 2021

v-hongli1 marked this pull request as ready for review June 17, 2021 02:59

v-hongli1 requested review from YijunXieMS, conniey and srnagar as code owners June 17, 2021 02:59

YijunXieMS suggested changes Jun 25, 2021

View reviewed changes

jsquire reviewed Jun 28, 2021

View reviewed changes

conniey requested changes Jun 28, 2021

View reviewed changes

jsquire reviewed Jun 30, 2021

View reviewed changes

v-hongli1 and others added 10 commits July 1, 2021 11:22

Fix issue#21717 Extend EventData with AmqpAnnotatedMessage by lihong …

0efe342

…202106161009

Fixed issue#21717 Extend EventData with AmqpAnnotatedMessage by lihon…

8d33358

…g 202106161337

Fixed ci exception (build) for issue#21717 Extend EventData with Amqp…

837ed79

…AnnotatedMessage by lihong 202106161337

Fixed ci exception (Test case) for issue#21717 Extend EventData with …

070e4a7

…AmqpAnnotatedMessage by lihong 202106161625

Fixed ci exception (Run SpotBugs, Checkstyle, RevApi, and Javadoc) fo…

56a6f9b

…r issue#21717 Extend EventData with AmqpAnnotatedMessage by lihong 202106161656

Fixed ci exception (Run SpotBugs, Checkstyle, RevApi, and Javadoc) fo…

b5bf879

…r issue#21717 Extend EventData with AmqpAnnotatedMessage by lihong 202106161801

Fixed ci exception (Run SpotBugs, Checkstyle, RevApi, and Javadoc) fo…

a77eae0

…r issue#21717 Extend EventData with AmqpAnnotatedMessage by lihong 202106161820

Fixed comment exception (Run SpotBugs, Checkstyle, RevApi, and Javado…

fe37756

…c) for issue#21717 Extend EventData with AmqpAnnotatedMessage by lihong 202106301016

Fixed comment exception (Run SpotBugs, Checkstyle, RevApi, and Javado…

d96cf39

…c) for issue#21717 Extend EventData with AmqpAnnotatedMessage by lihong 202106301030

Revert change in migration-guide.

a3f0137

jsquire reviewed Jul 2, 2021

View reviewed changes

...enthubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventData.java Outdated Show resolved Hide resolved

jsquire reviewed Jul 2, 2021

View reviewed changes

conniey added 6 commits July 2, 2021 13:01

Revert EventData

609a4dd

Revert EventHubMessageSerializer

16c6e1c

Adds MessageUtils class.

6d7e080

Adding overload to take AmqpAnnotatedMessage.

4eb1cfd

Update EventData to take AmqpAnnotatedMessage as a body.

1493566

Adds contentType, correlationId, messageId to EventData

61a92c0

conniey added 9 commits July 4, 2021 15:17

Update MessageSerializer to add in systemProperties.

3c14bf6

Make EventData more permissive when adding items to the map.

9cb5dd8

(WIP)

4e2c819

Unifying logic for AMQP message conversion.

7587e91

Revert changes in tests.

e28aa67

Only add logic to serialize when properties set.

847562b

Aggregate data for checking if objects are the same.

7f180bf

Update serializer test to also check AmqpRawMessage

b71441c

Fix assertion in SystemProperties test.

456d0ba

conniey reviewed Jul 7, 2021

View reviewed changes

Revert changes to InteropAmqpPropertiesTest.

a3ddaa9

YijunXieMS approved these changes Jul 7, 2021

View reviewed changes

...enthubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventData.java Outdated Show resolved Hide resolved

...enthubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventData.java Show resolved Hide resolved

conniey added 9 commits July 7, 2021 00:38

Adding comment to EventData.

ff884b0

Add extra assertions for RawAmqpMessage.

660ce3c

Add additional tests for message conversions.

65dfd58

Fixing spacing.

31a1e45

Update changelog.

dd8b114

Fixing checkstyle issues.

f914cea

Fixing issues where we poll the convenience method rather than the pr…

ff78517

…operties itself.

Fixing bugs in system properties.

02e4f01

Fixing interop tests.

0c2bdb9

Fix test error.

66239d5

conniey approved these changes Jul 9, 2021

View reviewed changes

Adding extra assertions for properties.

def4eb0

conniey enabled auto-merge (squash) July 9, 2021 15:34

conniey merged commit 5b16856 into Azure:main Jul 9, 2021

conniey deleted the issue#21717 branch July 9, 2021 16:15

moarychan mentioned this pull request Oct 8, 2021

[BUG] UnsupportedOperationException when building nativeHeaders #24649

Closed

Comments

Conversation

v-hongli1 commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 16, 2021

Uh oh!

YijunXieMS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsquire Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

conniey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

check-enforcer bot commented Jul 1, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

conniey Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

jsquire Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

YijunXieMS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

conniey commented Jul 9, 2021

Uh oh!

azure-pipelines bot commented Jul 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

v-hongli1 commented Jun 16, 2021 •

edited

Loading