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

Implement Dev Services for Kafka #17468

Merged
merged 1 commit into from
Jun 3, 2021

Conversation

cescoffier
Copy link
Member

Add support for Kafka as Dev Service.

It uses Red Panda as it's way faster (more than 20 seconds using the official image, ~ 15 seconds with Strimzi, less than 1 second with Red Panda).

@Ladicek
Copy link
Contributor

Ladicek commented May 26, 2021

I don't know anything about Dev Services (particularly how state is supposed to be managed in the processor class), but other than that, this looks OK baring a few comments.

@cescoffier cescoffier force-pushed the dev-services-for-kafka branch from 750489a to 6caf1a6 Compare May 26, 2021 12:04
and pull requests should be submitted there:
https://github.com/quarkusio/quarkus/tree/main/docs/src/main/asciidoc
////
= Dev Services for Kafka
Copy link
Member

Choose a reason for hiding this comment

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

Does it really make sense to have a specific document just for that?

Not sure how it's organized for others but it feels a bit weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to have it in the Kafka quickstart (which has a lot of extra content that needs to move). Dev Services and its configuration has nothing to do with the quickstart. When we will have a proper reference guide it will go there.

Having a Kafka reference guide is on my todo list.

@geoand
Copy link
Contributor

geoand commented May 26, 2021

Seems very similar to what we have for Mongo and Redis, so LGTM

@sberyozkin
Copy link
Member

Should there be some template support for it as well ? As in the quarkus-cache for example ?

@sberyozkin
Copy link
Member

It can be complemented later for sure if it can be of interest

@pjgg
Copy link
Contributor

pjgg commented May 27, 2021

Kafka DevServices! Great news!!!.

Just a quick one, does this first release of Kafka DevServices support AVRO (a schema registry)?

@cescoffier
Copy link
Member Author

@pjgg it supports Avro, but would not start a schema registry. We would need to find a slim, fast to start schema registry first.

@cescoffier
Copy link
Member Author

@sberyozkin @geoand it looks similar but is not exactly the same. Kafka requires customizing the container to configure the advertised brokers. So, the docker/container part is slightly more convoluted (well, not the end of the world).

@Ladicek
Copy link
Contributor

Ladicek commented May 27, 2021

Unrelated to this PR, but -- when we add extension(s) for Apicurio Registry (the Avro library will be first, but Protobuf will likely follow), we can add Dev Services support for schema registry. The Apicurio Registry is pretty fast to start (it's a Quarkus app after all! :-) ), so should be fine IMHO.

@cescoffier
Copy link
Member Author

@Ladicek ideally, we would have a "slim" container image that we can just run.

@Ladicek
Copy link
Contributor

Ladicek commented May 27, 2021

Apicurio Registry has a container image with in-memory storage; I think we can easily use that: https://hub.docker.com/r/apicurio/apicurio-registry-mem/tags

@cescoffier
Copy link
Member Author

Awesome! Exactly what we need.

@cescoffier
Copy link
Member Author

Does it needs to access the Kafka broker (the dev services for kafka emits a build item with the bootstrap server, so we should be fine, but we will have to check)

@Ladicek
Copy link
Contributor

Ladicek commented May 27, 2021

It doesn't need to access the Kafka broker -- not in the in-memory variant. They have Kafka-based storage as well, but I assume we wouldn't use that for dev services.

@rsvoboda
Copy link
Member

Red Panda was picked for Dev Services. What if user wants to use the official image or Strimzi? Is the Kafka DevServices going to be customizable to allow Strimzi for example?

@cescoffier
Copy link
Member Author

@rsvoboda at the moment it only supports red panda.

We can add strimzi, but the experience was quite bad on my machine. Note that it's not just changing the image name, the whole startup script, kafka advertising, and so on need to be done for that one too. Hopefully, we would get images using Kraft.

Don't forget that the user can still start its own cluster (strimzi or whatever he likes) and disables the dev services.

Copy link
Member

@stuartwdouglas stuartwdouglas left a comment

Choose a reason for hiding this comment

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

LGTM, but I just realized that we have actually missed native support in this and some of the other dev services PRs. It's probably best to just merge this one and then fix them all together.

Copy link
Member

@stuartwdouglas stuartwdouglas left a comment

Choose a reason for hiding this comment

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

Actually looks like it was added for the other ones, but is missing for this one, see io.quarkus.test.junit.NativeDevServicesHandler.

TBH I don't like that these are separate build items, they should be refactored to a single common one, it really just sets system properties so this could be simplified a lot I think.

@stuartwdouglas
Copy link
Member

#17574 is what I am talking about in terms of a generic build item.

@cescoffier
Copy link
Member Author

cescoffier commented Jun 1, 2021

So basically producing one per "key"?
It creates an implicit binding between exceptions using the name of the key.

@cescoffier
Copy link
Member Author

BTW, Trying to rush the integration of NativeDevServicesHandler. I need this in 2.0 final.

@cescoffier cescoffier force-pushed the dev-services-for-kafka branch from ca7f069 to eefcee6 Compare June 1, 2021 12:19
@cescoffier
Copy link
Member Author

I've added the DevServicesNativeConfigResultBuildItem.

@cescoffier cescoffier requested a review from stuartwdouglas June 2, 2021 07:07
@cescoffier
Copy link
Member Author

@stuartwdouglas can you have a look?

@cescoffier cescoffier force-pushed the dev-services-for-kafka branch from eefcee6 to 4481a1f Compare June 3, 2021 05:54
@cescoffier cescoffier requested a review from stuartwdouglas June 3, 2021 05:56
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 3, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 4481a1f

Status Name Step Test failures Logs Raw logs
JVM Tests - JDK 11 Build Test failures Logs Raw logs
✔️ JVM Tests - JDK 16

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/kafka-streams

io.quarkus.it.kafka.streams.KafkaStreamsStartupFailureTest.testShutdownBeforeKStreamsStarted - More details - Source on GitHub

@cescoffier cescoffier merged commit 4988e37 into quarkusio:main Jun 3, 2021
@quarkus-bot quarkus-bot bot added this to the 2.1 - main milestone Jun 3, 2021
@gsmet gsmet modified the milestones: 2.1 - main, 2.0.0.CR3 Jun 3, 2021
@cescoffier cescoffier deleted the dev-services-for-kafka branch June 9, 2021 08:32
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.

8 participants