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: update describe configs version #7543

Merged

Conversation

NyaliaLui
Copy link
Contributor

@NyaliaLui NyaliaLui commented Nov 28, 2022

Cover letter

KIP-569 updated DescribeConfigs to include a documentation string and config type enum in the response. Therefore this PR updates DescribeConfigs to the new API version with the new fields.

Fixes #4170

Changes from force-push 0e814de:

  • Group changes to the schemata and generator.py in the same commit and update that commit message the the sourced Kafka version
  • Use enum naming schema int_type for reserved keywords
  • Use the same integral sizes that Java uses
  • Use base_property.desc() for the documentation text
  • Define the mapping from property types -> describe_config_type. This is for the ConfigType field from KIP-569

Changes from force-push 15ff3eb:

  • Use reflection::is_std_vector
  • Account for unsigned integral types
  • Account for several ADTs such as model::compression
  • Recursively call property_config_type when the type is an optional
  • Pass std::nullopt into the documentation field when include_documentation is unset
  • Remove the make_documentation_string lambda. Instead pass the include_documentation option as an argument
  • Edit alter_configs unit test to use the new fields

Changes from force-push 9296d42:

  • Use a different "unknown" string when printing an unknown type
  • No need to use a temp var for capturing the result of property_config_type
  • Strictly base proeprty_config_type on typename T
  • Use std::optional<std::optional<T>> to capture a tristate<T>
  • Make documentation a std::optional instead of plumbing include_documentation
  • Change comment placements

Changes from force-push 0c539cb:

  • Resolve conflicts

Changes from force-push 7823436:

  • Split property_config_type into property_config_type_by_type and property_config_type_by_name

Changes from force-push 11b5235:

  • Convert std::optional to tristate in the tristate overload of add_topic_config
  • Pass tristate to final version of add_topic_config

Changes from force-push ee38f8c:

  • Add ducktape test using KCL

Changes from force-push bd39be6:

  • Account for disabled state in tristate<T> overrides - this was causing DT tests to fail
  • Account for all fundamental integral types
  • Account for known fundamental char types
  • Account for fixed width types
  • Default integral type is now int

Changes from force-push 16a0ad6:

  • Use constexpr for typetraits instead of a monolith function
  • Check integral types by size

Changes from force-push dc87280:

  • Use return type auto instead of int for num_bits
  • Formatting
  • Add !is_bool<T> to integral type traits

Changes from force-push 9f51c67:

  • Use !is_bool<T> only for the is_short<T> check

Changes from force-push d186c73:

  • Reword the integral type checking to be less confusing: short <= 16bits, 16bits < int <= 32bits, 32bits < long <= 64bits
  • Update comments to include the Java type sizes

Backports Required

  • none - not a bug fix
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v22.3.x
  • v22.2.x
  • v22.1.x

UX Changes

  • DescribeConfigs can now return the config's documentation and indicate the config type

Release Notes

Improvements

  • Updates DescribeConfigs Kafka API to support KIP-569

src/v/kafka/protocol/schemata/generator.py Show resolved Hide resolved
src/v/kafka/protocol/types.h Outdated Show resolved Hide resolved
src/v/kafka/server/handlers/describe_configs.cc Outdated Show resolved Hide resolved
src/v/kafka/protocol/types.h Outdated Show resolved Hide resolved
src/v/kafka/protocol/types.h Outdated Show resolved Hide resolved
src/v/kafka/protocol/types.h Outdated Show resolved Hide resolved
src/v/kafka/protocol/types.h Outdated Show resolved Hide resolved
src/v/kafka/server/handlers/describe_configs.cc Outdated Show resolved Hide resolved
src/v/kafka/types.h Outdated Show resolved Hide resolved
@NyaliaLui NyaliaLui force-pushed the update-describe-configs-version branch from 80390fe to 0e814de Compare November 29, 2022 20:59
@NyaliaLui NyaliaLui force-pushed the update-describe-configs-version branch from 0e814de to 15ff3eb Compare December 2, 2022 05:29
@NyaliaLui NyaliaLui marked this pull request as ready for review December 2, 2022 05:41
@NyaliaLui NyaliaLui requested a review from dlex December 2, 2022 05:42
src/v/kafka/protocol/types.cc Outdated Show resolved Hide resolved
src/v/kafka/server/handlers/describe_configs.cc Outdated Show resolved Hide resolved
src/v/kafka/server/handlers/describe_configs.cc Outdated Show resolved Hide resolved
src/v/kafka/server/handlers/describe_configs.cc Outdated Show resolved Hide resolved
@NyaliaLui NyaliaLui force-pushed the update-describe-configs-version branch from 15ff3eb to 9296d42 Compare December 8, 2022 22:03
@NyaliaLui NyaliaLui force-pushed the update-describe-configs-version branch from 9296d42 to 0c539cb Compare December 9, 2022 15:32
@NyaliaLui NyaliaLui force-pushed the update-describe-configs-version branch 2 times, most recently from 7823436 to 11b5235 Compare December 13, 2022 18:29
Copy link
Contributor

@dlex dlex left a comment

Choose a reason for hiding this comment

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

static_assert needs a change probably otherwise LGTM

src/v/kafka/server/handlers/describe_configs.cc Outdated Show resolved Hide resolved
src/v/kafka/server/handlers/describe_configs.cc Outdated Show resolved Hide resolved
@NyaliaLui NyaliaLui force-pushed the update-describe-configs-version branch from ee38f8c to bd39be6 Compare December 15, 2022 04:31
@NyaliaLui
Copy link
Contributor Author

CI Failures are due to tarball issue and #7788 is the patch for it

Copy link
Contributor

@graphcareful graphcareful left a comment

Choose a reason for hiding this comment

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

All of the changes in describe_configs.cc look great, only comments are on the TMP magic, I think we're good to merge after those are in. Nice work !

src/v/kafka/server/handlers/describe_configs.cc Outdated Show resolved Hide resolved
src/v/kafka/server/handlers/describe_configs.cc Outdated Show resolved Hide resolved
graphcareful
graphcareful previously approved these changes Dec 15, 2022
Copy link
Contributor

@graphcareful graphcareful left a comment

Choose a reason for hiding this comment

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

LGTM

graphcareful
graphcareful previously approved these changes Dec 15, 2022
dlex
dlex previously approved these changes Dec 15, 2022
src/v/kafka/server/handlers/describe_configs.cc Outdated Show resolved Hide resolved
This commit includes a ducktape test that run DescribeConfigs on a topic
with documentation and config type enabled.
@NyaliaLui
Copy link
Contributor Author

NyaliaLui commented Dec 16, 2022

CI Failures are:
#7816
and archival unit test failure that just got a fix merged in #7815

So we should be good to merge when K8s operator finishes

@piyushredpanda piyushredpanda merged commit 9d33915 into redpanda-data:dev Dec 19, 2022
@NyaliaLui NyaliaLui deleted the update-describe-configs-version branch March 2, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Returned config_type in DescribeConfigs is always 0
5 participants