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

Return accurate partition count and replication factor in CreateTopics response #16410

Merged
merged 6 commits into from
Feb 2, 2024

Conversation

oleiman
Copy link
Member

@oleiman oleiman commented Jan 31, 2024

This commit makes some changes to the create_topics path to ensure that partition count and replication factor of created (or already existing) topics are correctly populated in responses.

Includes a small change to ensure that topic configs are serialized correctly with respect to existing formatters (e.g. the default cleanup.policy should appear in create topics results as "delete" where previously it was "1").

Fixes #15722
Fixes #7946

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
  • v23.1.x

Release Notes

Bug Fixes

  • Fix an issue where create topics responses would show incorrect partition count and replication factor

@oleiman oleiman self-assigned this Jan 31, 2024
@oleiman
Copy link
Member Author

oleiman commented Jan 31, 2024

/dt

@oleiman oleiman marked this pull request as ready for review January 31, 2024 21:37
@oleiman oleiman changed the title Bug/15722/create topics response Return accurate partition count and replication factor in CreateTopics response Jan 31, 2024
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Feb 1, 2024

new failures in https://buildkite.com/redpanda/redpanda/builds/44558#018d61b4-0b8e-40e9-be5a-8fd15fe69f8d:

"rptest.tests.controller_upgrade_test.ControllerUpgradeTest.test_updating_cluster_when_executing_operations"

new failures in https://buildkite.com/redpanda/redpanda/builds/44558#018d6558-10ae-4a38-8f18-e619bf5cf974:

"rptest.tests.controller_upgrade_test.ControllerUpgradeTest.test_updating_cluster_when_executing_operations"

@vbotbuildovich
Copy link
Collaborator

@oleiman
Copy link
Member Author

oleiman commented Feb 1, 2024

src/v/cluster/topics_frontend.cc Outdated Show resolved Hide resolved
src/v/kafka/server/handlers/topics/types.h Outdated Show resolved Hide resolved
src/v/kafka/server/handlers/topics/types.cc Show resolved Hide resolved
tests/rptest/tests/topic_creation_test.py Outdated Show resolved Hide resolved
Specifically we noticed that cleanup.policy was populated with a stringified
int in CreateTopics results. We implement formatters for several topic config
types, so we should use them.

Using e.g. "delete" for the cleanup policy value (rather than "1") matches
the behavior of Apache Kafka.

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
@oleiman oleiman force-pushed the bug/15722/create-topics-response branch from 44f8e9a to ba3b786 Compare February 1, 2024 20:10
Copy link
Contributor

@michael-redpanda michael-redpanda left a comment

Choose a reason for hiding this comment

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

Just clean up the assert messages and we're gucci. Thanks!

src/v/kafka/server/handlers/create_topics.cc Outdated Show resolved Hide resolved
tests/rptest/tests/topic_creation_test.py Outdated Show resolved Hide resolved
@oleiman oleiman force-pushed the bug/15722/create-topics-response branch from ba3b786 to 8b6dc85 Compare February 1, 2024 21:53
@oleiman
Copy link
Member Author

oleiman commented Feb 1, 2024

force push contents

  • improve assertions
  • add pc and rf to audit failed responses
  • incorporate validate_only changes because everything is less invasive now

@oleiman
Copy link
Member Author

oleiman commented Feb 1, 2024

@graphcareful @michael-redpanda - Sorry, needs a bit more review. Pulled in the commits for #7946 because the scope of the changes became much smaller but the DT tests are still dependent on this one landing.

To amend create_topics responses with the partition count and replication
factor of the created (or already existing) topic.

Values pulled from metadata cache (similar to append_topic_configs)

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
- If the topic already exists, use that return code (36)
- If the topic does not exist, return the partition count and replication
  factor from the request.
- Responses now match Apache Kafka
Adapter for raw_create_topics that takes a list of client-supplied
dicts, packs them up in raw requests, and issues a create request.
Loads the result into a dict, extracts 'Topics' field, and returns
the result (or empty list if something went wrong).

Also adds a validate_only bool param to raw_create_topics

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Test for correct config serialization and treatment of specific and defaulted
partition count and replication factor values in responses.

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
@oleiman oleiman force-pushed the bug/15722/create-topics-response branch from 8b6dc85 to 0654662 Compare February 2, 2024 02:42
@oleiman
Copy link
Member Author

oleiman commented Feb 2, 2024

force push remove add_topic_properties call from audit failure path

Copy link
Contributor

@michael-redpanda michael-redpanda left a comment

Choose a reason for hiding this comment

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

lgtm, just one question

Comment on lines +270 to +271
result.num_partitions = t.num_partitions;
result.replication_factor = t.replication_factor;
Copy link
Contributor

Choose a reason for hiding this comment

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

question: just to confirm, if the topic already exists, we do not return that topics partition count or RF?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. That's what the latest Kafka does.

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

@oleiman oleiman merged commit 2c37cfd into redpanda-data:dev Feb 2, 2024
18 checks passed
@vbotbuildovich
Copy link
Collaborator

/backport v23.3.x

@vbotbuildovich
Copy link
Collaborator

/backport v23.2.x

@vbotbuildovich
Copy link
Collaborator

/backport v23.1.x

@oleiman oleiman deleted the bug/15722/create-topics-response branch February 2, 2024 17:12
@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v23.1.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-16410-v23.1.x-478 remotes/upstream/v23.1.x
git cherry-pick -x 59b43d37638077d5537c4ec7f2f6160ca13ba59d bf6763d71fca59d9d99b15d176494e84c9d718cc b48d3ac5244b2060f92be4a1ad9dc5e3944f5ef2 4d7a4667bba9784e38af40c2a08030171d83bd37 dad47cb7234b4b45bef695bbfd6f9638a1d1f776 0654662510c94b91665fee75c907ea9a91f17baf

Workflow run logs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants