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

MINOR: Clean up streams metric sensors #9696

Merged
merged 3 commits into from
Dec 9, 2020
Merged

MINOR: Clean up streams metric sensors #9696

merged 3 commits into from
Dec 9, 2020

Conversation

lct45
Copy link
Contributor

@lct45 lct45 commented Dec 4, 2020

Follow-up from #9614, updates streams metrics sensor logic

@lct45
Copy link
Contributor Author

lct45 commented Dec 4, 2020

@cadonna @mjsax for review

@highluck
Copy link
Contributor

highluck commented Dec 7, 2020

Is there any reason to change it to the same logic?

@cadonna
Copy link
Contributor

cadonna commented Dec 8, 2020

@highluck I am sorry that we change the code you authored, but over time during multiple reviews of this class, we realized that checking the sensor for null is easier readable and thought it would be good to consistently change all sensor check.

@chia7712
Copy link
Contributor

chia7712 commented Dec 8, 2020

Could we give a help method to handle similar code?

@cadonna
Copy link
Contributor

cadonna commented Dec 8, 2020

Could we give a help method to handle similar code?

That might be possible for all but client-level sensors. I think that is a good idea. @lct45 could you try to extract a method for all but client-level sensors?

@lct45
Copy link
Contributor Author

lct45 commented Dec 8, 2020

Just removed duplicate code @cadonna @chia7712

chia7712
chia7712 approved these changes Dec 8, 2020
View reviewed changes
Copy link
Contributor

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

@lct45 Thanks for your patch. LGTM!

cadonna
cadonna reviewed Dec 8, 2020
View reviewed changes
Copy link
Contributor

@cadonna cadonna left a comment

Choose a reason for hiding this comment

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

Thank @lct45 for the update!

I left one comment.

Could you rebase this PR to trunk? All tests failed for an issue that was recently resolved on trunk.

...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java Outdated Show resolved Hide resolved
cadonna
cadonna approved these changes Dec 8, 2020
View reviewed changes
Copy link
Contributor

@cadonna cadonna 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!

Just two minor comments

...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java Outdated Show resolved Hide resolved
...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java Outdated Show resolved Hide resolved
@highluck
Copy link
Contributor

highluck commented Dec 9, 2020

@cadonna
Thank you for explaining

@cadonna
Copy link
Contributor

cadonna commented Dec 9, 2020

Unrelated test failure of a known flaky test kafka.api.ConsumerBounceTest.testClose in JDK 8.

@chia7712 Could you merge this PR?

@chia7712 chia7712 merged commit 78a986b into apache:trunk Dec 9, 2020
@chia7712
Copy link
Contributor

chia7712 commented Dec 9, 2020

@lct45 Thanks for your patch. Merge it to trunk!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cadonna cadonna cadonna approved these changes

@chia7712 chia7712 chia7712 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@lct45 @highluck @cadonna @chia7712