-
Notifications
You must be signed in to change notification settings - Fork 167
Initial introduction of spring support and samples #293
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
Conversation
This PR is the result of collaboration between Dave Syer(@dsyer) and Oleg Zhurakousky(@olegz) of Spring team (VMWARE). It provides initial set of utility classes and interfaces to simplify integration of Cloud Events across many Spring projects Signed-off-by: Oleg Zhurakousky <ozhurakousky@vmware.com> Removed maven wrappers and other
slinkydeveloper
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.
Just did a quick pass, I'll do a proper review on monday
| String payloadWithHttpPrefix = "{\n" + | ||
| " \"ce-specversion\" : \"1.0\",\n" + | ||
| " \"ce-type\" : \"org.springframework\",\n" + | ||
| " \"ce-source\" : \"https://spring.io/\",\n" + | ||
| " \"ce-id\" : \"A234-1234-1234\",\n" + | ||
| " \"ce-datacontenttype\" : \"application/json\",\n" + | ||
| " \"ce-data\" : {\n" + | ||
| " \"version\" : \"1.0\",\n" + | ||
| " \"releaseName\" : \"Spring Framework\",\n" + | ||
| " \"releaseDate\" : \"24-03-2004\"\n" + | ||
| " }\n" + | ||
| "}"; |
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.
More than hardcoding these, how about putting them in test resources?
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.
Generally I like it that way for tests since it keeps everything in single place. That said I am ok to moving it if you feel strongly about it.
| @@ -0,0 +1,12 @@ | |||
| ## Spring Support | |||
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.
Can you add the badge and the pom dependency like the other modules? https://github.com/cloudevents/sdk-java/blob/master/kafka/README.md
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-dependencies</artifactId> | ||
| <version>2.4.0</version> |
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.
Can you change this to a property?
| </parent> | ||
|
|
||
| <artifactId>cloudevents-spring</artifactId> | ||
| <name>CloudEvents - support for Spring</name> |
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.
I think CloudEvents - Spring is fine
| <dependency> | ||
| <groupId>org.springframework.cloud</groupId> | ||
| <artifactId>spring-cloud-function-core</artifactId> | ||
| <version>3.1.0-SNAPSHOT</version> |
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.
Ah ok so the snapshot is because of what you sad in the comments? Is it possible to merge the support in spring-cloud-function-core before merging this one, so we can point out to a proper release? I would love to avoid merging a module depending on snapshots
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.
Also please make this version a property too
| <repository> | ||
| <id>spring-releases</id> | ||
| <name>Spring Releases</name> | ||
| <url>https://repo.spring.io/release</url> | ||
| <snapshots> | ||
| <enabled>false</enabled> | ||
| </snapshots> | ||
| </repository> |
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.
Aren't these published in maven central too?
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.
yes
slinkydeveloper
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.
Don't you convert Message <-> CloudEvent? Why? I might miss some context here
|
+1 on this idea. Once merged I'd like to add a sample using CloudEvents with Spring Cloud Function and the Solace Spring Cloud Stream Binder as a way to exchange Cloud Events over Solace Event Brokers. |
@slinkydeveloper Sorry for the delayed response as this one required a bit more details, so here it is. . . In SDK That said, you question is still valid in the context of interoperability and integration of Spring and non-Spring based systems (which includes Spring frameworks that do not rely on Spring Messaging) at the SDK level and for that we would need to provide something along the lines of MessageConverter (standard Spring mechanism for Message type conversion) to do the to/from So while this is an initial PR providing integration with several framework from the Spring portfolio, it is hopefully not the last one. |
|
Thanks for the explanation, I have a couple of comments:
Ok I'm trying to look at the examples to grasp how this integration looks like: https://github.com/cloudevents/sdk-java/pull/293/files#diff-3650ef1172cc81f44f7ba479f7399abf655d6ac8b4b88c4f3865652b07c969afR55-R67 <- here what if the user wants to send a custom
I wonder if we should place exactly this inside the sdk, to cover the use case above underlined (if i understood properly how spring works), more than the code provided in this PR, at least as first step. The integrations in this sdk provide the conversion X <-> CloudEvent, in order to allow people to integrate several systems using CloudEvent. That's the major goal of these integrations: then, for each integration, we might add on top the "niceties" to better integrate with the framework (eg in Kafka we provide a Last but not least, I might miss something, but in this PR I don't see support nor for structured mode nor for extensions. Our integrations should be 100% compatible and compliant with the CloudEvent spec, so we cannot neglect these 2 features |
Structured mode is definitely covered (e.g. see the samples - the HTTP one has both binary and structured very explicitly, and the Cloud Function samples support both as well). The presence of the constant
Yes, we looked at that, but it's the wrong level of abstraction really. Spring already handles all those concerns for HTTP, Kafka, etc. So we only really need the CE domain for the attributes (and probably extensions as you point out). I have an alternative implementation that ships most of the CE domain concerns back to the core SDK:
That much is clear. But it isn't a very useful model for the majority of Spring applications, mainly (I think) because Note that if |
I'm sorry I didn't saw it! Do you mean this one? https://github.com/cloudevents/sdk-java/pull/293/files#diff-cdd5264380e57fcd33464c1b7992c9d64b37c6b8ca62299fdc154a00cfb9018bR54-R71
I guess we could easily provide that creating a
Yes this is kinda a tricky corner case of the spec: extensions supports the same type system of the attributes, but they don't carry the type information with them when they're converted back and forth to protocol binding/event formats (e.g. think about http when the extensions are sent as headers). So, as a general form of abstraction, string/number/boolean is what we support today (which maps more or less to json extension types). But that part of the sdk has definitely room for improvements.
So if I understood your point correctly, you're saying that the sdk should not deal with payloads, which are usually handled by other abstractions (like Spring's
You can in fact: you could or implement your own CloudEventData which has a method like |
That is one of the example using spring-webmvc. No, you don't need to write a different handler between the structured and binary. The examples simply show variety of signatures you can use in thte context of a particular execution and have no bearings on the structure of the incoming event as you can see in this example.
In Spring, Also, as you said yourself several month ago:
Spring |
This PR is the result of collaboration between Dave Syer(@dsyer ) and Oleg Zhurakousky(@olegz ) of Spring team (VMWARE).
It provides initial set of utility classes and interfaces to simplify integration of Cloud Events across many Spring projects especially those that are based on Spring Messaging.
It also provides an initial set of examples to demonstrate several web solutions (i.e., spring-cloud-function-web, spring-webmvc, spring-webflux) as well as streaming solution such as Kafka (but not limited to given an existing Spring binding support for many messaging brokers. More samples will come once the PR is accepted.
IMPORTANT:
At the moment there is an experimental support for Cloud Event in Spring Cloud Function which was recently released with version 3.1.0-M5.
However, there is also a cloudevents branch in Spring Cloud Function, which removes that support and instead relies on Java SDK - this PR.
So in a way this PR (to work properly) is contingent upon ‘cloudevents’ branch of Spring Cloud Function. That said, once this PR is merged the ‘cloudevents’ branch will also be merged to master.
Signed-off-by: Oleg Zhurakousky ozhurakousky@vmware.com