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

Make KafkaStreams health checks async #43294

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PhilKes
Copy link
Contributor

@PhilKes PhilKes commented Sep 14, 2024


@Readiness
@ApplicationScoped
public class KafkaStreamsTopicsHealthCheck implements HealthCheck {
public class KafkaStreamsTopicsHealthCheck implements AsyncHealthCheck {
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually don't think this is going to work properly because KafkaStreamsTopicsHealthCheck makes a blocking call to listTopics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right of course, I guess the Uni.createFrom().item should enclose the entire method body to make it asynchronous. Pushed the changes (+ squashed).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I'll let @ozangunalp take it from here

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not as easy as it seems. We do not have a reactive Kafka Admin client checking required topics on the broker. So this health check is inherently blocking. Wrapping it in Uni doesn't make it reactive, unfortunately.

And even though we'd transformed it to reactive I don't think it'll resolve the referenced issue.

We may discuss however, to move the Kafka Streams topics check from readiness to startup. That would help with the pods getting killed.

@quarkus-bot

This comment has been minimized.

@PhilKes PhilKes force-pushed the kafka-streams-async-health-check branch from a52f265 to 81844b4 Compare September 16, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kafka Streams: use non-blocking health checks
3 participants