Skip to content

MINOR: Clean up streams metric sensors#9696

Merged
chia7712 merged 3 commits into
apache:trunkfrom
lct45:sensorCleanup
Dec 9, 2020
Merged

MINOR: Clean up streams metric sensors#9696
chia7712 merged 3 commits into
apache:trunkfrom
lct45:sensorCleanup

Conversation

@lct45

@lct45 lct45 commented Dec 4, 2020

Copy link
Copy Markdown

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

@lct45

lct45 commented Dec 4, 2020

Copy link
Copy Markdown
Author

@cadonna @mjsax for review

@highluck

highluck commented Dec 7, 2020

Copy link
Copy Markdown
Contributor

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

@cadonna

cadonna commented Dec 8, 2020

Copy link
Copy Markdown
Member

@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

chia7712 commented Dec 8, 2020

Copy link
Copy Markdown
Member

Could we give a help method to handle similar code?

@cadonna

cadonna commented Dec 8, 2020

Copy link
Copy Markdown
Member

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

lct45 commented Dec 8, 2020

Copy link
Copy Markdown
Author

Just removed duplicate code @cadonna @chia7712

chia7712
chia7712 approved these changes Dec 8, 2020
View reviewed changes

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

@lct45 Thanks for your patch. LGTM!

cadonna
cadonna reviewed Dec 8, 2020
View reviewed changes

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

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.

Comment thread ...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java Outdated
cadonna
cadonna approved these changes Dec 8, 2020
View reviewed changes

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

LGTM! Thanks!

Just two minor comments

Comment thread ...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java Outdated
Comment thread ...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java Outdated
@highluck

highluck commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

@cadonna
Thank you for explaining

@cadonna

cadonna commented Dec 9, 2020

Copy link
Copy Markdown
Member

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

chia7712 commented Dec 9, 2020

Copy link
Copy Markdown
Member

@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

Development

Successfully merging this pull request may close these issues.

4 participants

@lct45 @highluck @cadonna @chia7712