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.

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

MINOR: Refactor process rate and latency metrics on thread-level #8172

Merged
merged 2 commits into from
Mar 2, 2020

Conversation

cadonna
Copy link
Contributor

@cadonna cadonna commented Feb 26, 2020

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@cadonna
Copy link
Contributor Author

cadonna commented Feb 26, 2020

Call for review: @vvcephei

@vvcephei
Copy link
Contributor

Thanks @cadonna , for the cleanup.

I initially wrote the tests that way before I knew of the existence of the addRateOfSumAndSumMetricsToSensor utility. Now that we're using it, we can go back to ensuring that the ThreadMetrics just invokes the expected StreamsMetricsUtil methods, and not directly verify that the right metrics really got added.

Can you comment on why we should rename the sensor? I thought it might be a good idea to clearly indicate which sensor is for measuring latency versus counting operations, since there's been massive confusion about this in the past.

Copy link
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

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

Thanks!

@vvcephei
Copy link
Contributor

test this please

Copy link
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

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

Just curious what this PR trying to refactor? the non-testing part seems not making any logical changes.

@vvcephei
Copy link
Contributor

Retest this, please.

@vvcephei
Copy link
Contributor

Looks like the tests never ran the first time. I can't speak to @cadonna's motivation, @guozhangwang . I assume he just felt that my testing approach should have been cleaner, but didn't want to block the original PR.

@cadonna
Copy link
Contributor Author

cadonna commented Feb 28, 2020

@guozhangwang My motivation was to clean-up the tests. I see that the title is a bit misleading. It was mainly a test refactoring.

@guozhangwang
Copy link
Contributor

@guozhangwang My motivation was to clean-up the tests. I see that the title is a bit misleading. It was mainly a test refactoring.

Got it, thanks!

@vvcephei
Copy link
Contributor

vvcephei commented Mar 2, 2020

The Java 11 tests had just hung, but I really don't think it could have been due to this change.

@vvcephei vvcephei merged commit ea0c027 into apache:trunk Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants