Skip to content
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

KAFKA-14866:Remove controller module metrics when broker is shutting down #13473

Merged
merged 1 commit into from Jun 6, 2023
Merged

KAFKA-14866:Remove controller module metrics when broker is shutting down #13473

merged 1 commit into from Jun 6, 2023

Conversation

hudeqi
Copy link
Collaborator

@hudeqi hudeqi commented Mar 29, 2023 •
edited

In the current situation, KafkaController started 12 related metrics at startup, but did not remove these metrics when broker shutdown.
I removed these metrics from here when it is shutdown, and added related unit tests to check to prevent forgetting to remove metrics when new metrics are added in the future.
The change has successfully passed the unit test, and when omitting to remove the metric, the unit test fails.

divijvaidya
divijvaidya reviewed Mar 29, 2023
View reviewed changes
Copy link
Contributor

@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.

Thank you for the change.

Please add a unit test here which verifies that the number of calls to register metric and remove metric is same in this class. You can use verify() in Mockito to do that.

core/src/main/scala/kafka/controller/KafkaController.scala Outdated Show resolved Hide resolved
core/src/main/scala/kafka/controller/KafkaController.scala Outdated Show resolved Hide resolved
@hudeqi hudeqi mentioned this pull request Mar 29, 2023
Merged
@hudeqi hudeqi force-pushed the hdq_fix_controller branch 3 times, most recently from 20b43ac to c1d7804 Compare April 1, 2023 15:46
@hudeqi hudeqi requested a review from divijvaidya April 1, 2023 15:49
@hudeqi
Copy link
Collaborator Author

hudeqi commented Apr 3, 2023

Hello, please help to review this PR, thank you! @divijvaidya

divijvaidya
divijvaidya approved these changes Apr 3, 2023
View reviewed changes
Copy link
Contributor

@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.

Added a nit, otherwise code looks good to me. As a general suggestion for this and future PRs, it helps the reviewer if your description has the following sections:

Motivation (explaining why this change is required)
Code change (what actually is being changed in the code)
Testing (how did you validate that the code changes are correct)

This PR is ready for a committer to take a look at it.

core/src/test/scala/unit/kafka/controller/KafkaControllerTest.scala Outdated Show resolved Hide resolved
@hudeqi
Copy link
Collaborator Author

hudeqi commented Apr 3, 2023

Added a nit, otherwise code looks good to me. As a general suggestion for this and future PRs, it helps the reviewer if your description has the following sections:

Motivation (explaining why this change is required)
Code change (what actually is being changed in the code)
Testing (how did you validate that the code changes are correct)

This PR is ready for a committer to take a look at it.

I see, thanks a lot!

@hudeqi
Copy link
Collaborator Author

hudeqi commented Apr 4, 2023

And this pr. thx! @showuon

@hudeqi
Copy link
Collaborator Author

hudeqi commented Apr 6, 2023

Do other committers have time to review this PR?

@hudeqi
Copy link
Collaborator Author

hudeqi commented Apr 10, 2023

Do other committers have time to review this PR?

@guozhangwang Hello, can you help to review these two PRs? this and #13471

@hudeqi
Copy link
Collaborator Author

hudeqi commented Apr 19, 2023

Hello, can you help to review this PR? @mimaison

@hudeqi
Copy link
Collaborator Author

hudeqi commented Apr 24, 2023

Hello, can you help to review this PR? @mimaison

and pin here @mimaison

@hudeqi
Copy link
Collaborator Author

hudeqi commented May 9, 2023

bump this pr, Is there any other commiters to help review and merge this pr?

@hudeqi
Copy link
Collaborator Author

hudeqi commented May 9, 2023

bump this pr, Is there any other commiters to help review and merge this pr?

I'm sorry I didn't notice the comment ("We're past code freeze for 3.5 so moving this to the next release") in the corresponding jira

@divijvaidya
Copy link
Contributor

Hey @hudeqi
I had a similar PR merge recently (#13623) and the comments I received in that PR could be applied to this one also.
Specifically,

  1. the metric names could be verified individually in the test.
  2. the metric names could be moved in a const and we can use companion object to define those.

Perhaps, you are make those changes in this PR as request the same committer to review this PR as well?

@hudeqi
Copy link
Collaborator Author

hudeqi commented May 11, 2023

Hey @hudeqi I had a similar PR merge recently (#13623) and the comments I received in that PR could be applied to this one also. Specifically,

  1. the metric names could be verified individually in the test.
  2. the metric names could be moved in a const and we can use companion object to define those.

Perhaps, you are make those changes in this PR as request the same committer to review this PR as well?

thanks! divijvaidya!

@hudeqi
Copy link
Collaborator Author

hudeqi commented May 11, 2023

@dajac When you get a chance, please take a look. Thanks!

@hudeqi
Copy link
Collaborator Author

hudeqi commented May 31, 2023

@dajac When you get a chance, please take a look. Thanks!

Hello, pin again. @dajac

mimaison
mimaison reviewed Jun 5, 2023
View reviewed changes
Copy link
Member

@mimaison mimaison 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 PR! I left a few comments

core/src/test/scala/unit/kafka/controller/KafkaControllerTest.scala Outdated Show resolved Hide resolved
core/src/test/scala/unit/kafka/controller/KafkaControllerTest.scala Outdated Show resolved Hide resolved
core/src/test/scala/unit/kafka/controller/KafkaControllerTest.scala Outdated Show resolved Hide resolved
@hudeqi
Copy link
Collaborator Author

hudeqi commented Jun 6, 2023

Thanks for the PR! I left a few comments

Thanks for your review, I have updated it based on your comments. @mimaison

@hudeqi hudeqi requested a review from mimaison June 6, 2023 02:14
mimaison
mimaison approved these changes Jun 6, 2023
View reviewed changes
Copy link
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR

@mimaison mimaison merged commit 9ebe395 into apache:trunk Jun 6, 2023
1 check passed
@hudeqi hudeqi deleted the hdq_fix_controller branch June 6, 2023 09:15
@hudeqi hudeqi mentioned this pull request Sep 20, 2023
Closed
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

@mimaison mimaison mimaison approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@hudeqi @divijvaidya @mimaison