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

Upgrade metrics to 4.0.7, add CachedHistogram #1319

Merged
merged 7 commits into from
Dec 5, 2019
Merged

Upgrade metrics to 4.0.7, add CachedHistogram #1319

merged 7 commits into from
Dec 5, 2019

Conversation

cgtz
Copy link
Contributor

@cgtz cgtz commented Nov 21, 2019

  • Upgrade codahale 3.x to dropwizard 4.0.7, which includes fixes for
    histogram accuracy.
  • Fix high contention getSnapshot code by introducing caching for
    histogram values used by AdaptiveOperationTracker. Calling
    Histogram:getSnapshot is more expensive in recent versions of the
    metrics library because of a lock taken for reservoir rescaling.

@cgtz cgtz force-pushed the upgrade-codahale branch from f93708d to 2c47879 Compare December 2, 2019 19:14
@cgtz cgtz requested review from jsjtzyy and ankagrawal December 2, 2019 21:55
@codecov-io
Copy link

codecov-io commented Dec 2, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@75d2971). Click here to learn what that means.
The diff coverage is 87.03%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1319   +/-   ##
=========================================
  Coverage          ?   72.15%           
  Complexity        ?     6487           
=========================================
  Files             ?      470           
  Lines             ?    37174           
  Branches          ?     4684           
=========================================
  Hits              ?    26824           
  Misses            ?     9102           
  Partials          ?     1248
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/com.github.ambry.cloud/VcrServer.java 82.35% <ø> (ø) 8 <0> (?)
.../main/java/com.github.ambry/store/DumpLogTool.java 0% <ø> (ø) 0 <0> (?)
...rc/main/java/com.github.ambry.rest/RestServer.java 91.87% <ø> (ø) 13 <0> (?)
...main/java/com.github.ambry/store/DumpDataTool.java 0% <ø> (ø) 0 <0> (?)
...main/java/com.github.ambry.server/AmbryServer.java 79.02% <ø> (ø) 10 <0> (?)
....github.ambry/tools/perf/rest/NettyPerfClient.java 0% <ø> (ø) 0 <0> (?)
....github.ambry.router/AdaptiveOperationTracker.java 91.91% <100%> (ø) 29 <0> (?)
...n/java/com.github.ambry.utils/CachedHistogram.java 100% <100%> (ø) 3 <3> (?)
...ain/java/com.github.ambry/config/RouterConfig.java 97.8% <100%> (ø) 1 <0> (?)
....github.ambry.router/NonBlockingRouterMetrics.java 87.14% <72%> (ø) 62 <13> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75d2971...098ab2a. Read the comment docs.

@cgtz cgtz requested review from dharju and removed request for ankagrawal December 2, 2019 21:55
jsjtzyy
jsjtzyy reviewed Dec 3, 2019
View reviewed changes
Copy link
Contributor

@jsjtzyy jsjtzyy left a comment

Choose a reason for hiding this comment

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

Left a few comments.
Changes look good and I will approve this PR after comments are addressed.

ambry-router/src/main/java/com.github.ambry.router/AdaptiveOperationTracker.java Outdated Show resolved Hide resolved
ambry-api/src/main/java/com.github.ambry/config/RouterConfig.java Outdated Show resolved Hide resolved
ambry-router/src/main/java/com.github.ambry.router/NonBlockingRouterMetrics.java Show resolved Hide resolved
ambry-router/src/main/java/com.github.ambry.router/NonBlockingRouterMetrics.java Outdated Show resolved Hide resolved
ambry-router/src/main/java/com.github.ambry.router/NonBlockingRouterMetrics.java Outdated Show resolved Hide resolved
cgtz added 7 commits December 3, 2019 16:21
@cgtz
Upgrade metrics to 4.0.7, add CachedHistogram
5f96b9b 
- Upgrade codahale 3.x to dropwizard 4.0.7, which includes fixes for
  histogram accuracy.
- Fix high contention getSnapshot code by introducing caching for
  histogram values used by AdaptiveOperationTracker. Calling
  Histogram:getSnapshot is more expensive in recent versions of the
  metrics library because of a lock taken for reservoir rescaling.
@cgtz
Fix NPE
cc11ebe
@cgtz
Add copyright
7f5f2a2
@cgtz
More licenses
2a4c8c8
@cgtz
sadness
6cc9c8c
@cgtz
address comments
2c58bc5
@cgtz
address comment
098ab2a
@cgtz cgtz force-pushed the upgrade-codahale branch from 3c947c3 to 098ab2a Compare December 4, 2019 00:21
jsjtzyy
jsjtzyy approved these changes Dec 4, 2019
View reviewed changes
Copy link
Contributor

@jsjtzyy jsjtzyy left a comment

Choose a reason for hiding this comment

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

LGTM

dharju
dharju approved these changes Dec 5, 2019
View reviewed changes
Copy link
Contributor

@dharju dharju left a comment

Choose a reason for hiding this comment

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

lgtm too

@dharju dharju merged commit 92f3d17 into linkedin:master Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dharju dharju dharju approved these changes

@jsjtzyy jsjtzyy jsjtzyy 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
@cgtz @codecov-io @jsjtzyy @dharju