[v23.2.x] Fix configs returned in CreateTopicsResponse #17241
Merged
Conversation
This file contains 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
This extracts the topic config specific code in describe_configs and moves it into a separate file so that it can later be reused in the CreateTopic handler. (cherry picked from commit 47cf65f)
This adds unit tests for the config response creating methods. These methods grew quite complex with various overloads of the method exhibiting different behaviour, so I've added some tests partly to demonstrate their behaviour and partly to enable a future cleanup of the methods. (cherry picked from commit 773f836)
This is the main change of the PR. It changes the CreateTopics handler to use the shared code of DescribeConfigs to generate the configs returned to the client. We can share this code because the DescribeConfigs object is a more general version of the configs returned in the CreateTopics endpoint, so we can just derive the latter from the former. The reason for changing the CreateTopics handler this way is that currently the CreateTopics handler has a bug where it only ever returns the topic-specific override configs of the topic in the response, whereas it should return all the configs of the topic (incl both the topic-specific override configs and the global broker-level-set topic config defaults). This is the behavour that Apache Kafka exhibits and the behaviour that KIP 525 prescribes. Ref: * https://cwiki.apache.org/confluence/display/KAFKA/KIP-525+-+Return+topic+metadata+and+configs+in+CreateTopics+response (cherry picked from commit 41f9d66)
This removes somee methods in the kafka config handling code that have either now become unused or have already been unused even before the changes in this PR. (cherry picked from commit 1002b63)
This adds a regression test to ensure that the configs returned by describe configs and the configs returned by create topics match. This updates/removes an earlier test which only checked that a single specific config was present. NOTE: the old test I am updating would fail with the current code. That is because the new code returns a different source for the cleanup.policy config, namely DYNAMIC_TOPIC_CONFIG (source=1) instead of DEFAULT_CONFIG (source=5). Apache Kafka (and our DescribeConfigs endpoint) both return source=5 in this case, so I believe this is a bugfix rather than a regression. (cherry picked from commit 5edfb74)
This refactors the config response utility code to use types that are not specific to neither the describe configs nor the create topics handlers. (cherry picked from commit 4f5d6b6)
oleiman
approved these changes
Mar 22, 2024
| @@ -0,0 +1,902 @@ | |||
| /* | |||
There was a problem hiding this comment.
Is the delete_retention_ms vs log_retention_ms just a straight treewide rename? The merge diff is confusing because the string label is "log.retention.ms" in both cases.
There was a problem hiding this comment.
Yes, it looks like it was renamed here: #13257
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Backport of PR #16922
Closes #17238