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

Simplify and split HonoClient for applications versus protocol adapters #780

Closed
sysexcontrol opened this issue Aug 14, 2018 · 9 comments
Closed
Assignees
Milestone

Comments

@sysexcontrol
Copy link
Contributor

Originating from a discussion on the dev mailing list, I think we should split the HonoClient to at least two different use cases:

  • what typical applications need

    • telemetry and event consumers
    • command senders
  • what (custom) protocol adapters need

    • telemetry and event senders
    • tenant, credentials and registration clients

I am sure this needs some discussion - I can imagine that the tenant, credentials and registration clients might be desirable to have available outside a protocol adapter as well, so that we would have three different clients.
A use case for that could be a device registry client application IMHO.

The point of this issue is to come to a more focussed HonoClient, so that an application developer should not have to ask a question like "why do I have a telemetry sender available, but I cannot use it since I can only consume telemetry messages?". The same question could be asked for the tenant, credentials and registration clients, for which the remote APIs are usually not accessible for applications.

After having discussed this issue, we can rewrite it to describe what we want to achieve in the future with HonoClient (and possibly split it up).

Remark: for Command and Control we already made a small step into this direction : the CommandConnection is an extension of HonoClient and thus does not provide it's methods to applications.

@avgustinmm
Copy link
Contributor

I think that current approach is not consistent - e.g; having telemetry & event senders but not command consumers and command response senders.
The Hono Client should take one of the following approaches:

  • contain ALL APIs, including protocol adapter C&C. E.g. hono client will stand for all-in-one API/util for accessing Hono core.
  • contain only application related APIs. I.e. remove telemetry&event senders. It may or may not keep device/credential and tenant APIs. In this case a protocol adapter API shall be implemented that contains telemetry&event sender as well as device/credential and tenant APIs. If the message senders and admin API will be separated is up not a big difference to me.

@sophokles73 sophokles73 added this to the 0.8 milestone Aug 21, 2018
@sysexcontrol
Copy link
Contributor Author

I think that current approach is not consistent - e.g; having telemetry & event senders but not command consumers and command response senders.

Hi @avgustinmm : as I wrote we have the CommandConnection available (which is a HonoClient already). This separates a little on the adapter side.

However, we agree on that the hono client needs to be more clearly separated for the needs of

  • applications
  • and protocol adapters

Now it is separated by the features which undoubtedly is not good.

I propose to have an interface hierarchy for this purpose:

HonoApplicationClient extends HonoClient

HonoAdapterClient extends HonoClient

IMHO this would lead to three maven modules (instead of one that we have now):

  • hono-client : containing generic parts (like the connect method and so on)
  • hono-application-client: depending on hono-client, additionally providing the HonoApplicationClient interface and impl
  • hono-adapter-client: depending on hono-client, additionally providing the HonoAdapterClient interface and impl

This would need some code to be moved to the client module for the adapter specific part, which currently is only available in the adapter module hierarchy. By this way the implementation of Command and Control for new adapters should be eased (and OSGi should be supported as well, if I see things correctly).

This should address your current concerns - do you agree?

If yes, I would rewrite the issue description to be more concise.

@sophokles73
Copy link
Contributor

sophokles73 commented Nov 5, 2018

I like the idea of splitting into HonoAdapterClient and HonoApplicationClient. I assume that we agree that this would also mean moving all the factory methods (e.g. getOrCreateTelemetrySender) from HonoClient to the corresponding sub-interface, right?

This means that we are talking about quite an API break in HonoClient. I am generally fine with this, I just want to make sure that we all are on the same page here. However, I would rather postpone this to the 0.9 release because we are already quite close to the 0.8 release date and I would rather do this change without a hurry, leaving us enough time to do thorough testing. Given that we will break the API with this change anyway, I would also like to make a (breaking) change to the connect methods as well so that client code can register handlers to be notified whenever the connection is lost and re-established. This would help with #559 and would allow us to leave reconnection to HonoClient itself while application code could focus on housekeeping/re-establishing links.

@sysexcontrol @dejanb @ctron @pellmann @ppatierno WDYT?

@sysexcontrol
Copy link
Contributor Author

@sophokles73 first quick comment regarding postponing it to 0.9: I already wanted to propose the same, so 👍 from me for removing it from 0.8.
(more to follow later...)

@sysexcontrol
Copy link
Contributor Author

I assume that we agree that this would also mean moving all the factory methods (e.g. getOrCreateTelemetrySender) from HonoClient to the corresponding sub-interface, right?

Right, this is exactly the idea.

This means that we are talking about quite an API break in HonoClient.

Absolutely - this is why I completely agree to not force it into 0.8 (and do the mentioned thorough testing while doing it for 0.9).

@sysexcontrol
Copy link
Contributor Author

So I would move this to 0.9 if no one objects today?

@sysexcontrol sysexcontrol modified the milestones: 0.8, 0.9 Nov 6, 2018
@sophokles73 sophokles73 modified the milestones: 0.9, 1.0.0 Feb 26, 2019
@calohmn
Copy link
Contributor

calohmn commented Mar 13, 2019

