Skip to content

Code and examples for proposed changes/additions to transport APIs#164

Merged
n3wscott merged 1 commit into
cloudevents:masterfrom
alanconway:inter-transport
Aug 19, 2019
Merged

Code and examples for proposed changes/additions to transport APIs#164
n3wscott merged 1 commit into
cloudevents:masterfrom
alanconway:inter-transport

Conversation

@alanconway
Copy link
Copy Markdown
Contributor

@alanconway alanconway commented Jul 16, 2019

See Issu "Message interface for inter-transport use." #162

This PR is not intended to be pushed, it illustrates some ideas directly as working code and examples. As we agree on if/how the ideas should be integrated, I'll submit PRs to merge them into the main transport package.

It is a little easier to read as godoc, for convenience I put it here https://alanconway.github.io/godoc/pkg/github.com/cloudevents/sdk-go/pkg/cloudevents/transport/x/index.html
or you can use godoc to generate it locally

@alanconway
Copy link
Copy Markdown
Contributor Author

This is a little easier to read as godoc, for convenience I generated it here https://alanconway.github.io/godoc/pkg/github.com/cloudevents/sdk-go/pkg/cloudevents/transport/x/index.html
or you can use godoc to generate it locally

@alanconway
Copy link
Copy Markdown
Contributor Author

@n3wscott ping

@n3wscott
Copy link
Copy Markdown
Member

Oh! I was traveling. Sorry I missed this.

Let me look.

// Fast case: if the Message is already structured JSON we can
// send it directly, no need to decode and re-encode. Encoding a
// json.RawMessage to a json.Encoder() is just a byte-buffer copy.
return s.Encode(json.RawMessage(b))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no impl for Encode. I am confused why you would need to encode message. In the current SDK, message is created by passing an event through a codec.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Encode here is the standard json.Encoder.Encode() - the naming is confusing.

The new Message is an abstraction that can be passed between transports, so a Sender may be sending a Message that was encoded by a foreign Receiver. Message advertises if it already contains a structured encoding that the sender can potentially re-use with no re-encoding in the optimized case, but otherwise the sender will call Event() to extract (and possibly decode using some foreign codec) the Event and re-encode it using its own Codec. I didn't implement an explicit Codec in the example, I just used json calls directly - I probably should for clarity.

Codec still has role in creating the internal message format for each transport, and may be used directly by users that just want to create e.g. a HTTP message and then use their own client or server code. However for communicating from a Receiver to a Sender, the relevant codecs are used internally by each one, and the Message provides a uniform way to decode the message without knowing what Codec was used to create it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, ok so Message has to have some state in it to define in which transport the message is encoded so it can select the correct codec to convert it out.

It seems like a really interesting feature to be able to move the Message between transports. Middleware will be much faster this way too!

Implementations can easily be combined into a single Transport that does both
(e.g. for use by the client package)

Message.Event() allows conversion from any message to the common
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you can do this without understanding both the transport and the codec.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct - every Message implementation does understand the transport/codec it originated from - it is part of the transport implementation. Either Message.Event() would be implemented using Codec.Decode() or vice-versa, either way the API is uniform.

Message.Event() == Codec.Decode(message)

The reason for introducing Message.Event() is to allow a Sender to decode a message that originated from a different Receiver transport without needing to know how it was encoded. This also supports other Message implementations like EventMessage which is simply a Message interface wrapper for a locally-created Event object.


Overview of changes

Sender/Receiver interfaces that can be implemented and used separately.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to know how you can see re-implementing send/response for http.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like response events to go away, see #160
Can you comment on that issue with some details of the use-cases for response events? If it is a requirement it will certainly be possible to build it, but IMO it should be an optional extra mode of operation, not presented as the basic API. I would probably do something like:

type RequestMessage interface { Response(Message) }

basically it becomes another QoS alternative where we require a complete Message in response, not just some protocol-specific ack sequence.

I'd like to understand the use cases better though - if there is a real requirement for portable, routable response events then the cloudevents spec needs to be updated with a correlationId. The problem of routing responses may be bigger than a single client-server connection.

