Skip to content

Conversation

@slinkydeveloper
Copy link
Member

@slinkydeveloper slinkydeveloper commented Oct 7, 2020

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:

  • Users wants to read/write pojos in their CloudEvents, making the experience as nice as possible
  • We want to enforce serialization/deserialization to bytes, but we don't want to couple to specific serde libraries
  • We want an extensible representation
  • We want to avoid double encoding/decoding while serializing/deserializing to any event format (look the How to best deal with conversions/seralization to other formats that byte[]? #196 for an example outside the sdk), without coupling to a specific json library types

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:

  • C# just serves data as object (the java Object). No particular handling is done about it
  • Golang represents everything as byte array, so it suffers of the same issues we have
  • Rust has a union type defined as string, bytes or json value that represents data. This is possible because in Rust there is one standard de-facto library to interact with json.

How 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 of CloudEventData (if any data). Whatever implementation is, it must be able to convert to bytes
  • CloudEventData has a single method, toBytes(), that identify that going from data to bytes is a potentially expensive operation (because it may trigger a conversion from the internal type to bytes).
  • 3rd party integrators can implement CloudEventData adding whatever they want: marshaller/unmarshaller to pojos, a specific type representing the integration with their own data type, ...

Then the change to data is 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):

  • Provide a subinterface like POJOCloudEventData
  • Provide a concrete implementation of POJOCloudEventData (it may be BaseCloudEvent itself)
  • EventDataCodec interface to marshal a pojo to bytes

This 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

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper slinkydeveloper added this to the 2.0.0-milestone4 milestone Oct 7, 2020
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper slinkydeveloper added the enhancement New feature or request label Oct 7, 2020
@olegz
Copy link
Contributor

olegz commented Oct 7, 2020

Why do you insist on changing CloudEvent interface?
This has already been extensively debated in #198, #208, #211 without you providing any constructive counter argument that all can agree and get behind.
I am somewhat speechless. . .

For the record it's a staunch -1 vote.

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper slinkydeveloper changed the title [WIP] Introduce CloudEventData Introduce CloudEventData Oct 8, 2020
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@olegz
Copy link
Contributor

olegz commented Oct 8, 2020

Additional changes did not address the core problem which introduces unnecessary type dependency in CloudEvent strategy.
Also, many of us stated in previous WIP PRs on the same subject, the core strategy (interface) must be simple and concise. Any kind of specialization, any kind of additional dependency could be introduced in extension strategies, providing the core framework can still accept the core interface.

So definitive -1

@JemDay
Copy link
Contributor

JemDay commented Oct 21, 2020

If CloudEventData is the way I present the business data to the CloudEvent, how do I present a JsonValue or a ProtoMessage ? ... both JSON and Proto formats have the ability to carry the business data explicitly.

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 "data" property.

Apologies if I'm missing something..

@slinkydeveloper
Copy link
Member Author

@JemDay that's the idea of this PR: with CloudEventData we allow carrying JsonValue or ProtoMessage as data, so with a downcast to ProtoCloudEventData or JsonCloudEventData you can actually get the "concrete data" you want. How we implement that in serializers/deserializers is still TBD

@olegz
Copy link
Contributor

olegz commented Nov 9, 2020

@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?

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants