Skip to content
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

feat: add avro-turbo format #1197

Closed
wants to merge 0 commits into from
Closed

feat: add avro-turbo format #1197

wants to merge 0 commits into from

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Apr 27, 2023

Fixes #1187

@alexec alexec marked this pull request as ready for review April 30, 2023 01:26
@alexec
Copy link
Contributor Author

alexec commented Apr 30, 2023

I’ve raised this PR as a modification to the Avro format so it is clear how it is different. Not mergable in current state

@alexec
Copy link
Contributor Author

alexec commented Apr 30, 2023

@JemDay @doug for review.

@duglin
Copy link
Collaborator

duglin commented May 2, 2023

Did we want to rename the existing one or create an alternative?

@duglin
Copy link
Collaborator

duglin commented May 2, 2023

@alexec
Copy link
Contributor Author

alexec commented May 2, 2023

We want to create an alternative. I just did it like this so you can see the differences.

@duglin
Copy link
Collaborator

duglin commented May 3, 2023

@alexec if you recreate the original file, add the new one to the README and address the comment above I think we should be able to get this one in on tomorrow's ca..

@alexec
Copy link
Contributor Author

alexec commented May 3, 2023

@alexec if you recreate the original file, add the new one to the README and address the comment above I think we should be able to get this one in on tomorrow's ca..

done

README.md Outdated Show resolved Hide resolved
cloudevents/SDK.md Outdated Show resolved Hide resolved
@duglin
Copy link
Collaborator

duglin commented May 3, 2023

Link checker:

cloudevents/formats/avro-turbo-format.md: Translation file cloudevents/languages/zh-CN/formats/avro-turbo-format.md does not exist
cloudevents/formats/avro-turbo-format.md: Translation file cloudevents/languages/he/formats/avro-turbo-format.md does not exist

I think you can just create those 2 files and copy in the content from this file:
https://github.com/cloudevents/spec/blob/main/cloudevents/languages/zh-CN/formats/protobuf-format.md
and just change the English title. Then things should be good.

cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
@JemDay
Copy link
Contributor

JemDay commented May 3, 2023

One thing that's missing from this and the existing Avro format spec is the statement about the media-type to use for structured-mode.

My assumption is that that application/cloudevents+avro is used for the existing format spec, do we need a different media-type for turbo mode ?

@jskeet
Copy link
Contributor

jskeet commented May 4, 2023

I haven't looked at this yet, and won't have time before the meeting, but will do so tomorrow or next week. Prepare for nitpicking :)

@duglin
Copy link
Collaborator

duglin commented May 7, 2023

ping @JemDay @jskeet for review

@duglin
Copy link
Collaborator

duglin commented May 8, 2023

@alexec hope it's ok I edited the original comment so #1187 will auto-close

cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
@jskeet
Copy link
Contributor

jskeet commented May 10, 2023

I'm planning on reviewing this, but it's likely to be tomorrow - today I really want to create the issue around formats and data, and that'll take quite a lot of my time.

cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
cloudevents/formats/avro-turbo-format.md Outdated Show resolved Hide resolved
@alexec
Copy link
Contributor Author

alexec commented May 17, 2023

@jskeet most of this doc is copied from the original and updated. Whenever you suggest a change, in many cases it must mean you want the original changed too.

But that can’t be true, because if that was needed you would commented on the original.

Please advise.

@jskeet
Copy link
Contributor

jskeet commented May 17, 2023

But that can’t be true, because if that was needed you would commented on the original.

I've never gone through the original carefully, as it was already in place when I became interested in CloudEvents, and hasn't come up much in discussions. There may well be many things I'd have raised before it was approved, but which can't be changed now - but which can be fixed in this new format.

@duglin
Copy link
Collaborator

duglin commented May 17, 2023

rebase on SDK.md is needed

@alexec
Copy link
Contributor Author

alexec commented May 17, 2023

But that can’t be true, because if that was needed you would commented on the original.

I've never gone through the original carefully, as it was already in place when I became interested in CloudEvents, and hasn't come up much in discussions. There may well be many things I'd have raised before it was approved, but which can't be changed now - but which can be fixed in this new format.

Would you like to suggest changes by way of Github suggestions or a pull request? Do you want to do that to the original version? To be frank, I don’t really want to fix these issues because I want to move on so writing the Java SDK, and so I don’t want to spend another three weeks on rewriting this pull request.

@jskeet
Copy link
Contributor

jskeet commented May 17, 2023

Would you like to suggest changes by way of Github suggestions or a pull request?

I don't have time to do that at the moment, I'm afraid.

Do you want to do that to the original version?

No, because the original format has already been approved and implemented - a lot of the changes I'm suggesting here can't be retrofitted because they'd be breaking changes. (Some changes to wording could be, but I don't have time to go through it right now.)

To be frank, I don’t really want to fix these issues because I want to move on so writing the Java SDK, and so I don’t want to spend another three weeks on rewriting this pull request.

I would hope that the issues I've raised wouldn't actually take that long - and I'd also hope that fixing them now would save time in the long run.

@duglin
Copy link
Collaborator

duglin commented May 18, 2023

@alexec let me know when you think this one is ready (all comments have been addressed) and then we can add it to the agenda for a vote.

@duglin
Copy link
Collaborator

duglin commented May 23, 2023

@alexec rebase needed. Did you want to get this one in soon?

@alexec
Copy link
Contributor Author

alexec commented May 26, 2023

Had to create new PR due to Git-spolsion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avro
4 participants