Skip to content

MINOR: Remove the test for ZooKeeper metrics used by ZooKeeperClient#18775

Merged
chia7712 merged 2 commits intoapache:trunkfrom
Yunyung:KAFKA-18435-follow-up
Feb 3, 2025
Merged

MINOR: Remove the test for ZooKeeper metrics used by ZooKeeperClient#18775
chia7712 merged 2 commits intoapache:trunkfrom
Yunyung:KAFKA-18435-follow-up

Conversation

@Yunyung
Copy link
Copy Markdown
Collaborator

@Yunyung Yunyung commented Feb 1, 2025

Description

Remove the test for three metrics already removed in #18450, used only by ZooKeeper, and document them.

  • kafka.server:type=SessionExpireListener,name=SessionState
  • SessionExpireListener,name=ZooKeeperExpiresPerSec
  • SessionExpireListener,name=ZooKeeperDisconnectsPerSec

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@github-actions github-actions Bot added triage PRs from the community core Kafka Broker small Small PRs labels Feb 1, 2025
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Feb 1, 2025

Looks like we removed the tests, but what about the code that is ensuring the test was passing - it doesn't seem to be removed in this PR?

@Yunyung Yunyung changed the title MINOR: Remove ZooKeeper metrics used for ZooKeeperClient MINOR: Remove the test for ZooKeeper metrics used by ZooKeeperClient Feb 1, 2025
@Yunyung
Copy link
Copy Markdown
Collaborator Author

Yunyung commented Feb 1, 2025

The metrics were removed in #18450, but the test and documentation were forgotten.
So this follow-up fixes that.

Also, corrected the wording in this PR to avoid misunderstanding.

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Feb 1, 2025

Why do the test passing though - that seems odd.

@Yunyung
Copy link
Copy Markdown
Collaborator Author

Yunyung commented Feb 1, 2025

Yeah, here: 42ea29c#diff-5bc93ef5ce2e7ef966957f547fe93aa295042c90e92a29d44153e5b434bd952fR271 and 434fe7c#diff-5bc93ef5ce2e7ef966957f547fe93aa295042c90e92a29d44153e5b434bd952fR251, the changes ensure that KRaft has no such metrics and do not test ZooKeeper. So the test always passed before.

Copy link
Copy Markdown
Collaborator

@m1a2st m1a2st 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 this patch, LGTM

@github-actions github-actions Bot removed the triage PRs from the community label Feb 3, 2025
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@Yunyung thanks for this patch. overall LGTM except for one small comment

Comment thread docs/zk2kraft.html
@chia7712 chia7712 merged commit 9ba2621 into apache:trunk Feb 3, 2025
chia7712 pushed a commit to chia7712/kafka that referenced this pull request Feb 3, 2025
…pache#18775)

Reviewers: Ismael Juma <ismael@juma.me.uk>, Ken Huang <s7133700@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
pdruley pushed a commit to pdruley/kafka that referenced this pull request Feb 12, 2025
…pache#18775)

Reviewers: Ismael Juma <ismael@juma.me.uk>, Ken Huang <s7133700@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
manoj-mathivanan pushed a commit to manoj-mathivanan/kafka that referenced this pull request Feb 19, 2025
…pache#18775)

Reviewers: Ismael Juma <ismael@juma.me.uk>, Ken Huang <s7133700@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-approved core Kafka Broker small Small PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants