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: fix config source for default cleanup.policy config #17456

Merged
merged 2 commits into from
Apr 2, 2024

Conversation

pgellert
Copy link
Contributor

This fixes a bug in the reported config source of cleanup.policy. If I create a topic without a cleanup policy, the default cleanup policy is as configured on the broker (initially, cleanup.policy=delete). Describing a topic's configs shows:

cleanup.policy          delete      DYNAMIC_TOPIC_CONFIG

when it should show

cleanup.policy          delete      DEFAULT_CONFIG

Fixes #2225

Backports Required

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

Release Notes

Bug Fixes

  • Fix reported config source for cleanup.policy by reporting DEFAULT_CONFIG instead of DYNAMIC_TOPIC_CONFIG for the default value.

@pgellert pgellert self-assigned this Mar 28, 2024
@pgellert pgellert requested review from a team, michael-redpanda, ztlpn and graphcareful and removed request for a team March 28, 2024 11:22
@pgellert pgellert force-pushed the default-config-cleanup-policy branch from 33b34fd to 98876be Compare March 28, 2024 16:44
Currently Redpanda returns source=1 (topic-specific overwrite) for the
cleanup.policy config. This is different to Apache Kafka, which returns
source=5 (default config), even though the config value they return
(delete) is the same, so I believe this behaviour is a bug and this
commit fixes it by using the hide_default_override helper field.
@pgellert pgellert force-pushed the default-config-cleanup-policy branch from 98876be to 78fcff7 Compare March 28, 2024 17:04
Comment on lines +140 to +149
# New brokers may not report the version yet, so gather the cluster version
# based on multiple brokers' reported version
versions = list(
set(broker['version'] for broker in brokers_resp
if 'version' in broker))
assert len(versions) == 1, f"Unexpected versions: {versions}"
version_line = versions[0]

self.version = ver_triple(version_line)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Force-pushed to make the version uniformity check here more lenient by allowing missing versions from some nodes. This is to make the test_join_restart_catch_up test less flaky.

Copy link
Contributor

@ztlpn ztlpn left a comment

Choose a reason for hiding this comment

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

test changes LGTM!

@michael-redpanda
Copy link
Contributor

Kicking off a rebuild of CI as something in the infrastructure got messed up

@vbotbuildovich
Copy link
Collaborator

@pgellert pgellert merged commit 61f30f5 into redpanda-data:dev Apr 2, 2024
18 checks passed
@pgellert pgellert deleted the default-config-cleanup-policy branch April 2, 2024 11:27
@r-vasquez
Copy link
Contributor

Can we backport this PR at least to v23.3.x? We would like to have this in the latest versions for our Terraform Provider. 😄

Cc: @michael-redpanda

@oleiman
Copy link
Member

oleiman commented Apr 9, 2024

/backport v23.3.x

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.

Broker-side cleanup.policy should be DEFAULT_CONFIG, not DYNAMIC_TOPIC_CONFIG
7 participants