GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Avro format implementation by sunng87 · Pull Request #407 · cloudevents/sdk-java

Conversation

@sunng87

@sunng87 sunng87 commented Jul 16, 2021 •
edited by pierDipi
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

This patch is to provide an Avro format implementation for the CloudEvent 1.0.

Fixes #115

TODO items:

  • unit tests
  • extension fields?
  • data

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sunng87 added 6 commits July 16, 2021 21:31
@sunng87
(chore) initial commit
6575ac5 
Signed-off-by: Ning Sun <[email protected]>
@sunng87
import avro schema
662032e 
Signed-off-by: Ning Sun <[email protected]>
@sunng87
wip add serializer and deserializer for avro
0b64945 
Signed-off-by: Ning Sun <[email protected]>
@sunng87
wip improve serializer and deserializer
b0f17b8 
Signed-off-by: Ning Sun <[email protected]>
@sunng87
fix time serde and add data deserialization support
371d31e 
Signed-off-by: Ning Sun <[email protected]>
@sunng87
fix maven plugin version
30ca8ad 
Signed-off-by: Ning Sun <[email protected]>
@sunng87
sunng87 force-pushed the feature/avro branch 2 times, most recently from fca0a76 to 0e4cc31 Compare July 17, 2021 15:10
sunng87 added 2 commits July 17, 2021 23:11
@sunng87
add avro data wrapper and hides serde classes from public
8d8a668 
Signed-off-by: Ning Sun <[email protected]>
@sunng87
add test and fix serde issues
8505d78 
Signed-off-by: Ning Sun <[email protected]>
@sunng87
sunng87 force-pushed the feature/avro branch from 0e4cc31 to 8505d78 Compare July 17, 2021 15:13
@sunng87
sunng87 marked this pull request as ready for review July 19, 2021 08:18
@tompoch

tompoch commented Jul 20, 2021

Copy link
Copy Markdown

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tompoch

tompoch commented Jul 20, 2021

Copy link
Copy Markdown

I would expect some reference to the schema-registry maintaining the avro schemas. Am I missing something?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sunng87

sunng87 commented Jul 22, 2021

Copy link
Copy Markdown
Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pierDipi pierDipi self-assigned this Nov 3, 2021
@pierDipi
pierDipi self-requested a review November 3, 2021 07:47
@pierDipi pierDipi removed their assignment Nov 3, 2021
pierDipi
pierDipi reviewed Nov 9, 2021
View reviewed changes

@pierDipi pierDipi left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread formats/avro/src/main/java/io/cloudevents/avro/AvroCloudEventDataWrapper.java
Comment on lines +38 to +41
public AvroCloudEventDataWrapper(Map<String, Object> data) {
avroCloudEventData = new AvroCloudEventData();
avroCloudEventData.setValue(data);
}

@pierDipi pierDipi Nov 9, 2021

Copy link
Copy Markdown
Member

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?

Suggested change
public AvroCloudEventDataWrapper(Map<String, Object> data) {
avroCloudEventData = new AvroCloudEventData();
avroCloudEventData.setValue(data);
}
public AvroCloudEventDataWrapper(AvroCloudEventData avroCloudEventData) {
this.avroCloudEventData = Objects.requireNonNull(avroCloudEventData);
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sunng87 sunng87 Nov 10, 2021

Copy link
Copy Markdown
Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pierDipi pierDipi Nov 13, 2021

Copy link
Copy Markdown
Member

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?

Suggested change
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));
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sunng87 reacted with heart emoji
All reactions
  • ❤️ 1 reaction
Comment thread formats/avro/src/main/java/io/cloudevents/avro/AvroCloudEventDataWrapper.java Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread formats/avro/pom.xml Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread formats/avro/src/main/java/io/cloudevents/avro/AvroSerializer.java Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread formats/avro/src/main/java/io/cloudevents/avro/AvroCloudEventDataWrapper.java Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread formats/avro/src/test/java/io/cloudevents/avro/AvroFormatTest.java
Map<String, Object> testData = Map.of("name", "Ning", "age", 22.0);

@Test
public void testSerde() {

@pierDipi pierDipi Nov 9, 2021

Copy link
Copy Markdown
Member

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 class AvroCloudEventDataWrapperTest 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sunng87 sunng87 Nov 10, 2021

Copy link
Copy Markdown
Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pierDipi reacted with thumbs up emoji pierDipi reacted with eyes emoji
All reactions
  • 👍 1 reaction
  • 👀 1 reaction
Comment thread formats/avro/src/main/java/io/cloudevents/avro/AvroFormat.java Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread formats/avro/pom.xml Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread formats/avro/src/main/avro/spec.avsc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@sunng87
sunng87 force-pushed the feature/avro branch from 123bf68 to 4732e2d Compare November 10, 2021 14:25
@sunng87
addressing some review comments for avro format
c659e70 
Signed-off-by: Ning Sun <[email protected]>
@sunng87
sunng87 force-pushed the feature/avro branch 2 times, most recently from d9fe8c4 to 37c4a56 Compare November 11, 2021 16:07
@sunng87
test: add test case for v03
3af281b 
Signed-off-by: Ning Sun <[email protected]>
@sunng87
sunng87 force-pushed the feature/avro branch from 37c4a56 to 3af281b Compare November 13, 2021 12:28
@pkgonan

pkgonan commented Mar 31, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

@sunng87 @pierDipi Hi. Any changes of this pr?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sunng87

sunng87 commented Mar 31, 2022

Copy link
Copy Markdown
Author

hi @pkgonan I'm no longer working on this. Feel free to fork and continue this work
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pierDipi pierDipi pierDipi left review comments

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Modularization: avro module

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@sunng87 @tompoch @pkgonan @pierDipi
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.