Plan for glue-ing core to api

[slinkydeveloper]

1. Remove toBinaryMessage & toStructuredMessage implementing the CloudEventVisitor/CloudEventVisitable: Re: Proposal for reworking the unstructured read/write of events #150. This allows all protocol bindings/event formats to continue working
2. Remove spec version conversion (toV1() and toV03()) from CloudEvent interface: Reimplemented Spec version conversion #147
3. Flatten attributes implementation (this is not required, however it simplifies the code and reduce allocations): WIP from Flatten attributes interface/implementation #148
4. Remove assumption that CloudEvent implements CloudEventVisitable: This PR will move the code under the default implementation of CloudEventVisitable under a generic CloudEventVisitable implementation for CloudEvent interface (placed in cloudevents-core)
5. cloudevents-core depends on cloudevents-api

All PRs from 1 to 4 will change the CloudEvent interface (and friends) inside core in order to make it equal to the one inside api. After this is done, it will be trivial to remove the old interfaces in core and replace them with the ones in api.

[duglin]

thank you @slinkydeveloper
All, I wonder if we could use this issue to help flesh-out some of the work that Francesco sees in-front of us. Obviously discussing the overall order of these high-level tasks is important, but if any of them need further elaboration, or splitting up, let's discuss that here and perhaps sub-bullets might be needed.

[bsideup]

@slinkydeveloper thanks for posting the roadmap! Looking forward to the result 👍

As an alternative option to speed up the migration, you could also rename the current interface (to something like LegacyCloudEvent and make CloudEventV1Impl-like classes implement both, so that the SNAPSHOTs can already be used (especially by projects that only need -api). Just my 2c :)

[slinkydeveloper]

you could also rename the current interface

But then i need to change two times all the names 😄 hopefully this transition will be fast and painless, most of the code is already there. What I'm still missing is point 4, but that should be trivial given that i already have a generic code for it.

[slinkydeveloper]

I could collapse points 1 and 4 point into a single one, but #150 is already pretty big

[olegz]

I think we got our priorities in the wrong order. We're still talking about #150, #147 and #148 which were created long before the resent change to CloudEvent interface and introduction of API module. This means that all those 3 PRs are build on outdated assumptions and thus have rendered themselves obsolete. We need to revisit the core goals those PRs are trying to address with a fresh look and a stable project which as of right now we do not have and that should be priority number 1.
This means; Before we do anything we need to stabilize the workspace with bare minimum changes.
Which is:

1. Remove CloudEvent interface from the core
2. Introduce cloudevents-api dependency to the cloudevents-core and fix what needs to be fixed. Should be no more then 20-30 files to change. This will give us clean build as well as clean view into how recent changes affected the assumptions and views we had before.

Then we should create individual issues that are now based on the analysis and knowledge obtained from performing such merge.

[duglin]

would it help to see the list of classes and interfaces that would make up the 20-30 file change? Just a list of the proposed "first lines" of each - e.g.

public interface CloudEvent extends ....

so we can see how things extend/implement each other and which would not be part of the core object model but would instead be part of some util class (if any) - just so we can see if there's agreement on the overall structure of the change before someone spends time on a PR that might not have consensus.

[olegz]

Sure, here is the list of files requiring changes

