Introduce a BuildItem in DevKafkaService to add additional config #25140

zhfeng · 2022-04-25T14:51:53Z

Fix #25094

…dditional configs

quarkus-bot · 2022-04-25T14:52:11Z

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

title should not end up with ellipsis (make sure the title is complete)
title should not contain an issue number (use Fix #1234 in the description instead)

This message is automatically generated by a bot.

...ent/src/main/java/io/quarkus/kafka/client/deployment/DevServiceKafkaAdditionalBuildItem.java

gastaldi · 2022-04-25T17:27:42Z

...t/deployment/src/main/java/io/quarkus/kafka/client/deployment/DevServicesKafkaProcessor.java

+        HashMap<String, String> configs = new HashMap<>();
+        configs.put(KAFKA_BOOTSTRAP_SERVERS, address);
+        additions.forEach(c -> {
+            configs.put(c.getConfig(), address);


The BuildItem is used exclusively to set the Kafka's server address as a value for other keys?

yeah, we use it in camel.component.kafka.brokers see apache/camel-quarkus#3742

Also you can check the detail about the motivation of these changes in #25094

I see, I am just concerned that the class name DevServiceKafkaAdditionalBuildItem may be too generic for this reason

OK, it is hard to name it :) any thought ?

ozangunalp

I don't think that this is the right solution for this problem.
We need to understand why DevServicesLauncherConfigResultBuildItem doesn't contain the dev service config in native mode.
The code here seems to be the right way of getting the dev service config.

zhfeng · 2022-04-26T07:29:37Z

But in camel-quarkus-kafka, the above build step seems not run at all in native mode. I'm not sure it is because we produce RunTimeConfigurationDefaultBuildItem which do not be consumered in native?

ozangunalp · 2022-04-26T10:03:12Z

@zhfeng In integration tests, the application is built in "normal" mode, without dev services or test mode augmentation.
The availability of dev services for native integration tests is somewhat simulated with a separate build and injected to the application config, as you see it on the logs -Dkafka.bootstrap.servers=...

That's why contrary to what I wrote before, the previous code is not the right way of doing this.

In hindsight, I don't completely understand the need to have a camel.component.kafka.brokers default config only for the tests.
I think it is better to handle this case inside the test case.
You can check out io.quarkus.test.common.DevServicesContext which lets accessing dev service properties from tests, including integration tests.

Maybe @geoand would have another take on this?

Ref #25094

geoand · 2022-04-26T10:07:44Z

In hindsight, I don't completely understand the need to have a camel.component.kafka.brokers default config only for the tests.

I don't undertand this requirement at all either :)

@zhfeng can you describe the end goal of what you are trying to do?

zhfeng · 2022-04-26T10:34:54Z

@ozangunalp camel-quarkus-kafka extension reads camel.component.kafka.brokers and autowire it into the camel-kafka during the camel-quarkus based application bootstrap.

@gastaldi I want to get the camel.component.kafka.broker with the same value of kafka.bootstrap.servers automatically both in jvm and native mode. And in native, just like -Dcamel.component.kafka.brokers=... in args when launching the native test. Currently there is a workaround to set it in application.properties

%prod.camel.component.kafka.brokers=${kafka.bootstrap.servers}

@jamesnetherton do you have any input here?

zhfeng · 2022-04-28T06:38:46Z

@ozangunalp I find some similar codes with HibernateOrmProcessor.java which produce RunTimeConfigurationDefaultBuildItem to set some properties on Dev Services managed database.

So I wonder this is only availiable in JVM but not Native testing?

ozangunalp · 2022-04-28T14:59:51Z

Same for hibernate, that runtime default config would not be set on the application under test during integration tests.
That is why you would actually need quarkus.hibernate-orm.database.generation=drop-and-create in properties file for those ITs to work.

zhfeng · 2022-04-28T16:30:01Z

Thanks for explanation. So I think it could be much more reasonable to introduce such a BuilidItem in DevServiceKafka which could be very helpful for other extensions in IT native testing.

ozangunalp · 2022-04-29T07:49:56Z

I couldn't find where does the camel.component.kafka.brokers property is defined. If it is a safe assumption you can write camel.component.kafka.brokers=${kafka.bootstrap.servers} as a default configuration property in the camel extension.

zhfeng · 2022-04-29T14:59:40Z

yeah, we have an example https://github.com/apache/camel-quarkus-examples/tree/main/kafka. We only want camel.component.kafka.brokers=${kafka.bootstrap.servers} in Dev or Test profile when DevKafkaService is enabled. That's the reason we introudce a BuildStep in camel-quarkus-kafka extension.

Since the native IT test is running in prod profile, apache/camel-quarkus-examples#86 could be a good way.

cescoffier · 2022-05-01T15:08:13Z

Shouldn't this be done by the camel extension?
Our dev service for Kafka can emit a build item with the configuration if needed, but that's all we can do.

As highlighted before, having

camel.component.kafka.brokers=${kafka.bootstrap.servers}

is the right way to do it Today.

zhfeng · 2022-05-05T05:45:18Z

Thanks @cescoffier ! As I understand, the purpose of dev services is avoiding or lessing the configurations when running the tests. That was the motivation we did in camel-quarkus-kafka extension before. So I think it is only possible to achieve this target both in JVM and Native mode by introducing a BuildItem to produce an additional configuration.

cescoffier · 2022-05-05T05:47:33Z

@zhfeng I do not follow. First, dev services are not about reducing the configuration, but about running infrastructure services automatically.
Then, why can't you in the camel-quarkus-kafka define the property as mentioned above?

zhfeng · 2022-05-05T05:53:34Z

@cescoffier yeah, we can define the property but kafka.bootstrap.servers is only availiable when kafka dev service is enabled. We have to run the tests in different environments (dev services enabled and disabled).

cescoffier · 2022-05-05T06:05:47Z

This property is set by the dev service, but is used every time. So in prod mode, the user would have to set it (or use SBO).

zhfeng · 2022-05-05T06:13:26Z

Sorry, what is SBO ?

cescoffier · 2022-05-08T11:32:42Z

Service Binding Operator

zhfeng · 2022-05-09T01:12:51Z

Thanks all for reviewing. I'd like to close this PR and follow @cescoffier recommendation to set the property in prod mode.

Fix quarkusio#25094 Introduce a BuildItem in DevKafkaService to add a…

a6ae506

…dditional configs

quarkus-bot bot added the area/kafka label Apr 25, 2022

zhfeng changed the title ~~Fix #25094 Introduce a BuildItem in DevKafkaService to add additional…~~ Introduce a BuildItem in DevKafkaService to add additional config Apr 25, 2022

gastaldi reviewed Apr 25, 2022

View reviewed changes

...ent/src/main/java/io/quarkus/kafka/client/deployment/DevServiceKafkaAdditionalBuildItem.java Show resolved Hide resolved

gastaldi reviewed Apr 25, 2022

View reviewed changes

geoand requested a review from ozangunalp April 26, 2022 06:08

ozangunalp reviewed Apr 26, 2022

View reviewed changes

zhfeng closed this May 9, 2022

quarkus-bot bot added the triage/invalid This doesn't seem right label May 9, 2022

zhfeng mentioned this pull request May 9, 2022

URL to the Kafka brokers must be configured with the brokers option in native mode (continued) apache/camel-quarkus#3742

Closed

zhfeng mentioned this pull request Jun 8, 2022

URL to the Kafka brokers must be configured with the brokers option after upgrade to Quarkus 2.1.0.Final apache/camel-quarkus#2939

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a BuildItem in DevKafkaService to add additional config #25140

Introduce a BuildItem in DevKafkaService to add additional config #25140

zhfeng commented Apr 25, 2022 •

edited

Loading

quarkus-bot bot commented Apr 25, 2022

gastaldi Apr 25, 2022

zhfeng Apr 26, 2022

zhfeng Apr 26, 2022

gastaldi Apr 26, 2022

zhfeng Apr 26, 2022

ozangunalp left a comment

zhfeng commented Apr 26, 2022

ozangunalp commented Apr 26, 2022

geoand commented Apr 26, 2022

zhfeng commented Apr 26, 2022

zhfeng commented Apr 28, 2022

ozangunalp commented Apr 28, 2022

zhfeng commented Apr 28, 2022

ozangunalp commented Apr 29, 2022

zhfeng commented Apr 29, 2022

cescoffier commented May 1, 2022

zhfeng commented May 5, 2022

cescoffier commented May 5, 2022

zhfeng commented May 5, 2022

cescoffier commented May 5, 2022

zhfeng commented May 5, 2022

cescoffier commented May 8, 2022

zhfeng commented May 9, 2022

Introduce a BuildItem in DevKafkaService to add additional config #25140

Introduce a BuildItem in DevKafkaService to add additional config #25140

Conversation

zhfeng commented Apr 25, 2022 • edited Loading

quarkus-bot bot commented Apr 25, 2022

gastaldi Apr 25, 2022

Choose a reason for hiding this comment

zhfeng Apr 26, 2022

Choose a reason for hiding this comment

zhfeng Apr 26, 2022

Choose a reason for hiding this comment

gastaldi Apr 26, 2022

Choose a reason for hiding this comment

zhfeng Apr 26, 2022

Choose a reason for hiding this comment

ozangunalp left a comment

Choose a reason for hiding this comment

zhfeng commented Apr 26, 2022

ozangunalp commented Apr 26, 2022

geoand commented Apr 26, 2022

zhfeng commented Apr 26, 2022

zhfeng commented Apr 28, 2022

ozangunalp commented Apr 28, 2022

zhfeng commented Apr 28, 2022

ozangunalp commented Apr 29, 2022

zhfeng commented Apr 29, 2022

cescoffier commented May 1, 2022

zhfeng commented May 5, 2022

cescoffier commented May 5, 2022

zhfeng commented May 5, 2022

cescoffier commented May 5, 2022

zhfeng commented May 5, 2022

cescoffier commented May 8, 2022

zhfeng commented May 9, 2022

zhfeng commented Apr 25, 2022 •

edited

Loading