The Hono auth component also uses the credentialsService and tenantService clients of HonoClient.
E.g.

    public TenantServiceBasedX509Authentication(
            final HonoClient tenantServiceClient,
            final Tracer tracer) {

Instead of using HonoAdapterClient tenantServiceClient here, I think it be more consistent to use a HonoServiceClient class instead.

Therefore I would suggest:

HonoApplicationClient extends HonoClient    // telemetry and event consumers and command senders
HonoServiceClient extends HonoClient        // with the tenant, credentials and registration clients methods
HonoAdapterClient extends HonoServiceClient // with the telemetry and event sender methods

Concerning the connect methods in HonoClient:

Future<HonoClient> connect();

Instead of parameterizing HonoClient via generics here (so that HonoApplicationClient#connect() returns a Future<HonoApplicationClient>), I would suggest changing the signature to:

Future<Void> connect();

This also prevents possible confusion about whether the HonoClient instance on which connect was called could be different from the one returned via the connect() Future.

calohmn added a commit to bosch-io/hono that referenced this issue Mar 13, 2019
@sophokles73 sophokles73 self-assigned this Mar 14, 2019
@sophokles73
Copy link
Contributor

The Hono auth component also uses the credentialsService and tenantService clients of HonoClient.

I agree, we will need to have a more fine grained way of getting clients for the arbitrary (south bound) services instead of just having a single factory which is shaped according to the needs of protocol adapters only.

I would also like to separate the client factories from the connection management so that we do not need to change the connection management class(es) with each change we make to the services.

sophokles73 pushed a commit to bosch-io/hono that referenced this issue Mar 18, 2019
The factory methods for creating the arbitrary service clients have been
refactored into dedicated interfaces.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit to bosch-io/hono that referenced this issue Mar 18, 2019
The factory methods for creating the arbitrary service clients have been
refactored into dedicated interfaces.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit to bosch-io/hono that referenced this issue Mar 28, 2019
The factory methods for creating CommandConsumers have been
refactored into a dedicated interface.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit to bosch-io/hono that referenced this issue Mar 28, 2019
…ory.

The protocol adapters have been changed to use the
CommandConsumerFactory instead of the CommandConnection.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 3, 2019
The factory methods for creating CommandConsumers have been
refactored into a dedicated interface.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 3, 2019
The protocol adapters have been changed to use the
CommandConsumerFactory instead of the CommandConnection.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit to bosch-io/hono that referenced this issue Apr 3, 2019
The protocol adapters have been changed to use the
TenantClientFactory instead of the HonoClient.


Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 5, 2019
The protocol adapters have been changed to use the
TenantClientFactory instead of the HonoClient.


Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit to bosch-io/hono that referenced this issue Apr 5, 2019
The protocol adapters have been changed to use the
RegistrationClientFactory instead of the HonoClient
for creating RegistrationClient instances.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 8, 2019
The protocol adapters have been changed to use the
RegistrationClientFactory instead of the HonoClient
for creating RegistrationClient instances.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit to bosch-io/hono that referenced this issue Apr 8, 2019
The protocol adapters have been changed to use the
CredentialsClientFactory instead of the HonoClient
for creating CredentialsClient instances.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 9, 2019
The protocol adapters have been changed to use the
CredentialsClientFactory instead of the HonoClient
for creating CredentialsClient instances.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 10, 2019
sophokles73 pushed a commit to bosch-io/hono that referenced this issue Apr 10, 2019
The protocol adapters have been changed to use the
DownstreamSenderFactory instead of the HonoClient
for creating MessageSender instances.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 10, 2019
The protocol adapters have been changed to use the
DownstreamSenderFactory instead of the HonoClient
for creating MessageSender instances.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 11, 2019
Connections are now established and shut down via ConnectionLifecycle.
Thus, the existing methods accepting HonoClient as a parameter are now
obsolete.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 11, 2019
sophokles73 pushed a commit that referenced this issue Apr 11, 2019
sophokles73 pushed a commit that referenced this issue Apr 11, 2019
sophokles73 pushed a commit that referenced this issue Apr 11, 2019
sophokles73 pushed a commit that referenced this issue Apr 11, 2019
sophokles73 pushed a commit that referenced this issue Apr 15, 2019
HonoConnection will only be responsible for managing the underlying AMQP
connection and will no longer serve as a factory for service clients
anymore. The changed class name better reflects this fact.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 15, 2019
The DownstreamSenderFactory methods have been moved from
HonoConnectionImpl to DownstreamSenderFactoryImpl.
As part of this, the org.eclipse.hono.client.impl.*ClientImpl hierarchy
has been refactored to use a HonoConnection instance instead of passing
around multiple properties of the connection explicitly. This serves as
the groundwork for implementing the other client factories analogously
to the DownstreamSenderFactory.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 15, 2019
The methods for creating TenantClient instances have been moved from
HonoConnectionImpl to TenantClientFactoryImpl.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 15, 2019
The methods for creating RegistrationClient instances have been moved
from HonoConnectionImpl to RegistrationClientFactoryImpl.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 15, 2019
The methods for creating CredentialsClient instances have been moved
from HonoConnectionImpl to CredentialsClientFactoryImpl.


Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 15, 2019
The methods for creating MessageConsumer, CommandClient and
AsyncCommandClient instances have been moved from HonoConnectionImpl to
ApplicationClientFactoryImpl.

Signed-off-by: Kai Hudalla <[email protected]>
sophokles73 pushed a commit that referenced this issue Apr 15, 2019
The methods for creating command consumer links and
CommandResponseSender instances no longer depend on HonoConnectionImpl.

Signed-off-by: Kai Hudalla <[email protected]>
@sophokles73
Copy link
Contributor

fixed in da36835

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants