Re: Proposal for reworking the unstructured read/write of events

[slinkydeveloper]

This proposal comes from discussions in #143 and https://docs.google.com/document/d/1VY85Afhb-oj7m9Sm84U_f8kJp6boBS8nnuTMc9tkgO8/edit?usp=sharing

The goal of these changes is to provide an unstructured view of the event and an unstructured manner to write the event, without giving additional burden to the CloudEvent implementer (like asStructuredMessage() and asBinaryMessage() does). It's not a goal of this PR to address the other concerns of #143.

CloudEventVisitable

CloudEvent implements CloudEventVisitable, which is an interface that defines how an unstructured visit of the message happens. CloudEvent provides a default implementation for it, avoiding to put this burden to the CloudEvent implementer.

CloudEventVisitable is used by EventFormat implementations and protocol binding implementations, keeping low the allocation cost for encoding/decoding an event.

Now users can write binary messages using:

event
  .visit(VertxHttpServerResponseMessageVisitor.create(req.response()));

While they can write structured message using:

GenericStructuredMessage.fromEvent(event)
  .visit(VertxHttpServerResponseMessageVisitor.create(req.response()));

Two helper methods are also provided for that.

CloudEventBuilder and CloudEventVisitor/CloudEventVisitorFactory

CloudEventBuilder is the "write side" of CloudEvent. Concrete implementations provides "structured write" methods to write the CloudEvent (withId, withSource, etc) and implements the CloudEventVisitor<CloudEvent> interface, which provides "unstructured write" methods to create an event starting from a binary message.

Event conversion

The visitAttributes will be used as basis to reimplement specversion conversion, in order to remove the methods on the CloudEvent interface. This is shown in #147

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

[olegz]

As evident from the discussion here this PR together with #143 is no longer valid and should be rejected.

[slinkydeveloper]

I'm gonna merge this after the #155 as explained here #155 (comment)

[olegz]

I can't see how you can merge this PR, since it implies that CloudEvent is also a visitor. Of course with the changes proposed in #155 things would need t be reworked and that is a whole new PR that has no relation to this one.
So -1.
Let's close it and address one issue at a time.

[slinkydeveloper]

I can't see how you can merge this PR, since it implies that CloudEvent is also a visitor. Of course with the changes proposed in #155 things would need t be reworked and that is a whole new PR that has no relation to this one.

This PR won't affect the api module, and in any case i need to rework the core module to glue it to api module. Merging this PR just simplifies my job with this reworking and the APIs proposed here will vanish. So please let's just think about reaching the agreement on #155 and then as mantainer i'll figure out what's the best strategy to go forward. I intend to keep from this PR the implementation details, not the APIs

[olegz]

The agreement on #155 is reached.
My above comment is specific to this PR and it still stands valid. The #155 effectively invalidated both #143 and #155. I've closed #143.

[slinkydeveloper]

The agreement on #155 is reached.

Can you explicitly state it on #155 approving the PR?

[slinkydeveloper]

Ok this one is rebased on top of #155 and ready to be merged, it still doesn't use the new apis (followup prs will take care of it).

Let me know if there are any concerns about it.

[olegz]

With 101 files changed this is hardly a reviewable PR. We just spent a month debating PR with only 20 changed files. As suggested earlier, please close this PR in favor of manageable and reviewable units properly documented, tested etc.

[slinkydeveloper]

Most of the changes are due to the rebase commit, I can't split this PR because, as you know, it changes quite some crucial bits in the old interfaces. I want to merge this before re-gluing all things together, because i feel this is the more simple way to solve it and proceed further. I know it's hard, but please try to review it this way.

[slinkydeveloper]

I would love to try to import from this pr the new interfaces, but i don't guarantee any success, it will end up for sure in some classpath conflict... Lemme try it

[slinkydeveloper]

Nope, there are too much conflicts without at least putting together changes from this, #147 and and #148.

[olegz]

I think we need to step back.
Trying to fit/rework previous work into something for which it was not initially designed for will not work.
What is the end goal? Is it to merge 147, 148 and 150 at any cost? I doubt it's the direction we want to go.

I would suggest shifting our energy on building on the foundation that was established with #155, one bit at the time with proper reviews, testing, documentation etc.