Skip to content

Conversation

MahsaSeifikar
Copy link
Contributor

@MahsaSeifikar MahsaSeifikar commented Aug 27, 2025

This change adds the metric ControllerEventManager::AvgIdleRatio which
measures the amount of time the controller spends blocked waiting for
events vs the amount of time spent processing events. A value of 1.0
means that the controller spent the entire interval blocked waiting for
events.

Reviewers: José Armando García Sancio [email protected], Kevin Wu
[email protected], Alyssa Huang [email protected], TengYao
Chi [email protected], Reviewers: Chia-Ping Tsai
[email protected]

@github-actions github-actions bot added triage PRs from the community kraft labels Aug 27, 2025
Copy link
Contributor

@frankvicky frankvicky left a comment

Choose a reason for hiding this comment

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

@MahsaSeifikar: Please apply spotless

Copy link
Contributor

@kevin-wu24 kevin-wu24 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @MahsaSeifikar. I have a question about the wrapper classes being introduced:

@github-actions github-actions bot added the small Small PRs label Aug 28, 2025
@github-actions github-actions bot removed the triage PRs from the community label Aug 29, 2025
Copy link
Contributor

@kevin-wu24 kevin-wu24 left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @MahsaSeifikar

Two more comments:

Copy link
Contributor

@kevin-wu24 kevin-wu24 left a comment

Choose a reason for hiding this comment

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

Changes LGTM @MahsaSeifikar.

@MahsaSeifikar
Copy link
Contributor Author

@chia7712 Could you please review this metric KIP when you get a chance?

@frankvicky frankvicky requested a review from chia7712 September 3, 2025 14:40
Copy link
Contributor

@ahuang98 ahuang98 left a comment

Choose a reason for hiding this comment

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

what do you think about moving updateIdleStartTime into updateEventQueueProcessingTime? you would need to call it in one other place outside of that.

otherwise looks reasonable to me, thanks Mahsa!

Copy link
Member

@jsancio jsancio left a comment

Choose a reason for hiding this comment

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

Thanks for the change @MahsaSeifikar . I left some high-level comments and observations.

@github-actions github-actions bot added the core Kafka Broker label Sep 10, 2025
Copy link
Member

@jsancio jsancio left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @MahsaSeifikar

@github-actions github-actions bot removed the small Small PRs label Sep 15, 2025
@MahsaSeifikar
Copy link
Contributor Author

This AclControlManagerTest.testDeleteExceedsMaxRecords() failure seems unrelated to this PR and has shown flaky behavior in the past 28 days, link.

Here is the ticket KAFKA-19513 to fix this.

Copy link
Member

@jsancio jsancio left a comment

Choose a reason for hiding this comment

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

LGTM

@MahsaSeifikar MahsaSeifikar requested a review from jsancio October 1, 2025 16:52
@MahsaSeifikar
Copy link
Contributor Author

MahsaSeifikar commented Oct 1, 2025

This AdjustStreamThreadCountTest.shouldAddAndRemoveThreadsMultipleTimes failure seems unrelated to this PR and has shown flaky behavior in the past 2 days, link.

Here is the pr to fix this.

Copy link
Member

@jsancio jsancio left a comment

Choose a reason for hiding this comment

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

LGTM

@jsancio
Copy link
Member

jsancio commented Oct 2, 2025

Test failures seem unrelated to this change:

Found 1 test failures:
FAILED ❌ AdjustStreamThreadCountTest > shouldAddAndRemoveThreadsMultipleTimes()
Found 1 flaky test failures:
FLAKY ⚠️ DescribeStreamsGroupTest > testDescribeStreamsGroupWithShortTimeout()

@jsancio jsancio merged commit 8468317 into apache:trunk Oct 2, 2025
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants