-
Notifications
You must be signed in to change notification settings - Fork 561
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
Fix configs returned in CreateTopicsResponse #16922
Conversation
config_map_t from_cluster_type(const cluster::topic_properties& properties) { | ||
config_map_t config_entries; | ||
if (properties.compression) { | ||
config_entries[topic_property_compression] = from_config_type( | ||
*properties.compression); | ||
} | ||
if (properties.cleanup_policy_bitflags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that by switching to the method in DescribeConfigs, we’re also removing from the response some configs that are not considered in the now-shared code but were considered previously:
- topic_property_compaction_strategy (compaction.strategy)
- topic_property_recovery (redpanda.remote.recovery)
- topic_property_read_replica (redpanda.remote.readreplica)
However, these configs seem to be Redpanda-specific, they are not returned by Apache Kafka. So I think we are not required to return them, although we could choose to return them later as an improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe they should have always been included in DescribeConfigs
as well
new failures in https://buildkite.com/redpanda/redpanda/builds/45756#018e1592-c383-4678-9688-543ff278ad70:
new failures in https://buildkite.com/redpanda/redpanda/builds/45756#018e1592-c386-47b5-abf5-e4c5cdd4515a:
new failures in https://buildkite.com/redpanda/redpanda/builds/45756#018e1592-c38c-4abd-8e88-b4fa7afac5b8:
new failures in https://buildkite.com/redpanda/redpanda/builds/45756#018e15a3-c7b6-47fa-b1bb-00a096601764:
new failures in https://buildkite.com/redpanda/redpanda/builds/45756#018e15a3-c7c0-48b1-b17f-1da9189af1b1:
new failures in https://buildkite.com/redpanda/redpanda/builds/46050#018e32a8-ccf2-49b8-9026-1a0c2f381599:
new failures in https://buildkite.com/redpanda/redpanda/builds/46050#018e32a8-ccfa-4d50-9ffa-5b74170e3d39:
new failures in https://buildkite.com/redpanda/redpanda/builds/46050#018e32a8-ccf7-4c6c-a848-063b0ac4af91:
new failures in https://buildkite.com/redpanda/redpanda/builds/46050#018e32a8-ccf5-4bcc-9b66-2858941d87d5:
new failures in https://buildkite.com/redpanda/redpanda/builds/46091#018e370c-bca2-481b-a2d6-adacbd62f03b:
new failures in https://buildkite.com/redpanda/redpanda/builds/46091#018e3710-4618-452b-938f-ec1a5063db76:
new failures in https://buildkite.com/redpanda/redpanda/builds/46091#018e370c-bc9b-467a-903c-59f677886738:
new failures in https://buildkite.com/redpanda/redpanda/builds/46091#018e3710-4615-4fdb-ac2a-13d0357d2a3f:
new failures in https://buildkite.com/redpanda/redpanda/builds/46091#018e3710-4611-44e8-b422-bc63990a8511:
new failures in https://buildkite.com/redpanda/redpanda/builds/46091#018e3710-460d-41a0-887b-9b801db60757:
new failures in https://buildkite.com/redpanda/redpanda/builds/46091#018e370c-bc98-4767-9bea-d45382507ec5:
new failures in https://buildkite.com/redpanda/redpanda/builds/46091#018e370c-bc9f-469c-9a3c-2269b34762e3:
new failures in https://buildkite.com/redpanda/redpanda/builds/46175#018e3c82-23b2-4606-9b30-8bebcfcc881e:
new failures in https://buildkite.com/redpanda/redpanda/builds/46175#018e3caa-7084-4505-8b0b-e1a022103223:
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45756#018e1592-c389-4b5d-bd7a-495311754a86 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46091#018e370c-bc9f-469c-9a3c-2269b34762e3 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46175#018e3c82-23b9-4ff2-8f29-b72f9f303fe2 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46292#018e4376-5217-4e64-8fe3-11c3e1a04f5c ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46292#018e437b-169e-4f40-bf7f-255cee95ed9c ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46356#018e5169-2dc6-4e9e-8338-db947763b450 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46356#018e5192-e38f-4a28-afeb-522fa0417b01 |
31e603a
to
45cbc22
Compare
I force-pushed now to remove the last commit which changes the source returned for the |
: std::nullopt; | ||
} | ||
|
||
static inline void report_topic_config( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do want to in future commits clean up this so that we don't:
- Include many of these helper functions in the header
- Remove as many templates as we can from the header
- Expose one method that returns the configs and put the implementation in another source file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think the API to this shouldn't include the describe_configs_result
type, that type should only be referenced from describe_configs.cc
. Maybe we can make a new type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I've separated now the header and the implementation and introduced generic types for the helpers here.
static inline std::vector<creatable_topic_configs> make_configs( | ||
const cluster::metadata_cache& metadata_cache, | ||
const cluster::topic_configuration& cfg) { | ||
describe_configs_resource resource{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a new generic type that isn't request specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made this change now: ac39a7c
@@ -225,147 +225,4 @@ to_cluster_type(const creatable_topic& t) { | |||
return ret; | |||
} | |||
|
|||
template<typename T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a little confusing to see this big delete in a test/ commit. Just a nit, I personally would make the change that renders this block of code unused first, then in a follow up commit, deleting all now unused code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made the code deletion into a separate commit now.
} | ||
|
||
template<typename T> | ||
static inline void add_config( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove static
from all the functions in this header, and also inline
from any that are template functions. non-template free functions should have inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I've moved the code to a .cc file instead of the header, I left the inline
-ness of the code as it was previous to my PR, so I think this comment is outdated now.
@@ -0,0 +1,771 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
it is disappointing that we are moving all this code into a header. do you think we could leave the code in the .cc file and then move whatever is going to use it over to the .cc file too? it's ok if it that's not feasible or desired!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've refactored the changes now so that most of the code is in a .cc file.
@graphcareful @dotnwat the problem with moving the implementation from header methods to a .cc file is that it makes it much harder to test the code in
Am I missing an option here? Is there a better way to go about this? As I see it this comes down to a header file size vs test coverage trade-off. What do you think? |
@pgellert - one option here would be to put concrete proxy function declarations[1] in a separate header in the test directory, then put the implementations directly in [1] - for the specific // kafka/server/tests/config_response_utils_test_help.h
namespace kafka::test {
void add_topic_config_if_requested_tristate_int(
const describe_configs_resource& resource,
describe_configs_result& result,
std::string_view default_name,
const std::optional<int>& default_value,
std::string_view override_name,
const tristate<int>& overrides,
bool include_synonyms,
std::optional<ss::sstring> documentation);
} or something like that. whether it's a good idea or worth the hassle is probably debatable, but small headers and test coverage are both good goals IMO. |
75ede98
to
0667fb9
Compare
Thanks @oleiman, that's what I was missing. :) I've made the change now to have the unit tests build using a separate header while keeping the production header minimal. Let me know if you have any feedback on that part. In the meantime, I'm still working on the address the feedback to introduce a custom config type to make the methods in |
a56a536
to
6144b2e
Compare
Force-pushed to clean up the commit history |
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.
6144b2e
to
c4c04df
Compare
Force-pushed to rebase to dev as there were some conflicts with recent changes on dev |
c4c04df
to
6618b69
Compare
Force-pushed to fix a linking error with the |
6618b69
to
a3efdee
Compare
Force-pushed to fix a copy-paste error that was causing a bunch of ducktape tests to fail + made the move semantics more explicit. |
std::vector<creatable_topic_configs> make_configs( | ||
const cluster::metadata_cache& metadata_cache, | ||
const cluster::topic_properties& topic_config) { | ||
describe_configs_resource resource{}; | ||
describe_configs_result describe_result{}; | ||
|
||
report_topic_config( | ||
resource, describe_result, metadata_cache, topic_config, false, false); | ||
|
||
std::vector<creatable_topic_configs> result; | ||
result.reserve(describe_result.configs.size()); | ||
|
||
for (auto& describe_conf : describe_result.configs) { | ||
result.push_back(creatable_topic_configs{ | ||
.name = std::move(describe_conf.name), | ||
.value = std::move(describe_conf.value), | ||
.config_source = describe_conf.config_source, | ||
}); | ||
} | ||
|
||
return result; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a fragmented vector cc @BenPope ? Worrying about the 1k topics create overallocation issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that code was just copy-pasta'ed I think we should just keep it as is for now, since 95% of this PR is mainly moving around code so that it can invoked in two distinct places.
If we decide to change that it should be in a follow up PR, IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised it didn't conflict with a change I made in #16982, but it looks like I missed this. Yeah, I think it should be chunked_vector
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created an issue to take this up as a follow-up: https://github.com/redpanda-data/core-internal/issues/1174
47f01a1
to
10e6f30
Compare
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.
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
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.
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.
This refactors the config response utility code to use types that are not specific to neither the describe configs nor the create topics handlers.
10e6f30
to
4f5d6b6
Compare
Force-pushed to add the missing copyright header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
/backport v23.3.x |
/backport v23.2.x |
Failed to create a backport PR to v23.3.x branch. I tried:
|
Failed to create a backport PR to v23.2.x branch. I tried:
|
This PR fixes a bug where the Kafka API of redpanda is incorrectly returning only a subset of the configs in CreateTopics responses, whereas it is supposed to return all of the configs of the newly created topic.
This is because the CreateTopics handler only returns configs that are topic-level overrides - those specific to the topic itself - but doesn't return topic configs that are inherited from the global defaults.
This PR fixes this by using the fact that the DescribeConfigs endpoint already correctly combines the topic-specific overrides and the global defaults. It reuses the existing code in DescribeConfigs for the CreateTopics handler.
Fixes: #16578
Backports Required
Release Notes
Bug Fixes