[GitHub] [helix] jiajunwang commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
jiajunwang commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable. URL: https://github.com/apache/helix/pull/378#issuecomment-522772493 Please try to run "mvn test". 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] jiajunwang commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
jiajunwang commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable. URL: https://github.com/apache/helix/pull/378#issuecomment-521818405 > @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. 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] jiajunwang commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
jiajunwang commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable. URL: https://github.com/apache/helix/pull/378#issuecomment-521530508 @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 : ( 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] jiajunwang commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
jiajunwang commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable. URL: https://github.com/apache/helix/pull/378#issuecomment-520933866 One more thing, please also revisit the previous comments and resolve them if the problem has been fixed or no longer valid. 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] jiajunwang commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
jiajunwang commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable. URL: https://github.com/apache/helix/pull/378#issuecomment-519638967 @chenboat Please refer to HelixManagerProperties. We should be able to do something similar for the monitors. Some background here. We used to use System Properties for these configurations. You can still find a lot of these usages in our code. But we are trying to migrate to a more flexible way. So I think HelixManagerProperties would be a good example. Please let us know what's your preference. 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] jiajunwang commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
jiajunwang commented on issue #378: Make the reservoir used in
ZkClientPathMonitor metric configurable.
URL: https://github.com/apache/helix/pull/378#issuecomment-519387017
Thanks for the PR. I have several comments.
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?
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.
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
