Rework the Event interface #108

slinkydeveloper · 2020-03-23T09:25:02Z

Hi,
I wish to propose a rework of the Event interface and related. I've tried to use this sdk in a middleware experiment (https://github.com/slinkydeveloper/cloud-streaming-data-plane) and I've found several issues with the actual Event interface.

I think the problems from a middleware point of view are:

I don't care about the attributes version and I don't know it in advance, I just want to check if the event is valid and route it.
I don't need to map the payload of the event, it's an expensive operation for me.

While from an "end user" perspective (who reads and writes events from json or from http):

I usually don't care about the attributes version and I don't know it in advance, I just want to access to the basic metadata (id, type, time, etc). When I map back and forth to json I usually have no clue what is the attributes version
Because of the "polyglot" aspect of the data field, I don't want to handle the difference between base64 and json serialized. I just want to get the data in the way I want (as json or as binary array)

From the experience of working on sdk-go and sdk-rust I've tried to bake a modified Event interface, let me know your thoughts.

Signed-off-by: Francesco Guardiani [email protected]

Signed-off-by: Francesco Guardiani <[email protected]>

matzew · 2020-03-23T09:29:38Z

/lgtm

cc @fabiojose @jponge

slinkydeveloper · 2020-03-23T09:33:50Z

Although the languages are really different, I want to point how the structure is designed in sdk-rust:

fabiojose · 2020-04-15T12:49:42Z

@slinkydeveloper remember to signoff the commits

fabiojose · 2020-04-15T12:51:45Z

I would like to propose to remove any external dep, like Jackson and Bean Validations.

With that, CloudEvents API will work with any framework without issues.

It will be nice if we have an API module with just the definitions, them a JSON module with Jackson serialization, KAFKA, AVRO, protobuf, http and so on...

You know what I mean, modules for Event Formats and modules for Transport Binding.

When someone wants to use the API, add just this dependency. If they want JSON, they add the json-jackson module, if they want http they add http-vertx module and son on...

fabiojose · 2020-04-15T12:52:41Z

Although the languages are really different, I want to point how the structure is designed in sdk-rust:

