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

ContainerProperties from KafkaListenerContainerFactory are dropped #1770

Closed
thomasmey opened this issue Apr 15, 2021 · 5 comments · Fixed by #1771
Closed

ContainerProperties from KafkaListenerContainerFactory are dropped #1770

thomasmey opened this issue Apr 15, 2021 · 5 comments · Fixed by #1771

Comments

@thomasmey
Copy link

Hi,

I think the overwritten method here

protected ConcurrentMessageListenerContainer<K, V> createContainerInstance(KafkaListenerEndpoint endpoint) {

has a bug, as that it does not use the already existing container properties from super class org.springframework.kafka.config.AbstractKafkaListenerContainerFactory.containerProperties

So it basically drops any configuration like container clientId (!)

What do you think, bug or feature?

@garyrussell
Copy link
Contributor

If that were the case, nothing would work.

The properties are copied in later; see

protected void initializeContainer(C instance, KafkaListenerEndpoint endpoint) {
ContainerProperties properties = instance.getContainerProperties();
BeanUtils.copyProperties(this.containerProperties, properties, "topics", "topicPartitions", "topicPattern",
"messageListener", "ackCount", "ackTime", "subBatchPerPartition");

@thomasmey
Copy link
Author

Hi,

mhh, I think I see now where the confusion comes from:

when I do:
@Bean public ConcurrentKafkaListenerContainerFactory<String, String> kafkaListenerContainerFactory() { ConcurrentKafkaListenerContainerFactory<String, String> factory = new ConcurrentKafkaListenerContainerFactory<>(); factory.getContainerProperties().setClientId("clientId"); return factory; }
this can not work with the current code. the question is: should this (set a container client id programatically)?

makes sense?

@garyrussell
Copy link
Contributor

My apologies; I see it does NOT work in that case. Investigating...

@garyrussell
Copy link
Contributor

The client Id comes from the endpoint (populated from the @KafkaListener) and overwrites the container properties, even if not set (which is a bug).

If you want to change it programmatically, use a container customizer instead...

factory.setContainerCustomizer(container -> {
	container.getContainerProperties().setClientId("client-for-"
			+ container.getContainerProperties().getTopics()[0]);
});

@garyrussell garyrussell added this to the 2.7.1 milestone Apr 15, 2021
garyrussell added a commit to garyrussell/spring-kafka that referenced this issue Apr 15, 2021
Resolves spring-projects#1770

The clientId property, if set on the container factory, is overwritten
by an empty String if not defined on the annotation.

Check for an empty String (groupId too).

**cherry-pick to 2.6.x**
@thomasmey
Copy link
Author

My apologies; I see it does NOT work in that case. Investigating...

No, worries.
Thanks for looking into it 😃

artembilan pushed a commit that referenced this issue Apr 15, 2021
Resolves #1770

The clientId property, if set on the container factory, is overwritten
by an empty String if not defined on the annotation.

Check for an empty String (groupId too).

**cherry-pick to 2.6.x**

* Use simpler JavaUtils method.
artembilan pushed a commit that referenced this issue Apr 15, 2021
Resolves #1770

The clientId property, if set on the container factory, is overwritten
by an empty String if not defined on the annotation.

Check for an empty String (groupId too).

**cherry-pick to 2.6.x**

* Use simpler JavaUtils method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants