-
Notifications
You must be signed in to change notification settings - Fork 166
Introduce CloudEventData #250
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
Introduce CloudEventData #250
Conversation
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
pierDipi
left a comment
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.
LGTM
|
Additional changes did not address the core problem which introduces unnecessary type dependency in CloudEvent strategy. So definitive -1 |
|
If Eg .. If I pass a JsonValue I'd hope/expect that the resultant CloudEvent if using a JSONFormat would have the JSON content I the Apologies if I'm missing something.. |
|
@JemDay that's the idea of this PR: with |
|
@n3wscott @duglin @slinkydeveloper I am trying to understand how this change was actually merged, given that following the current and established community process this has been rejected several times providing detailed justification to which there was no followups? |
Introduction
I thought a little bit about the problem with the data representation. To do a little recap, we have open these issues/PRs with various discussions about this topic:
I tried to put together all the different discussions we had, the different feedbacks and prepare a proposal to solve the issue, hopefully once and for all 😄
What we're trying to solve
These are the problems we're trying to tackle:
All these problems goes down to a problem: today data can be represented, by our interfaces, only with a byte array.
How other sdks fix this issue
Before going deep into this proposal, I analyzed a bit what the other SDKs do:
dataasobject(the javaObject). No particular handling is done about itHow this new interface works
So this proposal is pretty simple: we add a layer of indirection between data and bytes with a new interface called
CloudEventData. In particular:getData()now returns an implementation ofCloudEventData(if any data). Whatever implementation is, it must be able to convert to bytesCloudEventDatahas a single method,toBytes(), that identify that going fromdatato bytes is a potentially expensive operation (because it may trigger a conversion from the internal type to bytes).CloudEventDataadding whatever they want: marshaller/unmarshaller to pojos, a specific type representing the integration with their own data type, ...Then the change to
datais ported to writer, making doable the optimizations to event format/protocol bindings.With this new interface, in order to support POJOs, in core we need to (in a followup, not this pr):
POJOCloudEventDataPOJOCloudEventData(it may beBaseCloudEventitself)EventDataCodecinterface to marshal a pojo to bytesThis change doesn't modify the existing assumptions, nor relaxing or enforcing them. It just makes the existing interface more extensible.
What I think it's still a problem in this interface is that we force users to downcast often to retrieve the non bytes internal representation.
Note: This PR is incomplete, I would love to gather feedback first and then move forward with all the changes required to make it working.
cc @johanhaleby @aliok @pierDipi @bsideup
Signed-off-by: Francesco Guardiani francescoguard@gmail.com