Skip to content
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

[#1761] Add AMQP adapter client. #1765

Merged
merged 7 commits into from Mar 24, 2020

Conversation

b-abel
Copy link
Contributor

@b-abel b-abel commented Feb 12, 2020

This is the first solution for #1761, which on the one hand uses existing code from the client module and on the other hand aims to expose as few classes as possible to the outside world that are designed for internal use in the protocol adapters.

The entry point is the factory interface AmqpAdapterClientFactory, which can be used to create clients.
I had to put some classes into the existing client.impl package because of the visibility of the code used, the rest I tried to separate a little bit.
Tests (and documentation) will be added once we have agreed on the overall direction.

It is a quick and easy solution, in a next step it would be good to have a simpler client interface that does not expose the link handling and scopes the connection to the tenant, but provides simple send methods. I hope to do another PR on this soon. For that, I would make a separate Maven module with minimal dependencies.

@b-abel
Copy link
Contributor Author

b-abel commented Feb 19, 2020

Added tracing and inherit from classes used inside the protocol adapters.

@b-abel
Copy link
Contributor Author

b-abel commented Mar 9, 2020

@calohmn What is the status of this issue?

@calohmn
Copy link
Contributor

calohmn commented Mar 10, 2020

@b-abel Can you squash and rebase (also to check with the new checkstyle rules introduced since) ?

@b-abel
Copy link
Contributor Author

b-abel commented Mar 10, 2020

Done.

Copy link
Contributor

@calohmn calohmn left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

LGTM

@sophokles73 sophokles73 added this to In progress in 1.2.0 via automation Mar 11, 2020
@sophokles73 sophokles73 added this to the 1.2.0 milestone Mar 11, 2020
@sophokles73
Copy link
Contributor

@b-abel are you going to integrate this new client into the protocol gateway example code?

@b-abel
Copy link
Contributor Author

b-abel commented Mar 11, 2020

@b-abel are you going to integrate this new client into the protocol gateway example code?

Yes.

@b-abel
Copy link
Contributor Author

b-abel commented Mar 11, 2020

I would add tests and documentation to this PR. Or do you prefer to open a new one for that?

@sophokles73
Copy link
Contributor

@b-abel any way you want it ... as long as you do create docs and tests ;-)

@sophokles73
Copy link
Contributor

@b-abel so, what's it gonna be? This PR or a separate one?

@b-abel
Copy link
Contributor Author

b-abel commented Mar 11, 2020

I will add them in this PR.

@sophokles73
Copy link
Contributor

@b-abel what is the status of this PR? Are you still working on docs and tests?

@b-abel
Copy link
Contributor Author

b-abel commented Mar 18, 2020

@b-abel what is the status of this PR? Are you still working on docs and tests?

Yes.

@b-abel
Copy link
Contributor Author

b-abel commented Mar 18, 2020

@b-abel what is the status of this PR? Are you still working on docs and tests?

Done.

@b-abel
Copy link
Contributor Author

b-abel commented Mar 23, 2020

I added the requested changes.

Device (or gateway) clients that connect to the AMQP adapter can be created
using AmqpAdapterClientFactory. The implementation reopens closed connections
and restores the receiver link for consuming commands afterward. Sender links
are cached and removed from the cache when closed so that they are recreated
the next time they are retrieved from the factory.

Signed-off-by: Abel Buechner-Mihaljevic <abel.buechner-mihaljevic@bosch.io>
…ownstream messages.

Having the content type required to send telemetry, event or command responses
was a copy&paste error. The content type might not always be available.
+ Fix Javadoc around that.

Signed-off-by: Abel Buechner-Mihaljevic <abel.buechner-mihaljevic@bosch.io>
…write required ones.

Signed-off-by: Abel Buechner-Mihaljevic <abel.buechner-mihaljevic@bosch.io>
…Client.

Signed-off-by: Abel Buechner-Mihaljevic <abel.buechner-mihaljevic@bosch.io>
… Adapter Client.

Signed-off-by: Abel Buechner-Mihaljevic <abel.buechner-mihaljevic@bosch.io>
Signed-off-by: Abel Buechner-Mihaljevic <abel.buechner-mihaljevic@bosch.io>
@b-abel
Copy link
Contributor Author

b-abel commented Mar 23, 2020

I just pushed an update.

deliveryFuture.setHandler(ctx.completing());

// THEN the future waits for the disposition to be updated by the peer
Thread.sleep(100L);
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please make sure to remove all occurrences of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I missed them.

…p() from tests.

Signed-off-by: Abel Buechner-Mihaljevic <abel.buechner-mihaljevic@bosch.io>
Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

LGTM

@sophokles73 sophokles73 merged commit e07eb47 into eclipse-hono:master Mar 24, 2020
1.2.0 automation moved this from In progress to Done Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
1.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants