Initial version of the Vert.x AMQP client

[cescoffier]

This PR contains the initial version of the Vert.x AMQP client.

[cescoffier]

@gemmellr Thanks for the detailed review!

I've resolved all the issues you reported. The big change it the removal of the reply feature, which simplifies a lot of things (API, behavior...)

[vietj]

I would like to discuss an simplifying API changes.

Currently I see that you can send messages by using the chain:

1. create a client
2. open a connection
3. create a sender
4. send the message

I would like to simplify to basically do avoid having the user create a connection and let the client create the connection under the hood. In addition a client only knows a single host so there is no ambiguity about which host to connect to.

So the change would be that a client allows to create a sender and a receiver without a connection. The connection is lazily created by the client the first time it's needed and closed when it's not needed anymore, so the user flow would be:

1. create a client
2. create a sender
3. send the message

We can also have one-shot methods to directly send individual messages when no write-stream is needed or the cost of creating a sender is not important: have on the client the same send methods that are on the sender, where the send method creates a sender, uses it and then dispose it, so sending a simple message is just a single operation on the client.

I believe such features don't require much changes.

[gemmellr]

@cescoffier Thanks for the updates. I'm glad to see the implicit reply stuff disappeared also.

The send method docs/javadoc didn't seem to really change, I still think they need attention per the original comments around the 'different address' stuff. I think its not clear and is very likely to be misunderstood since most of the time it wont actually do remotely what is implied by the current text (unless you happen to be using an 'anonymous' sender).

[gemmellr]

I'm not fond of simplifications like hiding the connection, but especially dislike the idea of of 'one shot send' methods. People inevitably use these simple things without proper consideration when presented with them. We constantly end up needing to discuss the resulting performance issues, and inefficient load it can place on clients and servers, etc etc. It also tends to complicate the impls when we then try doing things to reduce the impact of the inefficient operations when people do then [mis]use them and get into poor situations. If the alternative were massively difficult I might see more need but I don't think that's really the case.

[cescoffier]

The send method docs/javadoc didn't seem to really change

Let me check that, I may have miss it.

About the address. I'm confused about the address and the to field. So, I'm wondering if we should not remove the method taking the address as a parameter.

[gemmellr]

Removing those methods would suit me.

[vietj]

The simplification is because it is asynchronous and therefore sending a message requires 2 asynchronous method calls, we have applied this in various Vert.x clients and the feedback is rather positive than negative.

The goal is not to hide the connection, the goal is to provide a shortcut for people that want to go straight to the point and just send messages.

People inevitably use these simple things without proper consideration when presented with them can you elaborate why it would be more costly ?

[cescoffier]

@vietj I've added methods to create senders and receivers on the client itself (so no need to manage the connection explicitly).

@gemmellr I've added anonymous senders, dynamic receivers, terminus configuration on the receiver side (including a durable property), offered and desired capabilities on the receiver side.

Still 2 questions:

• does terminus configuration make sense on the sender side? Should we configure the source or target in this case (I would say target)
• when I close a durable receiver, I'm calling detach instead of close, is it the right behavior?

[gemmellr]

can you elaborate why it would be more costly ?

Those comments were mainly aimed at the 'instant send' method suggestion. Creating senders isnt free, and every time this approach is offered people abuse or misuse it unwittingly, then we wend up wasting time trying making other bits smarter to try and fix the problem we only created by trying to fix one that didn't really exist.

[gemmellr]

@cescoffier Yes terminus config makes sense on a sender. It will typically be the target that needs configuring, but it can be either for both sender and receiver (a link is always between a source and a target, its just a question which side is which based on if sending or receiving). Detaching the durable receiver link makes sense yes, if you want to come back later and resume consuming from the same 'subscription' you then can. If you don't you close it.

[cescoffier]

@gemmellr I've addressed your comments.

[cescoffier]

@gemmellr I've done another pass.

[vietj]

are we fine @gemmellr ?

[gemmellr]

I added some comments last week after the most recent changes, there are some issues in the tests causing significant delays (which themselves may imply some issues in the actual code).

[cescoffier]

@gemmellr Oh! I missed these comments.

[vietj]

thanks for the heads-up !

On 11 Mar 2019, at 17:00, Robbie Gemmell notifications@github.com wrote:

I added some comments last week after the most recent changes, there are some issues in the tests causing significant delays (which themselves may imply some issues in the actual code).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-amqp-client/pull/9#issuecomment-471603272, or mute the thread https://github.com/notifications/unsubscribe-auth/AANxitf0cpj-5JfiIjc7I5jNNzylftzuks5vVn2pgaJpZM4bJMGA.

[gemmellr]

Added another comment to above test etc discussion.

[cescoffier]

@vietj @gemmellr I've fixed the tests.

[gemmellr]

Expanded prior comment around the close(..) handling of disconnect a bit, still looks to be an issue there, and added some new comments on other niggles noticed while looking around again.

The precise trouble with the test delays seems fixed/sidestepped, and ones I could see were slow are running quickly now. From the CI output the overall job looks (I cant get the docker-using tests to work on either of my machines, and have given up trying) a good bit quicker now as a result which is nice, though still quite slow for all the tests there are.

[cescoffier]

The slowness is mainly due to the docker image. The broker is started and stopped quite often and this takes time.

[gemmellr]

Yep. Now seeing it in action (in CI at least) I'd probably remove the docker usage eventually, I don't think its adding much here except overhead. An embedded broker would be far faster. Most of the tests would be better tests of the client itself if they didn't use a broker.

[cescoffier]

I have been requested to test against other brokers, so the docker approach is pretty convenient for now.

[vietj]

@gemmellr @cescoffier can this PR be merged soon for vert.x 3.7 as tech preview ?

[gemmellr]

Fine by me

[cescoffier]

Woohoo! Merging!

[cescoffier]

Thanks @gemmellr for the review!

[vietj]

thanks @gemmellr it's a great step forward!

[gemmellr]

Though I did expect my earlier feedback on issues with the latest changes (https://github.com/vert-x3/vertx-amqp-client/pull/9#discussion_r267148165) to be looked at before it was "merged soon".