-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Implement a label based discovery for Kafka Dev Service #17837
Implement a label based discovery for Kafka Dev Service #17837
Conversation
@cescoffier This looks great and will solve the problem I'm seeing with Dev Services for Keycloak - where a 2nd container is starting if I press Continuous Testing |
I went through several dev services implementations in the last few days and I think we really need a common class that helps implementing a dev service based on a single container. They all have a lot of things in common, but each also has some idiosyncratic differences. If we had that, it could also handle this label-based sharing. |
...t/deployment/src/main/java/io/quarkus/kafka/client/deployment/DevServicesKafkaProcessor.java
Outdated
Show resolved
Hide resolved
...t/deployment/src/main/java/io/quarkus/kafka/client/deployment/DevServicesKafkaProcessor.java
Outdated
Show resolved
Hide resolved
...oyment/src/main/java/io/quarkus/kafka/client/deployment/KafkaDevServicesBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
3daf27b
to
d59c7f6
Compare
@stuartwdouglas What do you want to do about this one? Should be move forward? |
I think it looks good. |
Ok, I will rebase , polish a bit and ask for reviews. |
4e6c482
to
de3a86f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some minor comments.
...t/deployment/src/main/java/io/quarkus/kafka/client/deployment/DevServicesKafkaProcessor.java
Outdated
Show resolved
Hide resolved
core/deployment/src/main/java/io/quarkus/deployment/DevServices.java
Outdated
Show resolved
Hide resolved
de3a86f
to
9b14b91
Compare
@stuartwdouglas I updated the code to use a specific label for Kafka. |
...t/deployment/src/main/java/io/quarkus/kafka/client/deployment/DevServicesKafkaProcessor.java
Outdated
Show resolved
Hide resolved
...oyment/src/main/java/io/quarkus/kafka/client/deployment/KafkaDevServicesBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
...oyment/src/main/java/io/quarkus/kafka/client/deployment/KafkaDevServicesBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
...oyment/src/main/java/io/quarkus/kafka/client/deployment/KafkaDevServicesBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
...oyment/src/main/java/io/quarkus/kafka/client/deployment/KafkaDevServicesBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
Looks good, all my comments are about documentation (and one is about logging). |
d06e724
to
55b7a89
Compare
This workflow status is outdated as a new workflow run has been triggered. |
Instead to have to configure the kafka.bootstrap.url, it looks for containers having a specific label and compute the bootstrap server. Discovery can be disabled in the configuration, and it is only used in dev mode (disabled for test).
55b7a89
to
b601886
Compare
I know I'm a pain, but looks good now! :-) |
Instead to have to configure the kafka.bootstrap.url, it looks for containers having a specific label and computes the bootstrap server.
Discovery can be disabled in the configuration, and it is only used in dev mode (disabled for test).
@stuartwdouglas we discussed this briefly on the mailing list. It's a draft but works.
TODO: