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.

Sign up for GitHub

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

Jump to bottom

KAFKA-17031: Make RLM thread pool configurations public #17499

Merged
merged 6 commits into from
Oct 21, 2024
Merged

KAFKA-17031: Make RLM thread pool configurations public #17499

merged 6 commits into from
Oct 21, 2024

Conversation

fvaleri
Copy link
Contributor

@fvaleri fvaleri commented Oct 14, 2024 •
edited
Loading

According to KIP-950, remote.log.manager.thread.pool.size should be marked as deprecated and replaced by two new configurations: remote.log.manager.copier.thread.pool.size and remote.log.manager.expiration.thread.pool.size.

@fvaleri
KAFKA-17031: Make RLM thread pool configurations public
b9a465b 
According to KIP-950, remote.log.manager.thread.pool.size should be marked as deprecated and replaced by two
new configurations: remote.log.manager.copier.thread.pool.size and remote.log.manager.expiration.thread.pool.size.

Signed-off-by: Federico Valeri <[email protected]>
@github-actions github-actions bot added storage Pull requests that target the storage module small Small PRs labels Oct 14, 2024
gaurav-narula
gaurav-narula reviewed Oct 14, 2024
View reviewed changes
Copy link
Collaborator

@gaurav-narula gaurav-narula left a comment

Choose a reason for hiding this comment

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

Looks good, a question about the default value though

storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java Show resolved Hide resolved
showuon
showuon approved these changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@showuon showuon left a comment

Choose a reason for hiding this comment

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

LGTM!

satishd
satishd reviewed Oct 17, 2024
View reviewed changes
Copy link
Member

@satishd satishd left a comment

Choose a reason for hiding this comment

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

Thanks @fvaleri for the PR, left a comment.

storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java Show resolved Hide resolved
showuon
showuon approved these changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@showuon showuon left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for tohe fix.

@fvaleri
Copy link
Contributor Author

fvaleri commented Oct 17, 2024 •
edited
Loading

The default value of -1 means that this will be set to the configured value of remote.log.manager.thread.pool.size, if available; otherwise, it defaults to 10

This is a bit confusing IMO. What if the user sets -2? I think that we can use -1 as default, and fallback when value is <=0. Wdyt? I also have a test to fix.

@fvaleri
Fix testFromPropsInvalid
af4792e 
Signed-off-by: Federico Valeri <[email protected]>
@github-actions github-actions bot added the core Kafka Broker label Oct 17, 2024
@fvaleri fvaleri requested a review from satishd October 17, 2024 11:39
gaurav-narula
gaurav-narula approved these changes Oct 17, 2024
View reviewed changes
Copy link
Collaborator

@gaurav-narula gaurav-narula left a comment

Choose a reason for hiding this comment

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

LGTM

@satishd
Copy link
Member

satishd commented Oct 17, 2024

This is a bit confusing IMO. What if the user sets -2? I think that we can use -1 as default, and fallback when value is <=0. Wdyt? I also have a test to fix.

-2 is invalid value and it will fail. The valid values are -1 and value >0. -1 indicates to be remote.log.manager.thread.pool.size's value if exists else it's default(which is 10)

@fvaleri
Copy link
Contributor Author

fvaleri commented Oct 17, 2024

@satishd thanks for the feedback. Latest commit should address your request.

@fvaleri
Address comment from Satish
106ac74 
Signed-off-by: Federico Valeri <[email protected]>
satishd
satishd reviewed Oct 18, 2024
View reviewed changes
Copy link
Member

@satishd satishd left a comment

Choose a reason for hiding this comment

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

Thanks @fvaleri for the updates. Can you please add validation for these configs(remote.log.manager.copier.thread.pool.size, remote.log.manager.expiration.thread.pool.size) and the respective UTs?

@fvaleri
Add validation
f934ac0 
Signed-off-by: Federico Valeri <[email protected]>
@fvaleri fvaleri requested a review from satishd October 18, 2024 15:52
satishd
satishd reviewed Oct 21, 2024
View reviewed changes
Copy link
Member

@satishd satishd left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @fvaleri for updating the PR. Please address the below items.

  • It seems there are testfailures because of style checks.
  • Add a UT to verify that the copier and expiration threadpools fallsback to the expected default value(10) if not configured.

@fvaleri
Address comment from Satish
8d6eb69 
Signed-off-by: Federico Valeri <[email protected]>
@fvaleri
Copy link
Contributor Author

fvaleri commented Oct 21, 2024

Test failures are unrelated. They work locally and are reported as flaky.

@fvaleri fvaleri requested a review from satishd October 21, 2024 15:40
cmccabe
cmccabe approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@cmccabe cmccabe left a comment

Choose a reason for hiding this comment

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

LGTM

@cmccabe
Copy link
Contributor

cmccabe commented Oct 21, 2024

Since this changes the behavior of the new tiered storage configurations, we will need to get it into 3.9 if it is to get in at all. I will commit it now.

@cmccabe cmccabe merged commit 84ab3b9 into apache:trunk Oct 21, 2024
5 of 6 checks passed
cmccabe pushed a commit that referenced this pull request Oct 21, 2024
@fvaleri @cmccabe
KAFKA-17031: Make RLM thread pool configurations public and fix defau…
7842e25 
…lt handling (#17499)

According to KIP-950, remote.log.manager.thread.pool.size should be marked as deprecated and replaced by two new configurations: remote.log.manager.copier.thread.pool.size and remote.log.manager.expiration.thread.pool.size. Fix default handling so that -1 works as expected.

Reviewers: Luke Chen <[email protected]>, Gaurav Narula <[email protected]>, Satish Duggana <[email protected]>, Colin P. McCabe <[email protected]>
@fvaleri fvaleri deleted the kafka-rlm-pools-doc branch October 22, 2024 07:02
abhishekgiri23 pushed a commit to abhishekgiri23/kafka that referenced this pull request Nov 2, 2024
@fvaleri @agiri23
KAFKA-17031: Make RLM thread pool configurations public and fix defau…
ec14ba2 
…lt handling (apache#17499)

According to KIP-950, remote.log.manager.thread.pool.size should be marked as deprecated and replaced by two new configurations: remote.log.manager.copier.thread.pool.size and remote.log.manager.expiration.thread.pool.size. Fix default handling so that -1 works as expected.

Reviewers: Luke Chen <[email protected]>, Gaurav Narula <[email protected]>, Satish Duggana <[email protected]>, Colin P. McCabe <[email protected]>
@satishd satishd mentioned this pull request Nov 20, 2024
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kamalcph kamalcph kamalcph left review comments

@gaurav-narula gaurav-narula gaurav-narula approved these changes

@showuon showuon showuon approved these changes

@cmccabe cmccabe cmccabe approved these changes

@satishd satishd Awaiting requested review from satishd

Assignees
No one assigned
Labels
ci-approved core Kafka Broker small Small PRs storage Pull requests that target the storage module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@fvaleri @satishd @cmccabe @kamalcph @showuon @gaurav-narula @frankvicky