modified:   core/pom.xml
deleted:    core/src/main/java/io/cloudevents/CloudEventRemove.java
modified:   core/src/main/java/io/cloudevents/Extension.java
modified:   core/src/main/java/io/cloudevents/extensions/DistributedTracingExtension.java
modified:   core/src/main/java/io/cloudevents/impl/BaseCloudEventBuilder.java
modified:   core/src/main/java/io/cloudevents/impl/CloudEventImpl.java
modified:   core/src/main/java/io/cloudevents/message/BinaryMessage.java
modified:   core/src/main/java/io/cloudevents/message/Message.java
modified:   core/src/main/java/io/cloudevents/v03/CloudEventBuilder.java
modified:   core/src/main/java/io/cloudevents/v1/CloudEventBuilder.java
modified:   core/src/test/java/io/cloudevents/extensions/DistributedTracingExtensionTest.java
modified:   core/src/test/java/io/cloudevents/impl/CloudEventImplTest.java
modified:   core/src/test/java/io/cloudevents/message/EventMessageRoundtripTest.java
modified:   core/src/test/java/io/cloudevents/mock/CSVFormat.java
modified:   core/src/test/java/io/cloudevents/test/Data.java
modified:   core/src/test/java/io/cloudevents/v03/CloudEventBuilderTest.java
modified:   core/src/test/java/io/cloudevents/v1/CloudEventBuilderTest.java
modified:   formats/json-jackson/src/main/java/io/cloudevents/format/json/CloudEventSerializer.java
modified:   http/restful-ws/src/main/java/io/cloudevents/http/restful/ws/CloudEventsProvider.java
modified:   http/vertx/src/test/java/io/cloudevents/http/vertx/VertxHttpClientRequestMessageVisitorTest.java
modified:   http/vertx/src/test/java/io/cloudevents/http/vertx/VertxHttpServerResponseMessageVisitorTest.java
modified:   kafka/src/main/java/io/cloudevents/kafka/CloudEventSerializer.java
modified:   kafka/src/test/java/io/cloudevents/kafka/CloudEventDeserializerTest.java
modified:   kafka/src/test/java/io/cloudevents/kafka/CloudEventMessageDeserializerTest.java
modified:   kafka/src/test/java/io/cloudevents/kafka/CloudEventMessageSerializerTest.java
modified:   kafka/src/test/java/io/cloudevents/kafka/KafkaProducerMessageVisitorTest.java

For more details you can also look at my branch where i have made these changes and have a stable build now - https://github.com/olegz/sdk-java/tree/realign

I can issue a PR if that would help

[duglin]

I can't speak to whether the steps listed in the first comment are in the right order, or whether they're the minimal set, but I do have a question... from my reading of the comments it appears that there is agreement that at the end of this process we should:
1 - Not have the CloudEvent interface in 'core'
2 - 'api' should have the only definition of the CloudEvent interface ('api' might have other interfaces defined, but I'm just focusing on the CloudEvent one)
3 - 'core' should depend on 'api'

Is that correct?

[bsideup]

@duglin sounds good!

[duglin]

If there's agreement on those 3 points, I'm wondering if we can split this into pieces and first focus on getting to those 3 end-goals... regardless of how we get there. Meaning, I believe that Pivotal's main concern is that the 'api' layer remain clean, so we can focus on retaining that while also ensuring 'core' properly uses 'api' (e.g. doesn't duplicate the CE interface).

The 2nd part would then be to discuss the 'core' layer. While we may not have 100% agreement on the exact steps outlined by @slinkydeveloper's in the first comment, it should lay the foundation by cleaning up the code base and getting us out of the WIP stage we're in now. Then, since I'm pretty sure we don't have agreement yet on the design of 'core', we can at least then discuss any possible redesigns w/o worrying about 'api' and w/o mixing up any 'clean-up' steps that need to happen right now.

I realize this means that Pivotal will probably not use 'core' in the near term, but I'm hoping they'll continue to help in any redesign so they can use it in the not too distant future. But in the meantime, this should free the 'api' layer from any lingering clean-up items so they can at least use that safely.

Thoughts?

[olegz]

Yes, the 1, 2, 3 steps you (@duglin) outlined is the overall goal to which we should get to as quick as possible so we can have a stable build.
Everything else such as what's been addressed in other PRs, are separate concerns which must not be mixed into the 1, 2, 3 steps.

We can not have these large PRs that address many different concerns.

• They are nearly impossible to properly review
• Their scope is not clear because of their size, a symptom which generally implies more then one concern in a single PR
• They leave too many TODO for later
• They don't provide a clear path for reverting if later down the road you realize you made a mistake.
• They typically lack proper testing
• They typically lack proper documentation, especially javadoc