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

Add ApplicationClient addOnClientReadyHandler(), fix example application startup #3278

Merged

Conversation

calohmn
Copy link
Contributor

@calohmn calohmn commented Jun 2, 2022

This is especially needed for the KafkaApplicationClient, letting a given handler be invoked when the Kafka producer
is ready.
Also introduce a lifecycleStatus in AbstractServiceClient, preventing successive start() invocations.

Adding addOnClientReadyHandler() in ApplicationClient and ProtonBasedApplicationClient.
(KafkaApplicationClient change already done in #3282.)

Also fixing application client startup in Hono example application.

@calohmn calohmn added the Client label Jun 2, 2022
@calohmn calohmn added this to the 2.0.0 milestone Jun 2, 2022
@calohmn calohmn added this to In progress in 2.0.0 via automation Jun 2, 2022
@@ -102,6 +103,11 @@ public KafkaApplicationClientImpl(
this.consumerConfig = consumerConfig;
}

@Override
public void addOnClientReadyHandler(final Handler<AsyncResult<Void>> handler) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this method being used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in Hono itself. But it is useful for applications using the Hono (Kafka)ApplicationClient.

@sophokles73
Copy link
Contributor

@calohmn Are you still working on this? Do you want/need this in 2.0.0?

@calohmn calohmn removed this from the 2.0.0 milestone Jun 8, 2022
@calohmn calohmn removed this from In progress in 2.0.0 Jun 8, 2022
@calohmn
Copy link
Contributor Author

calohmn commented Jun 8, 2022

@sophokles73 I'm still on it, yes. But I think this should be merged after the 2.0.0 release.

@sophokles73
Copy link
Contributor

@calohmn is this ready to be merged?

@calohmn calohmn marked this pull request as draft July 13, 2022 06:08
@calohmn
Copy link
Contributor Author

calohmn commented Jul 13, 2022

@sophokles73 Not yet.

@calohmn calohmn force-pushed the PR/applicationclient_readyhandler branch from f9ba003 to fecf257 Compare September 23, 2022 13:54
@calohmn
Copy link
Contributor Author

calohmn commented Sep 23, 2022

I've added the lifecycleStatus handling now directly in ProtonBasedApplicationClient in order to implement the added addOnClientReadyHandler method.
Changing AbstractServiceClient would have meant a lot more changes.

@calohmn calohmn marked this pull request as ready for review September 23, 2022 14:15
Also improve AbstractServiceClient start/stop extensibility.

Signed-off-by: Carsten Lohmann <carsten.lohmann@bosch.io>
Signed-off-by: Carsten Lohmann <carsten.lohmann@bosch.io>
@calohmn calohmn force-pushed the PR/applicationclient_readyhandler branch from fecf257 to aa10a03 Compare September 26, 2022 10:31
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

@calohmn calohmn merged commit c3de211 into eclipse-hono:master Sep 26, 2022
@calohmn calohmn deleted the PR/applicationclient_readyhandler branch September 26, 2022 11:19
@calohmn calohmn added this to In progress in 2.2.0 via automation Sep 27, 2022
@calohmn calohmn added this to the 2.2.0 milestone Sep 27, 2022
@calohmn calohmn changed the title Add ApplicationClient addOnClientReadyHandler() Add ApplicationClient addOnClientReadyHandler(), fix example application startup Sep 27, 2022
@calohmn calohmn moved this from In progress to Done in 2.2.0 Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
2.2.0
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants