Skip to content

Do not send "Create Topic req" when topic is already present #4402

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Do not send "Create Topic req" when topic is already present #4402

wants to merge 1 commit into from
+28 −11

Conversation

matzew
Copy link
Contributor

@matzew matzew commented Jun 4, 2025 •
edited
Loading

Fixes #4406

Proposed Changes

  • adding new IsTopicNotReady
  • adding checks on reconcilers for this

Release Note

Docs

@knative-prow knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/control-plane size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 4, 2025
@knative-prow Knative Prow
Copy link

knative-prow bot commented Jun 4, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2025
@knative-prow knative-prow bot requested review from aliok and creydr June 4, 2025 10:21
@codecov Codecov
Copy link

codecov bot commented Jun 4, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 42.85714% with 8 lines in your changes missing coverage. Please review.

Project coverage is 29.97%. Comparing base (f4c76b5) to head (e0d227f).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...lane/pkg/reconciler/base/receiver_condition_set.go 0.00% 8 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4402      +/-   ##
==========================================
- Coverage   29.97%   29.97%   -0.01%     
==========================================
  Files         290      290              
  Lines       19363    19372       +9     
==========================================
+ Hits         5805     5806       +1     
- Misses      13104    13112       +8     
  Partials      454      454

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@matzew matzew force-pushed the IsTopicNotReady branch from e0d227f to 5b9473b Compare June 5, 2025 15:18
@knative-prow knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 5, 2025
@matzew
Copy link
Contributor Author

matzew commented Jun 6, 2025

/retest-required

@matzew matzew changed the title WIP: Do not send "Create Topic req" when topic is already present Do not send "Create Topic req" when topic is already present Jun 6, 2025
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 6, 2025
@matzew
Copy link
Contributor Author

matzew commented Jun 6, 2025

/retest

pierDipi
pierDipi reviewed Jun 9, 2025
View reviewed changes
control-plane/pkg/reconciler/broker/broker.go
Comment on lines +342 to +346
if statusConditionManager.IsTopicNotReady() {
topic, err := kafka.CreateTopicIfDoesntExist(kafkaClusterAdminClient, logger, topicName, topicConfig)
if err != nil {
return "", statusConditionManager.FailedToCreateTopic(topic, err)
}
Copy link
Member

Choose a reason for hiding this comment

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

note that conditions are reset on every reconciliation https://github.com/knative/pkg/blob/3c3a920206ea7ff7739c170f56746ea518f192f3/reconciler/reconcile_common.go#L63-L66

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed a huge reduction of "create topic" requests, with this PR

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a way to work around this would be to update the CreateTopicIfDoesntExist function to check if the topic exists before sending a create topic request?

As far as I can tell, the reason for so many requests is that the create topic method doesn't actually check if the topic exists before sending the request, so we could add that logic here:

eventing-kafka-broker/control-plane/pkg/kafka/topic.go

Lines 179 to 183 in db1e609

createTopicError := admin.CreateTopic(topic, &config.TopicDetail, false)
if err, ok := createTopicError.(*sarama.TopicError); ok && err.Err == sarama.ErrTopicAlreadyExists {
return topic, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the list topics would still send a request to kafka brokers

@matzew
Copy link
Contributor Author

matzew commented Jun 10, 2025

/retest

@matzew matzew force-pushed the IsTopicNotReady branch from 5b9473b to 6bbb92d Compare June 12, 2025 07:33
@matzew
Copy link
Contributor Author

matzew commented Jun 12, 2025

/retest-required

@matzew
In sink, channel and broker check if the topic is already present, to…
202223e 
… avoid unneeded network calls (e.g. for Create Topic request)

Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew matzew force-pushed the IsTopicNotReady branch from 6bbb92d to 202223e Compare July 9, 2025 05:50
@knative-prow Knative Prow
Copy link

knative-prow bot commented Jul 9, 2025

@matzew: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
unit-tests_eventing-kafka-broker_main 202223e link true /test unit-tests
channel-integration-tests-sasl-ssl_eventing-kafka-broker_main 202223e link true /test channel-integration-tests-sasl-ssl
channel-integration-tests-sasl-plain_eventing-kafka-broker_main 202223e link true /test channel-integration-tests-sasl-plain
channel-integration-tests-ssl_eventing-kafka-broker_main 202223e link true /test channel-integration-tests-ssl
reconciler-tests-keda_eventing-kafka-broker_main 202223e link true /test reconciler-tests-keda
channel-reconciler-tests-ssl_eventing-kafka-broker_main 202223e link true /test channel-reconciler-tests-ssl
channel-reconciler-tests-sasl-ssl_eventing-kafka-broker_main 202223e link true /test channel-reconciler-tests-sasl-ssl
channel-reconciler-tests-sasl-plain_eventing-kafka-broker_main 202223e link true /test channel-reconciler-tests-sasl-plain
integration-tests_eventing-kafka-broker_main 202223e link true /test integration-tests
reconciler-tests_eventing-kafka-broker_main 202223e link true /test reconciler-tests
reconciler-tests-namespaced-broker_eventing-kafka-broker_main 202223e link true /test reconciler-tests-namespaced-broker
upgrade-tests_eventing-kafka-broker_main 202223e link true /test upgrade-tests

Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierDipi pierDipi pierDipi left review comments

@Cali0707 Cali0707 Cali0707 left review comments

@aliok aliok Awaiting requested review from aliok

@creydr creydr Awaiting requested review from creydr

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

new Kafka Broker/Channel/Sink requests "CreateTopics" n times
3 participants
@matzew @pierDipi @Cali0707