* [Event](https://github.com/cloudevents/sdk-rust/blob/master/src/event/event.rs)

* [Data union type](https://github.com/cloudevents/sdk-rust/blob/master/src/event/data.rs)

* [Attributes union type](https://github.com/cloudevents/sdk-rust/blob/master/src/event/attributes.rs)

* [Extension value union type](https://github.com/cloudevents/sdk-rust/blob/master/src/event/extensions.rs)

@slinkydeveloper is really hard to create the same structure in every SDK.

fabiojose · 2020-04-15T12:54:54Z

Go ahead! it's looking nice!

slinkydeveloper · 2020-04-15T12:58:13Z

Ok I'll continue down that road, but be aware that this means reworking most of the sdk. Plus I'm going to drop 0.2, because of its weird extension things (and because according to this document we don't need it https://github.com/cloudevents/spec/blob/master/SDK.md#contribution-acceptance

fabiojose · 2020-04-15T13:19:39Z

@slinkydeveloper consider these issues #111, #112 , #113 , #114 , #115 . This PR would be the start of really good modularization.

slinkydeveloper · 2020-04-15T13:36:33Z

@fabiojose since those changes are radical, i think we need a v2 version of the sdk for this. Do you agree on branching the current master creating a 1.x branch?

Signed-off-by: Francesco Guardiani <[email protected]>

…uages like Java has method overloading Signed-off-by: Francesco Guardiani <[email protected]>

Signed-off-by: Francesco Guardiani <[email protected]>

n3wscott · 2020-04-16T15:01:21Z

api/src/main/java/io/cloudevents/CloudEvent.java

 */
-public interface CloudEvent<A extends Attributes, T> {
+public interface CloudEvent {


yes this is a good change. too many generics makes this really hard to use.

n3wscott · 2020-04-16T15:03:47Z

api/src/main/java/io/cloudevents/format/BinaryUnmarshaller.java

@@ -133,9 +133,9 @@ private BinaryUnmarshaller() {}
 		 */
 		HeadersStep<A, T, P> builder(EventBuilder<T, A> builder);
 	}
-
+
 	private static final class Builder<A extends Attributes, T, P> implements


this still has the layer cake of generics

This still needs to be removed 😄

n3wscott · 2020-04-16T15:07:04Z

api/src/main/java/io/cloudevents/json/CloudEventSerializer.java

+
+import java.io.IOException;
+
+public class CloudEventSerializer extends StdSerializer<CloudEvent> {


nice. nice nice.

n3wscott · 2020-04-16T15:07:47Z

api/src/main/java/io/cloudevents/json/Json.java

        module.addSerializer(ZonedDateTime.class, new ZonedDateTimeSerializer());
        module.addDeserializer(ZonedDateTime.class, new ZonedDateTimeDeserializer());
        MAPPER.registerModule(module);
    }

+    protected static final DateTimeFormatter RFC3339_DATE_FORMAT = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ssXXX");


for what it's worth, the go sdk uses the RFC3339 Nano format... but they both work together.

n3wscott · 2020-04-16T15:08:47Z

api/src/main/java/io/cloudevents/json/Json.java

        module.addSerializer(ZonedDateTime.class, new ZonedDateTimeSerializer());
        module.addDeserializer(ZonedDateTime.class, new ZonedDateTimeDeserializer());
        MAPPER.registerModule(module);
    }

+    protected static final DateTimeFormatter RFC3339_DATE_FORMAT = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ssXXX");
+
+    public static String encode(final CloudEvent event) throws IllegalStateException {


encode == structured mode?

So these are marshal/unmarshal methods for Cloudevent to go back and forth to JsonValue/String encoded json/binary encoded json

n3wscott · 2020-04-16T15:15:12Z

Here is my little demo of using the current java sdk in a spring app and it was quite painful:

https://github.com/n3wscott/spring-ce-demo/blob/master/src/main/java/com/n3wscott/demo/DemoApplication.java

I would love to see the amount of work I need to do be reduced. This PR is starting in that direction, Thanks @slinkydeveloper !!

Signed-off-by: Francesco Guardiani <[email protected]>

api/src/main/java/io/cloudevents/CloudEvent.java

api/src/main/java/io/cloudevents/SpecVersion.java

api/src/main/java/io/cloudevents/extensions/DistributedTracingExtension.java

bsideup · 2020-04-20T07:32:08Z

api/src/main/java/io/cloudevents/extensions/ExtensionsParser.java

+public final class ExtensionsParser {
+
+    private static class SingletonContainer {
+        private final static ExtensionsParser INSTANCE = new ExtensionsParser();


ExtensionsParser as enum with a single INSTANCE value would work as well

TBH I prefer this way, the singletons with enums sounds too weird to me 😄

Actually... is it possible to avoid singletons for extensions by passing an extension parser to the event parser

It would be nice to be able to provide non-static extension parsers (e.g. by injecting with some DI framework)

I think as a first pass is fine, we'll improve this part later. For now i just made the bare minimum to make it working, since it's my priority 😄.

Signed-off-by: Francesco Guardiani <[email protected]>

bsideup

FTR Although I really appreciate @slinkydeveloper's work on this, and there are some great things in it, I think this PR is too big to be accepted.

There wasn't enough (public?) discussions on issues like this one: Modularization: api module #111
It clearly can be separated into a few smaller PRs (e.g. Remove Attributes interface #118)
Some decisions IMO require more discussions, and not as PR comments, but as separate issues (e.g. the use of Optional).

As was stated, it is planned to create 2.x version of the SDK, and it is a great chance to rethink and fix the issues of 1.x. A massive PR is not the best approach to handle it if you ask me :)

olegz · 2020-04-21T09:16:20Z

I have a more general question. Why do we need to make *-api module bringing so many dependency that are not really relevant to the task in hand. What is the task in hand? IMHO - to define strategies to support the specification. We should be free to write parsers, readers, writers, validators etc., if we choose to without bringing unnecessary dependencies which may actually create conflict. Also see - #111

At it's current state cloudevents-api-1.0.3 already brings the following dependencies

jackson-core-2.10.1.jar
jackson-databind-2.10.1.jar
jackson-annotations-2.10.1.jar
jackson-datatype-jdk8-2.10.1.jar
hibernate-validator-6.0.17.Final.jar
validation-api-2.0.1.Final.jar
jboss-logging-3.3.2.Final.jar
classmate-1.3.4.jar
jakarta.el-3.0.3.jar

Why. . .?
There is a specification which essentially defines a contract and that should be enough for anyone to consume/produce CloudEvent types and that is all that such module should contain.
Sure, as a community we can collaborate on implementation for dealing with these events (e.g., marshalers, unmarshalers, etc. . ), but that should be a completely separate effort... may be even several efforts given the infancy of the current API.

We certainly don't wan to get into arguments such as ". . .this marshaller is more performant than that,. . . Gson vs Jackson etc. . ., "

bsideup · 2020-04-21T09:17:58Z

@olegz FYI this PR seems to remove the dependencies from -api:
#108 (comment)

slinkydeveloper · 2020-04-21T10:17:50Z

There wasn't enough (public?) discussions on issues like this one:

This discussion started the 27 Feb 2020 in sdk meetings (https://docs.google.com/document/d/1JIZxnV_w-dMrSAguG4bqog1vBlD94mm5aFjJlfb-R34/edit?usp=sharing) but nobody had counter-arguments again a big refactor of the sdk through a pr like this.

As I said, I don't like proceeding this way and I'm ready to engage further discussions (like Optionals and @Nullables). This is just a first pass, to clean up some stuff we don't need any more because of 0.2 and, most important, to align the APIs to other SDKs, bringing the Event/Message levels to provide on one side easy to use apis to read/write events and, on the other side, a simple api to marshal/unmarshal events back and forth the wire.

It's not perfect: on the contrary it needs a lot of work and there are no definitive decisions, but we need to start somewhere. Follow-up PRs will improve the work done here and, i underline, i'm really happy to engage the community to discuss these and future improvements.

slinkydeveloper · 2020-04-21T10:21:27Z

I'm going to also open a Github project with things we want to have, to discuss and to improve for sdk v2

bsideup · 2020-04-21T10:29:51Z

@slinkydeveloper

This discussion started the 27 Feb 2020 in sdk meetings (https://docs.google.com/document/d/1JIZxnV_w-dMrSAguG4bqog1vBlD94mm5aFjJlfb-R34/edit?usp=sharing) but nobody had counter-arguments again a big refactor of the sdk through a pr like this.

I am sure there were some discussions, but it would be better if they were more asynchronous (e.g. on GitHub issues) because not everyone can join the call and have the answers ready :)

It's not perfect: on the contrary it needs a lot of work and there are no definitive decisions, but we need to start somewhere.

I hope my comments did not sound attacking - that's a great change and I am glad to see it happening! Just wanted to mention (as a feedback) that, as an external collaborator, I find the way the SDK gets such changes a bit closed group-ed.

I'm going to also open a Github project with things we want to have, to discuss and to improve for sdk v2

That's a great idea! It will definitely help the visibility of both the current and upcoming states 👍

slinkydeveloper · 2020-04-21T11:26:25Z

@bsideup Everything that relates to extensions read/write for now is just done to be the bare minimum compilable, I've opened this issue to discuss apis about it #122

Signed-off-by: Francesco Guardiani <[email protected]>

Json roundtrip works Added extensions to tests Signed-off-by: Francesco Guardiani <[email protected]>

slinkydeveloper · 2020-04-21T16:47:49Z

I removed the WIP flag, for me this PR is ready to merge.

Signed-off-by: Francesco Guardiani <[email protected]>

fabiojose · 2020-04-22T12:09:54Z

LGTM

Proposal to refactor the Event class

5362a80

Signed-off-by: Francesco Guardiani <[email protected]>

slinkydeveloper added the enhancement New feature or request label Mar 23, 2020

di mentioned this pull request Apr 2, 2020

Consider a more Pythonic API cloudevents/sdk-python#26

Closed

slinkydeveloper added 4 commits April 16, 2020 11:41

Some steps forward

670d1c1

Signed-off-by: Francesco Guardiani <[email protected]>

After writing for months in a toy language, I forgot that modern lang…

0113207

…uages like Java has method overloading Signed-off-by: Francesco Guardiani <[email protected]>

Extensions

ce98e7c

Signed-off-by: Francesco Guardiani <[email protected]>

Sketching Message APIs

59d4a79

Signed-off-by: Francesco Guardiani <[email protected]>

n3wscott reviewed Apr 16, 2020

View reviewed changes

WIP

ad5ab5d

Signed-off-by: Francesco Guardiani <[email protected]>