Skip to content

KAFKA-15129;[1/N] Remove metrics in LogCleanerManager when LogCleaner shutdown#13924

Merged
divijvaidya merged 4 commits intoapache:trunkfrom
hudeqi:clean_metric_
Jul 3, 2023
Merged

KAFKA-15129;[1/N] Remove metrics in LogCleanerManager when LogCleaner shutdown#13924
divijvaidya merged 4 commits intoapache:trunkfrom
hudeqi:clean_metric_

Conversation

@hudeqi
Copy link
Copy Markdown
Contributor

@hudeqi hudeqi commented Jun 28, 2023

This pr is used to remove the metrics in LogCleanerManager when logCleaner is closed.
This pr has passed the corresponding unit test, and it is part of KAFKA-15129.

@hudeqi hudeqi marked this pull request as draft June 29, 2023 11:03
@clolov clolov self-requested a review June 29, 2023 12:36
@clolov clolov added the core Kafka Broker label Jun 29, 2023
@hudeqi hudeqi marked this pull request as ready for review June 29, 2023 16:18
@hudeqi
Copy link
Copy Markdown
Contributor Author

hudeqi commented Jun 29, 2023 •
edited
Loading

I have submitted some PRs (two others: https://github.com/apache/kafka/pull/13926、https://github.com/apache/kafka/pull/13929) on the topic of "clean metric", please help to review, thank you! @clolov @divijvaidya

clolov
clolov approved these changes Jun 30, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@clolov clolov left a comment

Choose a reason for hiding this comment

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

Beyond wanting the variable names to start with lower-case unless there is a good reason to not do so, I am happy with this!

clolov
clolov reviewed Jun 30, 2023
View reviewed changes
Comment thread core/src/main/scala/kafka/log/LogCleanerManager.scala
divijvaidya
divijvaidya reviewed Jun 30, 2023
View reviewed changes
Comment thread core/src/main/scala/kafka/log/LogCleanerManager.scala Outdated
Comment thread core/src/test/scala/unit/kafka/log/LogCleanerTest.scala Outdated
@hudeqi hudeqi requested a review from divijvaidya June 30, 2023 14:41
divijvaidya
divijvaidya approved these changes Jun 30, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

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

Looks good! I will wait for CI to complete before we merge this.

@hudeqi
Copy link
Copy Markdown
Contributor Author

hudeqi commented Jul 3, 2023

Hi, can we merge this and review the next three PR of the topic? @divijvaidya

@divijvaidya
Copy link
Copy Markdown
Member

divijvaidya commented Jul 3, 2023

Unrelated test failures, merging this in. @hudeqi I have rest of the PRs in my queue and will reach to them in a few days.

[Build / JDK 20 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.IdentityReplicationIntegrationTest.testOffsetTranslationBehindReplicationFlow()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13924/5/testReport/junit/org.apache.kafka.connect.mirror.integration/IdentityReplicationIntegrationTest/Build___JDK_20_and_Scala_2_13___testOffsetTranslationBehindReplicationFlow__/)
[Build / JDK 20 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testOffsetTranslationBehindReplicationFlow()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13924/5/testReport/junit/org.apache.kafka.connect.mirror.integration/MirrorConnectorsIntegrationExactlyOnceTest/Build___JDK_20_and_Scala_2_13___testOffsetTranslationBehindReplicationFlow__/)
[Build / JDK 20 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testOffsetTranslationBehindReplicationFlow()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13924/5/testReport/junit/org.apache.kafka.connect.mirror.integration/MirrorConnectorsIntegrationExactlyOnceTest/Build___JDK_20_and_Scala_2_13___testOffsetTranslationBehindReplicationFlow___2/)
[Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testOffsetTranslationBehindReplicationFlow()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13924/5/testReport/junit/org.apache.kafka.connect.mirror.integration/MirrorConnectorsIntegrationExactlyOnceTest/Build___JDK_17_and_Scala_2_13___testOffsetTranslationBehindReplicationFlow__/)
[Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testOffsetTranslationBehindReplicationFlow()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13924/5/testReport/junit/org.apache.kafka.connect.mirror.integration/MirrorConnectorsIntegrationExactlyOnceTest/Build___JDK_8_and_Scala_2_12___testOffsetTranslationBehindReplicationFlow__/)
[Build / JDK 8 and Scala 2.12 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13924/5/testReport/junit/org.apache.kafka.controller/QuorumControllerTest/Build___JDK_8_and_Scala_2_12___testBalancePartitionLeaders__/)
[Build / JDK 8 and Scala 2.12 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13924/5/testReport/junit/org.apache.kafka.controller/QuorumControllerTest/Build___JDK_8_and_Scala_2_12___testBalancePartitionLeaders___2/)
[Build / JDK 11 and Scala 2.13 / kafka.admin.TopicCommandIntegrationTest.testDescribeUnderMinIsrPartitionsMixed(String).quorum=zk](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13924/5/testReport/junit/kafka.admin/TopicCommandIntegrationTest/Build___JDK_11_and_Scala_2_13___testDescribeUnderMinIsrPartitionsMixed_String__quorum_zk/)
[Build / JDK 11 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13924/5/testReport/junit/org.apache.kafka.controller/QuorumControllerTest/Build___JDK_11_and_Scala_2_13___testBalancePartitionLeaders__/)
[Build / JDK 11 and Scala 2.13 / org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13924/5/testReport/junit/org.apache.kafka.trogdor.coordinator/CoordinatorTest/Build___JDK_11_and_Scala_2_13___testTaskRequestWithOldStartMsGetsUpdated__/)

@divijvaidya divijvaidya merged commit 48eb8c9 into apache:trunk Jul 3, 2023
@hudeqi hudeqi deleted the clean_metric_ branch July 3, 2023 22:12
kamalcph
kamalcph reviewed Jul 4, 2023
View reviewed changes
Comment thread core/src/main/scala/kafka/log/LogCleanerManager.scala
@hudeqi hudeqi restored the clean_metric_ branch July 4, 2023 11:56
@hudeqi hudeqi mentioned this pull request Jul 5, 2023
Merged
dajac
dajac reviewed Jul 6, 2023
View reviewed changes
Comment thread core/src/main/scala/kafka/log/LogCleanerManager.scala
divijvaidya pushed a commit that referenced this pull request Jul 7, 2023
@hudeqi
MINOR: Fix regression introduced in #13924 (#13958)
574f394 
Fixes a regression introduced in PR #13924 by moving the map from static to a instance specific variable.
---------

Co-authored-by: Deqi Hu <deqi.hu@shopee.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@divijvaidya divijvaidya divijvaidya approved these changes

@dajac dajac dajac left review comments

@kamalcph kamalcph kamalcph left review comments

@clolov clolov clolov approved these changes

Assignees

No one assigned

Labels

core Kafka Broker

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hudeqi @divijvaidya @dajac @kamalcph @clolov