Skip to content

Add support for IdlePartitionEventInterval #28290

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

Closed
wants to merge 5 commits into from

Conversation

pascal-ayotte
Copy link
Contributor

IdlePartitionEventInterval was added, but the value is never retrieved from configuration. Adding support to set the value from configuration. No Issue found related to this.

pascal-ayotte and others added 3 commits October 4, 2021 09:37
IdlePartitionEventInterval was added, but the value is not retrieved from configuration. Adding support to set the value from configuration.
@pivotal-cla
Copy link

@pascal-ayotte Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@pascal-ayotte Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 11, 2021
@wilkinsona
Copy link
Member

Thanks for the pull request, @pascal-ayotte. The changes as currently proposed do not compile as there's no getIdlePartitionEventInterval() event on org.springframework.boot.autoconfigure.kafka.KafkaProperties.Listener.

It looks like support for this was added in Spring Kafka 2.7. @garyrussell do you think it makes sense to add a configuration property for this?

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Oct 11, 2021
@garyrussell
Copy link
Contributor

@wilkinsona It's not a commonly used property; I typically recommend setting such properties in some other bean definition...

@Bean
SomeOtherBean otherBean(ConcurrentKafkaListenerContainerFactory<?, ?> factory() {
    factory.getContainerProperties().set...
    ...
    return someOtherBean;
}

There are many such properties; we typically only expose common properties as Boot properties (similar to the earlier discussions about the many Kafka consumer and producer properties).

OTOH, in the past, we have added them to Boot's properties on-demand when requested (often via user contributions).

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 11, 2021
@wilkinsona
Copy link
Member

Thanks, Gary. This one feels a little bit different to me as there's a setter on container factory whereas other additions (such as #19220) have directly mapped one of our properties to a Kafka property.

It doesn't sound like you have an objection so I think it makes sense to add this one, particularly given the setter in Spring Kafka.

@pascal-ayotte Do you have time to update your proposal to add a private Duration idlePartitionEventInterval field to KafkaProperties and the corresponding getter and setter methods?

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Oct 12, 2021
@garyrussell
Copy link
Contributor

@wilkinsona I have no objections either way (but, for clarity, the setter is not on the factory, but on the factory's containerProperties). The parameter name container is a bit misleading.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 12, 2021
@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Oct 12, 2021
@pascal-ayotte
Copy link
Contributor Author

@wilkinsona thanks for the feedback and sorry for the delay, I'll look into adding the idlePartitionEventInterval field to KafkaProperties and the corresponding getter and setter methods.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 20, 2021
@wilkinsona wilkinsona removed the status: feedback-provided Feedback has been provided label Oct 20, 2021
@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Oct 25, 2021
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Oct 26, 2021
@wilkinsona wilkinsona added this to the 2.7.x milestone Oct 26, 2021
@philwebb philwebb force-pushed the main branch 3 times, most recently from 1ca278f to 902dd0b Compare November 19, 2021 20:17
@snicoll snicoll changed the title Support IdlePartitionEventInterval in config Add support for IdlePartitionEventInterval Jan 3, 2022
@snicoll snicoll self-assigned this Jan 3, 2022
@snicoll snicoll modified the milestones: 2.7.x, 2.7.0-M1 Jan 3, 2022
snicoll pushed a commit that referenced this pull request Jan 3, 2022
snicoll added a commit that referenced this pull request Jan 3, 2022
@snicoll snicoll closed this in a0d4651 Jan 3, 2022
@snicoll
Copy link
Member

snicoll commented Jan 3, 2022

@pascal-ayotte thank you for making your first contribution to Spring Boot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants