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

KAFKA-15776: Support added to update remote.fetch.max.wait.ms dynamically #16203

Merged
merged 4 commits into from
Jun 10, 2024

Conversation

kamalcph
Copy link
Contributor

@kamalcph kamalcph commented Jun 5, 2024

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@kamalcph kamalcph added the tiered-storage Related to the Tiered Storage feature label Jun 5, 2024
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.

Thanks for the PR. Left some minor comments.

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!

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 @kamalcph for the PR, overall LGTM. Left a minor comment in the test.

assertThrows(classOf[ConfigException], () => config.dynamicConfig.validate(newProps, perBrokerConfig = false))
// invalid value "0"
newProps.put(RemoteLogManagerConfig.REMOTE_FETCH_MAX_WAIT_MS_PROP, "0")
assertThrows(classOf[ConfigException], () => config.dynamicConfig.validate(newProps, perBrokerConfig = true))
Copy link
Member

Choose a reason for hiding this comment

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

Can you also check perBrokerConfig = false for value 0 like you did for negative value -1 in the earlier code?

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 @kamalcph for addressing the review comments. LGTM.

@satishd
Copy link
Member

satishd commented Jun 10, 2024

A few test failures that are unrelated to this change, will merge it to trunk and 3.8 branches.

@satishd satishd merged commit f359908 into apache:trunk Jun 10, 2024
1 check failed
@satishd
Copy link
Member

satishd commented Jun 10, 2024

@kamalcph Can you raise a PR against 3.8 branch as I would like those changes to be reviewed in PR as it has conflicts with that branch?

@kamalcph kamalcph deleted the KAFKA-15776a branch June 11, 2024 04:18
kamalcph added a commit to kamalcph/kafka that referenced this pull request Jun 11, 2024
@kamalcph
Copy link
Contributor Author

@kamalcph Can you raise a PR against 3.8 branch as I would like those changes to be reviewed in PR as it has conflicts with that branch?

Opened #16275 and #16276 to cherry-pick the KAFKA-15776 feature to the 3.8 branch. PTAL.

satishd pushed a commit that referenced this pull request Jun 11, 2024
gongxuanzhang pushed a commit to gongxuanzhang/kafka that referenced this pull request Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tiered-storage Related to the Tiered Storage feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants