Streamline CloudEvent strategy to ensure it's clean from any assumption #143
Conversation
- Streamlined CloudEvent strategy to ensure it's clean from any assumptions on possible implementation details to ensure that CloudEvent type created outside of SDK is just as valid and operable as the one created by SDK. - Created CloudEventUtils as general utility class to contain set of static method to manipulate various aspects of CloudEvent. Factory methods that were previosuly in CloudEvent as well as other interafces were moved. Tests and other implementations were adjusted accordingly. - Cleaned up logic in Message and BinaryMessage related to 'can never happen` comment as well as fixing compilation errors these classes were generating in some IDEs. Signed-off-by: Oleg Zhurakousky <ozhurakousky@pivotal.io>
|
@slinkydeveloper sorry, I didn't pat attention to |
Can you be more specific about this one? Can I ask why you should create a |
|
@slinkydeveloper there are and will be |
|
Sure, it is directly related to what you and I discussed and agreed on slack last week. Basically |
| * @return the version of the Cloud Events specification which this event uses. | ||
| */ | ||
| Map<String, Object> getExtensions(); | ||
| SpecVersion getSpecVersion(); |
There was a problem hiding this comment.
I think CloudEvent should implement Attributes interface
There was a problem hiding this comment.
After this change, Attributes role becomes minor, and I can even see the interface being removed (there is no such "interface" in the spec and the attributes are defined next to other fields like data anyways)
Does it make sense to keep Attributes in CloudEvent type at all?
There was a problem hiding this comment.
Personally I tend to agree with Sergey, but would also like to have a separate discussion on that.
There was a problem hiding this comment.
in go we make the top level event implement the attributes and also an accessor to get the holder of attributes, if that was translated to this lib, it would be that CloudEvents extends Attributes and also has a getter getAttributes() Attributes
There was a problem hiding this comment.
@n3wscott it kind of contradicts your previous comment, but it could very well be me misinterpreting this comment for you simply stating how things are currently done in go.
| */ | ||
| String getType(); | ||
|
|
||
| BinaryMessage asBinaryMessage(); |
There was a problem hiding this comment.
I think these should not be removed, since they simplify a lot the integration between the event in memory representation and the "wire" representation, please revert these.
There was a problem hiding this comment.
These are utility operations that simply don't belong in the core strategy of CloudEvents. I simply moved them to CloudEventUtils class as you and I discussed last week.
| @Nullable | ||
| String getDataContentType(); | ||
|
|
||
| static io.cloudevents.v1.CloudEventBuilder buildV1() { |
There was a problem hiding this comment.
Maybe It would make sense to have a simple CloudEventBuilder interface, and move these static methods there more than in CloudEventsUtils
There was a problem hiding this comment.
Builder, Utils, Factory are all different concepts. In fact as Java developer when I see the class name that ends with Builder i look for builder pattern. *Utils and *Factory are different. We can certainly discuss it but in spirit of small and manageable/reviewable PRs that address one thing at the time I decided to only address things that are relevant to streamlining CE
There was a problem hiding this comment.
maybe worth considering "Encoders" (in addition to but separate from versioned "Builders", but also not as vague as "Utils") as the structured and binary formats are encoding modes in CloudEvents terminology, and an Encoder interface could be useful, as there will certainly be new encodings in the future.
| } | ||
|
|
||
| @Override | ||
| public void visitExtensions(BinaryMessageExtensionsVisitor visitor) throws MessageVisitException { |
There was a problem hiding this comment.
One of the core pattern of message/event exchanges is their immutability since events represent moment in history and thus can not be re-written. So giving user reference to a mutable map is very dangerous
| } | ||
|
|
||
| @SuppressWarnings("rawtypes") | ||
| public static BinaryMessageVisitorFactory defaultBinaryMessageVisitorFactory() { |
| /** | ||
| * The event context attributes | ||
| */ | ||
| Attributes getAttributes(); |
There was a problem hiding this comment.
If CloudEvent implements Attributes, then this method can still live here.
The reason to keep this method is to allow forward compatibility or backward compatibility with old/new attributes, accessing some specific "features" of a specific spec version
There was a problem hiding this comment.
Isn't casting to CloudEventsV1Impl would serve the same purpose?
There was a problem hiding this comment.
User of SDK can still downcast it to CloudEventImpl and we can certainly even make it public there for reason you just explained, but with regard to CloudEvent interface we should start looking at it as something that should never change.
There was a problem hiding this comment.
In go we had been casting to a version to access but it turns out that putting the accessors at the top level like you might in java style code and internally redirect to the correct attribute was pretty valuable for moving code forward in the cloudevent versions.
We choose to keep the accessor interface to be aligned with the most current version of the spec and swizzle internally to make older encodings look like new ones.
There was a problem hiding this comment.
@n3wscott I can get behind it and as you can see at the moment that is exactly what we're doing
I see, I don't disagree with this but I think you removed too much from the
|
|
@slinkydeveloper I believe I addressed all your points in individual comments, but let me summarize to match yours"
|
Well, since you're removing in this PR the entrypoint of the
The sdk needs an unstructured view of the event to simplify the serialization/deserialization implementation, and that's what
Since you're doing the change, please move the builders where they belong more than in a |
|
". . .This is a core feature of the sdk and of the CloudEvent contract. . .
With regard to The same with |
As I stated in the other comment, the serialize/deserialize process is implemented through the unstructured view aka
Again, I think it's definitely related to this PR the change to the builder, I don't see the reason why to move temporary to a Utils class to move them back in another PR...
The SDK atm is in active development for a new major release so we can definitely remove stuff, depending on the context. |
I disagree here. Systems like Debezium may represent the events they generate as
IMO "must" here is incorrect. As in my previous point, the
Because many other previous changes were done in multiple PRs, with "I will do it later" and TODOs? Keeping PRs small (given that v2 is still WIP) makes it much easier to review them and focus on a single change. |
Can you please argument this particular topic?
If you want to split in several PRs, then revert here the changes made to the |
|
With regard to ... CloudEvent type may be useful as an in-memory type..... Examples are plenty:
I can continue but that should be enough. With regard to "...CloudEvent are useful in the real world if they can be serialized/deserialized to bindings/event formats....". That is an opinion not a fact. Bindings being optional specification, on the other hand, is a fact. With regard to "...then revert here the changes made to the buildV1()...". Also ". . . SDK atm is in active development for a new major release so we can definitely remove stuff,. . ". - that is exactly what we're doing here, removing functionality that is not justified. Basically look at it this way; If this is indeed an active development, then let's apply one of the best principals of software design and pretend we have a clean sheet of paper and argue about what to put on it as opposed to put everything on it and argue about what to remove. |
This is an antipattern: the business logic of an application should not depend on
As you can see here, bindings are not optional to sdk development: https://github.com/cloudevents/spec/blob/master/SDK.md#technical-requirements
I'm sorry, but I don't see the reason to remove these methods in this PR and move in an Utils class. |
|
"...This is an antipattern: the business logic of an application should not depend on CloudEvent..." Anyway, would love to see what @jamesward @n3wscott and others think "...As you can see here, bindings are not optional. . ." |
Ok maybe I didn't explained myself clearly: Let's assume you want to create a CQRS application where commands comes from kafka as cloudevents. Your business logic should not depend from the fact that you received the command as CloudEvent: You should take the cloudevent and convert it into the domain specific command POJO. |
|
|
"...You should take the cloudevent and convert it into the domain specific command POJO...". "... CloudEvent is the main entry point of the sdk features and it's a primary goal to make as easy as possible the interaction between the event and the other sdk features....." |
I'm specifically speaking about
This is specifically covered by the builders: with the previous CQRS example, the eventual integration between my CQRS command POJOs and the kafka producer uses the builder to build a |
|
". . .I'm specifically speaking about toStructuredMessage and toBinaryMessage. . ." Creation of CloudEvents "...is specifically covered by the builders:... " |
Builders provides a structured way to build the event, MessageVisitor provides an unstructured way to build events |
|
Throughout the life of this PR no one was questioning builders or visitors. We are simply moving them out of |
|
Hello folks. Honestly I can't say if this is a better or worse change than what is in the code base at the moment because there is no agreement on the higher level vision for how the SDK should be architected. I think @fabiojose, @slinkydeveloper, @olegz and @bsideup (I would be happy to help this process too) need to pause and write some docs to understand what the goal in the v2 of the java SDK is. It helped for go to start with the UX you want to provide to the integrators, and it helped to think of that UX in terms of several personas: a middleware, a sdk extender (someone adding a protocol, perhaps not in-tree), a consumer (and their can be several of these, like http, FaaS, green field, legacy integrators... etc) If you do not see and agree about the north star goals of what we are trying to provide then these large refactors that add and remove features only hurt adoption because no one can use the code. So let's take a pause and have some meetings, write some docs, and then get to back work. It helped to start with a google doc and then turn that into markdown but you can do anything you want... maybe a hackmd would be better here. tl;dr: without a plan, we are lost. |
|
Can we discuss this on the SDK call tomorrow? |
|
I'm sorry, maybe i explained myself badly, but i explicitly reject the PR for several reasons:
I'm looking forward to a split of the PR as follows:
After the discussion we had last week, I thought about how we could provide an unstructured view of the cloudevent without putting this burden in the hands of the Feel free to close this one or open another one for the other changes |
|
@slinkydeveloper Let me address your individual points
I am certainly not closing this PR and urging you to reconsider or provide detail technical justification based on the original merits of this PR as to why it is invalid. |
fabiojose
left a comment
fabiojose
left a comment
There was a problem hiding this comment.
Despite the rich discussion, I want to register my point of view as a java programmer:
First, +1 for this PR. Let's keep a clear definition, it's better than an interface with a lot of methods.
- it really uncommon to have utility methods defined at interfaces. Interfaces define signatures, not behaviors (it's just a friendly remember :)
- fellows, this is java! What about use idiomatic Java?
- Let's maintain just the support for spec 1.0, it will be easiest to get things done
- I understand the wish to have the same API for every language, but this SDK will use by people who uses java.
- I agree with @n3wscott, we need to define a direction and together reach it
- I understand the nature of OSS collaboration, agreement, and disagreement are very common. But, let take the best of this PR and make our considerations.
| return this.attributes; | ||
| } | ||
|
|
||
| public CloudEvent toV03() { |
There was a problem hiding this comment.
This may be dropped away, one version will be easiest to maintain.
There was a problem hiding this comment.
It would be nice if instead of CloudEventImpl with version converting methods there could instead be V1CloudEvent as the "impl" and a CloudEventBuilder could support conversion if a V1CloudEvent is passed in but V03 is requested via something like builder.from(somev1CloudEvent).asV03().
| } | ||
|
|
||
| @SuppressWarnings("rawtypes") | ||
| public static BinaryMessageVisitorFactory defaultBinaryMessageVisitorFactory() { |
| return builder.build().toV1(); | ||
| } | ||
| switch (sv) { | ||
| case V03: |
There was a problem hiding this comment.
Let's drop out the v0.3, it is not required to support this version.
| } | ||
|
|
||
| @ParameterizedTest() | ||
| @MethodSource("io.cloudevents.test.Data#v03Events") |
There was a problem hiding this comment.
Let's drop out the support for spec 0.3
|
|
||
| assertThat(eventV03.getAttributes().getSpecVersion()) | ||
| assertThat(eventV03.getSpecVersion()) | ||
| .isEqualTo(SpecVersion.V03); |
There was a problem hiding this comment.
Let's drop out the support for spec 0.3
|
Hi @fabiojose , it would be amazing to have just support for one spec version because it could vastly over simplify the architectures of the sdks, but we can't:
|
The URL does not state that there is any hard requirement:
I agree. This is why it is important to have main interfaces spec-free and encapsulate every per-spec aspect into a dedicated subset of the SDK. But that's a separate discussion and does not belong to this PR. |
|
BTW for who didn't noticed, this is my alternative proposal: #146 |
True, but you can't ignore that you need to marshal/unmarshal event formats/protocol bindings supporting different spec versions, hence the need for an unstructured read/write of the event. Otherwise, each event format/protocol binding must handle by itself the different spec versions |
I prefer to deal with it in FIFO order, and, since this PR is not "out" yet (and it seems that the majority agrees on a positivity of this change), unless you have strong counter arguments based on facts, I recommend we deliver this PR first.
I am not arguing here. But it does not mean that there is only one approach (the current one) to the problem. |
I thought about that, but what I don't like is that |
The callee of
I think we need more here than just a personal taste.
I would not agree with it being definitely more complex. In fact, I think it is the opposite.
If it can be a default method (aka - the implementors are not required to provide anything to make it work), does it even belong to the interface then?
Was there a symmetry? byte[] serialized = EventFormatProvider
.getInstance()
.resolveFormat("application/json")
.serialize(event);byte[] serialized = event
.asStructuredMessage(format)
.visit(new KafkaSerializerMessageVisitorImpl(headers))byte[] serialized = data
.asBinaryMessage()
.visit(new KafkaSerializerMessageVisitorImpl(headers));CloudEvent event = EventFormatProvider.getInstance()
.resolveFormat(JsonFormat.CONTENT_TYPE)
.deserialize(bytes)MessageUtils.parseStructuredOrBinaryMessage(
() -> KafkaHeaders.getParsedKafkaHeader(headers, KafkaHeaders.CONTENT_TYPE),
format -> new GenericStructuredMessage(format, payload),
() -> KafkaHeaders.getParsedKafkaHeader(headers, KafkaHeaders.SPEC_VERSION),
sv -> new KafkaBinaryMessageImpl(sv, headers, payload),
UnknownEncodingMessage::new
).toEvent()I only see a variety of ways to serialize/deserialize CloudEvents. Some are using methods on the interface, some accept the event itself, some make use of the A true symmetry would be 3 types: |
|
While public support for this PR has already been expressed by members of java community representing several large organizations including organization behind the maintainers of this repository, as well as members representing other non-java communities as it was evident on the CNCF call we had last week (30/04/2020) we seem to remain at an impasse. So I am writing this up to primarily raise awareness of such impasse and to provide a summary, so we as a community can find a path forward. Non of the engineers behind this PR ever questioned or argued the fact that SDK must:
These and more are the responsibility of the Cloud Events SDK, period. But SDKs consist of many moving parts designed to collaborate with one another and in relation to CloudEvents Java SDK this PR is addressing one such moving part, and that is The emphasis here is on Java Interface, which means we need to take into consideration certain design principles that are widely followed in Java community as we would be expected to do the same for communities representing other languages and runtime environments And while there are many that could be applied here, the core design principles that are questioned by the opposition to this PR are Single Responsibility and Interface Segregation. So what this PR does is the following:
There is nothing else in this PR. It does not remove any features and it does not break any test. So, we are looking for suggestions from senior members of the greater Cloud Events community on how to move forward, so tagging a few that I know, but please feel free to share it @clemensv, @duglin @jamesward @n3wscott |
|
While the limited scope of this PR is outlined in the last comment, we do realize that there are still several points of contention here. However, by looking at some of the individual comments on these contentious points, it also appears that we are closer to an agreement than we think (if we look at each of them separately). So, here is the outline of the main points of contention:
To address these point we are proposing the withdrawal of this PR in favor of 3 independent PRs corresponding to the 3 following issues which address the 3 points of contention outlined above.
|
|
Next week I'll try to sum up a proposal with all the interfaces in a separate temporary module. I would say we should try to find an agreement on that, so then we can then plumb the interfaces with the actual implementation of all sdk pieces |
|
Here it is: #155 |
|
Thank you Francesco! While we may continue our debate about value of visitors in general, we can now do it some other time ;) Exactly what we've been advocating for. Clean, concise and free of any assumptions. Great to be moving forward! |
|
@olegz if you agree with that, please state explicitly in that pr so we can merge and move forward. I'm gonna do 2/3 prs in the next week to re-glue things together |
|
@slinkydeveloper i certainly will, give me till tomorrow morning. Getting late here ;) |
|
The core concerns expressed in this PR are now addressed in #155, as changes proposed by this PR are included in #155.
Rejecting and closing. |
7 participants
type created outside of SDK is just as valid and operable as the one created by SDK.
methods that were previosuly in CloudEvent as well as other interafces were moved. Tests and other implementations were adjusted accordingly.
Signed-off-by: Oleg Zhurakousky ozhurakousky@pivotal.io