Skip to content

MINOR:Refactor the metric names into constants in ReplicaManager#13705

Merged
divijvaidya merged 3 commits into
apache:trunkfrom
hudeqi:optimize_metric_match
Jun 17, 2023
Merged

MINOR:Refactor the metric names into constants in ReplicaManager#13705
divijvaidya merged 3 commits into
apache:trunkfrom
hudeqi:optimize_metric_match

Conversation

@hudeqi

@hudeqi hudeqi commented May 11, 2023

Copy link
Copy Markdown
Contributor

Inspired by #13623, the code of the metric in ReplicaManager is optimized (to improve the commit of #13471).

@hudeqi

hudeqi commented May 11, 2023

Copy link
Copy Markdown
Contributor Author

and this, thanks! @divijvaidya @dajac

@kirktrue kirktrue left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mostly reviewing this to learn more about the broker side of things. Seems like a straightforward clean up.

@machi1990 machi1990 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

Comment thread core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala Outdated
@hudeqi
hudeqi force-pushed the optimize_metric_match branch from d3b5c41 to 725dd27 Compare May 16, 2023 02:32
@hudeqi
hudeqi requested a review from divijvaidya May 16, 2023 02:39
Comment thread core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala Outdated
@divijvaidya divijvaidya added the core Kafka Broker label May 19, 2023
@hudeqi
hudeqi requested a review from divijvaidya May 31, 2023 10:13
@divijvaidya

Copy link
Copy Markdown
Member

@hudeqi you requested the review today but I think there are some existing comments above that have not yet been addressed

@hudeqi
hudeqi force-pushed the optimize_metric_match branch from 9470943 to 16e7265 Compare June 14, 2023 12:16
@hudeqi

hudeqi commented Jun 14, 2023

Copy link
Copy Markdown
Contributor Author

Hi, @divijvaidya, it has been updated according to your comment, thank you!

@divijvaidya divijvaidya self-assigned this Jun 14, 2023

@divijvaidya divijvaidya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me. One minor thing is to update the PR name (since we use the same for commit messages). As I understand, we are not really optimizing anything here. We are refactoring the metric names into constants.

Can you please change the PR description to reflect the same?

@hudeqi hudeqi changed the title MINOR:Optimize the use of metrics in ReplicaManager and remove checks MINOR:Refactor the metric names into constants in ReplicaManager Jun 14, 2023
@hudeqi

hudeqi commented Jun 14, 2023

Copy link
Copy Markdown
Contributor Author

Looks good to me. One minor thing is to update the PR name (since we use the same for commit messages). As I understand, we are not really optimizing anything here. We are refactoring the metric names into constants.

Can you please change the PR description to reflect the same?

ok, I see, updated the pr name. @divijvaidya

@divijvaidya
divijvaidya self-requested a review June 14, 2023 16:02
@divijvaidya

Copy link
Copy Markdown
Member

I triggered the CI tests again since last run wasn't complete. Will merge this in after they are successful.

@hudeqi

hudeqi commented Jun 15, 2023

Copy link
Copy Markdown
Contributor Author

I triggered the CI tests again since last run wasn't complete. Will merge this in after they are successful.

It seems that the failure of CI check has nothing to do with this pr. @divijvaidya

@divijvaidya

Copy link
Copy Markdown
Member

It seems that the failure of CI check has nothing to do with this pr. @divijvaidya

Hmm...not sure about that. There are 99 tests failing and I am not comfortable saying that they are not because of this PR. Let's wait a day or 2 for CI to stabilize, after that we can merge from trunk again and try the CI for this PR.

@hudeqi

hudeqi commented Jun 15, 2023

Copy link
Copy Markdown
Contributor Author

It seems that the failure of CI check has nothing to do with this pr. @divijvaidya

Hmm...not sure about that. There are 99 tests failing and I am not comfortable saying that they are not because of this PR. Let's wait a day or 2 for CI to stabilize, after that we can merge from trunk again and try the CI for this PR.

Is this CI test often unstable? I've hardly ever seen anything that can be completely checked successfully. 😂

@hudeqi
hudeqi force-pushed the optimize_metric_match branch from c0fda76 to 16e7265 Compare June 16, 2023 12:30
@divijvaidya

Copy link
Copy Markdown
Member

Failing tests are unrelated. These are frequently failing tests in trunk.

[Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationBaseTest.testReplicateSourceDefault()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/org.apache.kafka.connect.mirror.integration/MirrorConnectorsIntegrationBaseTest/Build___JDK_17_and_Scala_2_13___testReplicateSourceDefault__/)
[Build / JDK 17 and Scala 2.13 / kafka.api.ConsumerBounceTest.testClose()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/kafka.api/ConsumerBounceTest/Build___JDK_17_and_Scala_2_13___testClose__/)
[Build / JDK 17 and Scala 2.13 / kafka.api.TransactionsTest.testBumpTransactionalEpoch(String).quorum=kraft](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/kafka.api/TransactionsTest/Build___JDK_17_and_Scala_2_13___testBumpTransactionalEpoch_String__quorum_kraft/)
[Build / JDK 17 and Scala 2.13 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testNewAndChangedTopicsInDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/kafka.zk/ZkMigrationIntegrationTest/Build___JDK_17_and_Scala_2_13____1__Type_ZK__Name_testNewAndChangedTopicsInDualWrite__MetadataVersion_3_4_IV0__Security_PLAINTEXT/)
[Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.IdentityReplicationIntegrationTest.testReplicateSourceDefault()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/org.apache.kafka.connect.mirror.integration/IdentityReplicationIntegrationTest/Build___JDK_11_and_Scala_2_13___testReplicateSourceDefault__/)
[Build / JDK 11 and Scala 2.13 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testNewAndChangedTopicsInDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/kafka.zk/ZkMigrationIntegrationTest/Build___JDK_11_and_Scala_2_13____1__Type_ZK__Name_testNewAndChangedTopicsInDualWrite__MetadataVersion_3_4_IV0__Security_PLAINTEXT/)
[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-13705/11/testReport/junit/org.apache.kafka.controller/QuorumControllerTest/Build___JDK_11_and_Scala_2_13___testBalancePartitionLeaders__/)
[Build / JDK 8 and Scala 2.12 / kafka.api.ConsumerBounceTest.testClose()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/kafka.api/ConsumerBounceTest/Build___JDK_8_and_Scala_2_12___testClose__/)

@divijvaidya
divijvaidya merged commit 09e8adb into apache:trunk Jun 17, 2023
@hudeqi
hudeqi deleted the optimize_metric_match branch June 17, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Kafka Broker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants