[GitHub] [helix] chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable. URL: https://github.com/apache/helix/pull/378#issuecomment-523745879 This PR is ready to be merged, approved by [@jiajunwang]. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable. URL: https://github.com/apache/helix/pull/378#issuecomment-523745090 > @chenboat If you want this code merged, please review https://github.com/apache/helix/wiki/Pull-Request-Merge-Steps Done. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable. URL: https://github.com/apache/helix/pull/378#issuecomment-522853075 This PR is ready to be merged, approved by [@jiajunwang]. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable. URL: https://github.com/apache/helix/pull/378#issuecomment-522852156 > Please try to run "mvn test". I ran the mvn for helix-core. There are transient failures which can pass after re-run. The mbean tests are all good. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable. URL: https://github.com/apache/helix/pull/378#issuecomment-522284425 > > @chenboat @narendly I'm thinking about if there is a better way to test. > > My major concern is that we make the protected method public just for testing. > > Moreover, it seems we can only verify if the system property is read correctly. We cannot verify if it has been applied to the Histogram as expected. The window field is private in the metrics lib. I haven't got a good idea now : ( > > I got something. It could be a bad idea, but better than nothing. > Can we add a test case that: > > 1. set the time window to 2 seconds. > 2. create a monitor and update a histogram metric data once. Verify the expected data presents in the histogram metric. > 3. Wait for 3 seconds. Check the same histogram metric data, the data should be have been reset. Thanks for the suggestion. It makes sense. I added a test case using the same idea. Please check out my latest commit. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable. URL: https://github.com/apache/helix/pull/378#issuecomment-521888486 > @chenboat @narendly I'm thinking about if there is a better way to test. > > My major concern is that we make the protected method public just for testing. > Moreover, it seems we can only verify if the system property is read correctly. We cannot verify if it has been applied to the Histogram as expected. The window field is private in the metrics lib. I haven't got a good idea now : ( While most of the logic about my diff is about loading system property to init the SlidingWindow, I do understand the need to check the histogram value. Most of the current Unit tests are not aiming for testing histogram values -- they passed even I change the window length -- my understanding is that this metrics is buried pretty deep in the code. Let me take a look at some tests which could verify the histograms. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
chenboat commented on issue #378: Make the reservoir used in
ZkClientPathMonitor metric configurable.
URL: https://github.com/apache/helix/pull/378#issuecomment-519586561
Thanks for all the feedbacks. Comments inline below.
> 1. I agree to make the window configurable. It makes sense.
> 2. The current implementation passes the configure item all the way from
Accessor to the monitor. This is not necessary, in my opinion. Moreover, it
would be extremely difficult to apply this solution to all of our monitors.
Have you tried to add a global configuration and apply it to all the monitors?
Good point. I mainly look at the fix for the accessor we used (i.e., in
Pinot). I can work on the global config change. Do you know if there is any
global configuration class already in Helix? If not, I can add a new one.
> 3. FYI, we also did some investigation before. What we found is these code
in
https://github.com/dropwizard/metrics/blob/3748f09b249f47a24ef868595fed4556ec5e92b1/metrics-core/src/main/java/com/codahale/metrics/SlidingTimeWindowReservoir.java
>@override
>public Snapshot getSnapshot() {
>trim();
>return new UniformSnapshot(measurements.values());
>}
>This basically means on every get, the metrics lib will clone all the
data. And we believe this causes frequent GC and high memory usage.
From our Heapdump analysis (screenshot in the original issue #382), we found
the main contributors to the memory are the time bounded linked list of
SlidingTimeWindowReservoir objects in a prod server. That is the reason we want
to tune the window length.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
