KAFKA-14039 Fix AlterConfigPolicy usage in KRaft #12374
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
@mumrah Thanks for the patch. Are there any integration tests we can add to |
Member
Author
|
@hachikuji I added coverage for the new behavior in the test you mentioned. The test will first set a regular broker config, and later verify that it isn't passed to the policy when we're modifying other configs for the broker resource. |
mumrah
added a commit
that referenced
this pull request
Jul 26, 2022
Only pass configs from the request to the AlterConfigPolicy. This changes the KRaft usage of the AlterConfigPolicy to match the usage in ZK mode. Reviewers: Jason Gustafson <jason@confluent.io>
3 tasks
cmccabe
pushed a commit
that referenced
this pull request
Sep 1, 2022
…12578) #12374 adjusted the invocation of the alter configs policy check in KRaft to match the behavior in ZooKeeper, which is to only provide the configs that were explicitly sent in the request. While the code was correct for the incremental alter configs case, the code actually included the implicit deletions for the legacy/non-incremental alter configs case, and those implicit deletions are not included in the ZooKeeper-based invocation. This patch adds a test to check for this and adjusts ConfigurationControlManager code so that the test passes -- the adjusted test is confirmed to fail locally otherwise. We also add a log statement to emit any unexpected stack traces in the alter config code path. Reviewers: José Armando García Sancio <jsancio@gmail.com>, Colin P. McCabe <cmccabe@apache.org>
cmccabe
pushed a commit
that referenced
this pull request
Sep 1, 2022
…12578) #12374 adjusted the invocation of the alter configs policy check in KRaft to match the behavior in ZooKeeper, which is to only provide the configs that were explicitly sent in the request. While the code was correct for the incremental alter configs case, the code actually included the implicit deletions for the legacy/non-incremental alter configs case, and those implicit deletions are not included in the ZooKeeper-based invocation. This patch adds a test to check for this and adjusts ConfigurationControlManager code so that the test passes -- the adjusted test is confirmed to fail locally otherwise. We also add a log statement to emit any unexpected stack traces in the alter config code path. Reviewers: José Armando García Sancio <jsancio@gmail.com>, Colin P. McCabe <cmccabe@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The existing AlterConfigPolicy behavior in ZK clusters is to only validate configs that have been included into the IncrementalAlterConfigs request. While this could be considered a bug, we should make KRaft have the same behavior as ZK with regards to pluggable interfaces like this.
This patch changes the behavior of ConfigurationControlManager to only pass configs to the AlterConfigPolicy that were included in the request. The full set of configs is still passed through the ConfigurationValidator defined by the controller, but only altered configs are passed down to the AlterConfigPolicy.