Comment thread pkg/cloudevents/transport/x/message.go Outdated
receiver := NewExReceiver(r)
sender := NewExSender(w)
for i := 0; i < count; i++ {
if m, err := receiver.Receive(context.TODO()); err != nil {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see, the http transport could send and the resp could be queued and ready for the next Receive call.

Copy link
Copy Markdown
Contributor Author

@alanconway alanconway Jul 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - key thing here is how to present the request/response correlation in the API. I'd like it not to be the first thing the users see, or perceived as the "normal" way of sending events, but I think we can find a way to provide it as an optional feature for transports (like HTTP) that are RCP underneath.

@n3wscott
Copy link
Copy Markdown
Member

What do you think about making a branch in this repo to start staging this stuff and then we can work together on it, or a branch in your repo.

I think I still need to be convinced of the removal of codecs as an abstraction of how to turn events into messages. But codecs could be internal details of the transport Message objects rather than how I have it as a detail of the transport.

Let's keep going on this idea. I think it could turn into a much easer adaptation for non-http transports.

Thanks for the detailed comments in the PR!

@n3wscott
Copy link
Copy Markdown
Member

I have cut a 0.8.0 release to make room for these changes to come in.

@alanconway
Copy link
Copy Markdown
Contributor Author

What do you think about making a branch in this repo to start staging this stuff and then we can work together on it, or a branch in your repo.

I think I still need to be convinced of the removal of codecs as an abstraction of how to turn events into messages. But codecs could be internal details of the transport Message objects rather than how I have it as a detail of the transport.

My bad - I'm not proposing the removal of Codec, I just didn't bother to write an explict one in the example - fixing.

Let's keep going on this idea. I think it could turn into a much easer adaptation for non-http transports.

Yes - the biggest variable here is what we do about #160. I think adding a RequestMessage interface might be a good compromise for the #160 use cases while removing responses from the main transport API, but I would like to understand the requirement for response events better before I dive into that.

Thanks for the detailed comments in the PR!

I'll update with explicit Codec and RequestMessage { Response() }to see how that looks. I'm thinking that a response-capable transport (HTTP) would do if m, ok := m.(RequestMessage) { respond } whereas response-ignorant transports would blindly Finish() the message, in which case the requester can decide if that's an error, or if it will manufacture a fake response for the ultimate clien.t

@n3wscott
Copy link
Copy Markdown
Member

Moving Message out of the transport also leads to interesting developments around sharing codec that are very common in strategy, like binary mode in AMQP, PubSub and HTTP all use the equivalent of headers to send the data, the Key value is different in each, but the strategy is the same...

@alanconway
Copy link
Copy Markdown
Contributor Author

Moving Message out of the transport also leads to interesting developments around sharing codec that are very common in strategy, like binary mode in AMQP, PubSub and HTTP all use the equivalent of headers to send the data, the Key value is different in each, but the strategy is the same...

Agreed, e.g. the if Structured() else Event() dance could be part of a generic library usable by all transports that allow structured tunneling. Also the correct handling of different QoS options definitely should be captured generically, each transport just calls some relevant functions when it gets it's transport-specific ack indicator.

I'll probably do one or two more pushes in the current form to continue the discussion points so far, then try merging it into the main transport package and re-implementing some existing transports as you suggest.

Please see the code and doc for details.

Signed-off-by: Alan Conway <aconway@redhat.com>
@n3wscott
Copy link
Copy Markdown
Member

LGTM

Let's start this plan.

@n3wscott n3wscott merged commit 75cfef9 into cloudevents:master Aug 19, 2019
@matzew
Copy link
Copy Markdown
Member

matzew commented Aug 22, 2019

🎉

@alanconway alanconway deleted the inter-transport branch August 22, 2019 21:52
n3wscott pushed a commit to n3wscott/sdk-go that referenced this pull request Sep 20, 2019
…loudevents#164)

Please see the code and doc for details.

Signed-off-by: Alan Conway <aconway@redhat.com>
n3wscott pushed a commit to n3wscott/sdk-go that referenced this pull request Sep 20, 2019
…loudevents#164)

Please see the code and doc for details.

Signed-off-by: Alan Conway <aconway@redhat.com>
Signed-off-by: Scott Nichols <nicholss@google.com>
n3wscott added a commit that referenced this pull request Sep 20, 2019
* starting to add context for 0.4

Signed-off-by: Scott Nichols <nicholss@google.com>

* Enabling Pub/Sub to send and receive from multiple topics and subscriptions. (#171)

Signed-off-by: Scott Nichols <nicholss@google.com>

* Code and examples for proposed changes/additions to transport APIs (#164)

Please see the code and doc for details.

Signed-off-by: Alan Conway <aconway@redhat.com>
Signed-off-by: Scott Nichols <nicholss@google.com>

* fix a syntax error in a logw line. (#174)

Signed-off-by: Scott Nichols <nicholss@google.com>

* Changing Send signature: Adding Context return param (#177)

Signed-off-by: nachocano <nachoacano@gmail.com>
Signed-off-by: Scott Nichols <nicholss@google.com>

* Renamed pkg/cloudevents/transport/x to pkg/binding (#181)

Main types are binding.Message, binding.Sender and binding.Receiver which
reads well in code, corresponds to CE spec use of the term "binding", and
avoids clashes with existing package name "transport".

Part of #180 - The AMQP transport will be re-factored to use this package.

Signed-off-by: Alan Conway <aconway@redhat.com>
Signed-off-by: Scott Nichols <nicholss@google.com>

* fix extra header write. (#179)

Signed-off-by: Scott Nichols <nicholss@google.com>

* fix codec v0.3 double quote encoding (#184)

Signed-off-by: Diego Marangoni <diegomarangoni@me.com>
Signed-off-by: Scott Nichols <nicholss@google.com>

* Propagate pubsub subscriber errors (#185)

* Found a blocking issue on pubsub transport if a subscriber had an error.

Signed-off-by: Scott Nichols <nicholss@google.com>

* make sure cancel is called

Signed-off-by: Scott Nichols <nicholss@google.com>

* go mod.

Signed-off-by: Scott Nichols <nicholss@google.com>
Signed-off-by: Scott Nichols <nicholss@google.com>

* Fix nil pointer exception in codec timestamp handling (#187)

Signed-off-by: Scott Nichols <nicholss@google.com>

* Leaked a debug line. (#189)

Signed-off-by: Scott Nichols <nicholss@google.com>

* preping for v1.0, adjusting the interfaces.

Signed-off-by: Scott Nichols <nicholss@google.com>

* fix a typo on the godoc.

Signed-off-by: Scott Nichols <nicholss@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants