-
Notifications
You must be signed in to change notification settings - Fork 160
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
Avro format implementation #407
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Ning Sun <[email protected]>
Signed-off-by: Ning Sun <[email protected]>
Signed-off-by: Ning Sun <[email protected]>
Signed-off-by: Ning Sun <[email protected]>
Signed-off-by: Ning Sun <[email protected]>
Signed-off-by: Ning Sun <[email protected]>
fca0a76
to
0e4cc31
Compare
Signed-off-by: Ning Sun <[email protected]>
Signed-off-by: Ning Sun <[email protected]>
What is the relation of this to the content mode as defined in the Kafka Protocol Binding? Do I read correctly that this PR corresponds to the structured mode, but not to the binary? |
I would expect some reference to the schema-registry maintaining the avro schemas. Am I missing something? |
This patch is to implement Avro as a cloudevent serialization format. It doesn't cover kafka binding for now. Perhaps we can provide that integration in future. |
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.
Awesome @sunng87!
I left some comments/questions/suggestions.
public AvroCloudEventDataWrapper(Map<String, Object> data) { | ||
avroCloudEventData = new AvroCloudEventData(); | ||
avroCloudEventData.setValue(data); | ||
} |
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.
Can we inject AvroCloudEventData
directly?
public AvroCloudEventDataWrapper(Map<String, Object> data) { | |
avroCloudEventData = new AvroCloudEventData(); | |
avroCloudEventData.setValue(data); | |
} | |
public AvroCloudEventDataWrapper(AvroCloudEventData avroCloudEventData) { | |
this.avroCloudEventData = Objects.requireNonNull(avroCloudEventData); | |
} |
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 reason I'm using a map here is I tried to avoid exposing generated data type in a public api. The AvroCloudEventData
contains some Avro serialization logic.
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.
OK, sounds good!
Can we add only the Objects.requireNonNull
check?
public AvroCloudEventDataWrapper(Map<String, Object> data) { | |
avroCloudEventData = new AvroCloudEventData(); | |
avroCloudEventData.setValue(data); | |
} | |
public AvroCloudEventDataWrapper(Map<String, Object> data) { | |
avroCloudEventData = new AvroCloudEventData(); | |
avroCloudEventData.setValue(Objects.requireNonNull(data)); | |
} |
formats/avro/src/main/java/io/cloudevents/avro/AvroCloudEventDataWrapper.java
Outdated
Show resolved
Hide resolved
formats/avro/src/main/java/io/cloudevents/avro/AvroSerializer.java
Outdated
Show resolved
Hide resolved
formats/avro/src/main/java/io/cloudevents/avro/AvroCloudEventDataWrapper.java
Outdated
Show resolved
Hide resolved
Map<String, Object> testData = Map.of("name", "Ning", "age", 22.0); | ||
|
||
@Test | ||
public void testSerde() { |
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.
Can we add more test cases?
- Using a CloudEvent v0.3 (
CloudEventBuilder.v03()
) - passing null as avroCloudEventData to
AvroCloudEventDataWrapper
(we can create a new classAvroCloudEventDataWrapperTest
for this - Testing all data types specified in the union type for
data
field (null included): https://github.com/cloudevents/sdk-java/pull/407/files#diff-09cbc79dec7471fa039c796a1fdfc9f13013cb7e9636e23bc32687378f59cb2aR23-R60
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.
Ok. I will do it in next few commits.
123bf68
to
4732e2d
Compare
Signed-off-by: Ning Sun <[email protected]>
d9fe8c4
to
37c4a56
Compare
Signed-off-by: Ning Sun <[email protected]>
37c4a56
to
3af281b
Compare
hi @pkgonan I'm no longer working on this. Feel free to fork and continue this work |
This patch is to provide an Avro format implementation for the CloudEvent 1.0.
Fixes #115
TODO items: