You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@helix.apache.org by Zhen Zhang <ne...@gmail.com> on 2014/03/14 23:17:09 UTC
Review Request 19240: [HELIX-319] refactor MonitoringClient to accommodate
distributed monitoring server
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19240/
-----------------------------------------------------------
Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
Repository: helix-git
Description
-------
another refactoring based on last iteration:
1) remove spectating on monitoring cluster, use a simple round-robin approach to find the first available monitoring server
2) add a sharding key in MonitoringEvent
3) refactor AlertProxy, using direct java function call from clj instead of jetty/http
Diffs
-----
helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java 743f8b4
helix-core/src/main/java/org/apache/helix/monitoring/MonitoringEvent.java 80006fb
helix-monitor-client/src/main/java/org/apache/helix/monitoring/ClientUtil.java e69de29
helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java e69de29
helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java e69de29
helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAgent.java 61e3d6c
helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAlertProxy.java 5fe8ebe
helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannConfigs.java 193b763
helix-monitor-server/src/test/java/org/apache/helix/monitoring/IntegrationTest.java 9d28dc3
helix-monitor-server/src/test/java/org/apache/helix/monitoring/MonitoringTestHelper.java e69de29
helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestClientServerMonitoring.java 588bc35
helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannAgent.java 39ece64
helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannAlertProxy.java 700682a
helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannClientWrapper.java e69de29
helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannMonitoringServer.java f7d0585
Diff: https://reviews.apache.org/r/19240/diff/
Testing
-------
Thanks,
Zhen Zhang
Re: Review Request 19240: [HELIX-319] refactor MonitoringClient to
accommodate distributed monitoring server
Posted by Kanak Biscuitwala <ka...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19240/#review37674
-----------------------------------------------------------
Ship it!
helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/HelixAlertMessenger.java
<https://reviews.apache.org/r/19240/#comment69303>
Consider making this async
- Kanak Biscuitwala
On March 18, 2014, 4:39 p.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19240/
> -----------------------------------------------------------
>
> (Updated March 18, 2014, 4:39 p.m.)
>
>
> Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> another refactoring based on last iteration:
> 1) remove spectating on monitoring cluster, use a simple round-robin approach to find the first available monitoring server
> 2) add a sharding key in MonitoringEvent
> 3) refactor AlertProxy, using direct java function call from clj instead of jetty/http
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java 743f8b4
> helix-core/src/main/java/org/apache/helix/monitoring/MonitoringEvent.java 80006fb
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/riemann/ClientUtil.java e69de29
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/riemann/RawRiemannClient.java e69de29
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/riemann/RiemannClientWrapper.java e69de29
> helix-monitor-server/pom.xml 2cbddfd
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/HelixAlertMessenger.java 5fe8ebe
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/RiemannAgent.java 61e3d6c
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/RiemannConfigs.java 193b763
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/RiemannMonitoringServer.java d4f11a5
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/MonitoringTestHelper.java e69de29
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/IntegrationTest.java 9d28dc3
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestClientServerMonitoring.java 588bc35
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestHelixAlertMessenger.java 700682a
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestRiemannAgent.java 39ece64
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestRiemannClientWrapper.java e69de29
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestRiemannMonitoringServer.java f7d0585
>
> Diff: https://reviews.apache.org/r/19240/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 19240: [HELIX-319] refactor MonitoringClient to
accommodate distributed monitoring server
Posted by Zhen Zhang <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19240/
-----------------------------------------------------------
(Updated March 18, 2014, 11:39 p.m.)
Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
Repository: helix-git
Description
-------
another refactoring based on last iteration:
1) remove spectating on monitoring cluster, use a simple round-robin approach to find the first available monitoring server
2) add a sharding key in MonitoringEvent
3) refactor AlertProxy, using direct java function call from clj instead of jetty/http
Diffs (updated)
-----
helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java 743f8b4
helix-core/src/main/java/org/apache/helix/monitoring/MonitoringEvent.java 80006fb
helix-monitor-client/src/main/java/org/apache/helix/monitoring/riemann/ClientUtil.java e69de29
helix-monitor-client/src/main/java/org/apache/helix/monitoring/riemann/RawRiemannClient.java e69de29
helix-monitor-client/src/main/java/org/apache/helix/monitoring/riemann/RiemannClientWrapper.java e69de29
helix-monitor-server/pom.xml 2cbddfd
helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/HelixAlertMessenger.java 5fe8ebe
helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/RiemannAgent.java 61e3d6c
helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/RiemannConfigs.java 193b763
helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/RiemannMonitoringServer.java d4f11a5
helix-monitor-server/src/test/java/org/apache/helix/monitoring/MonitoringTestHelper.java e69de29
helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/IntegrationTest.java 9d28dc3
helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestClientServerMonitoring.java 588bc35
helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestHelixAlertMessenger.java 700682a
helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestRiemannAgent.java 39ece64
helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestRiemannClientWrapper.java e69de29
helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestRiemannMonitoringServer.java f7d0585
Diff: https://reviews.apache.org/r/19240/diff/
Testing
-------
Thanks,
Zhen Zhang
Re: Review Request 19240: [HELIX-319] refactor MonitoringClient to
accommodate distributed monitoring server
Posted by Zhen Zhang <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19240/
-----------------------------------------------------------
(Updated March 18, 2014, 11:31 p.m.)
Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
Repository: helix-git
Description
-------
another refactoring based on last iteration:
1) remove spectating on monitoring cluster, use a simple round-robin approach to find the first available monitoring server
2) add a sharding key in MonitoringEvent
3) refactor AlertProxy, using direct java function call from clj instead of jetty/http
Diffs (updated)
-----
helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java 743f8b4
helix-core/src/main/java/org/apache/helix/monitoring/MonitoringEvent.java 80006fb
helix-monitor-server/pom.xml 2cbddfd
helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/HelixAlertMessenger.java 5fe8ebe
helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/RiemannAgent.java 61e3d6c
helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/RiemannConfigs.java 193b763
helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/RiemannMonitoringServer.java d4f11a5
helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/IntegrationTest.java 9d28dc3
helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestClientServerMonitoring.java 588bc35
helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestHelixAlertMessenger.java 700682a
helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestRiemannAgent.java 39ece64
helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestRiemannMonitoringServer.java f7d0585
Diff: https://reviews.apache.org/r/19240/diff/
Testing
-------
Thanks,
Zhen Zhang
Re: Review Request 19240: [HELIX-319] refactor MonitoringClient to
accommodate distributed monitoring server
Posted by Zhen Zhang <ne...@gmail.com>.
> On March 14, 2014, 11:58 p.m., Kishore Gopalakrishna wrote:
> > shud we have a separate package under monitoring for Riemann. Are we emitting any metrics.
>
> Zhen Zhang wrote:
> yes. we can have a separate package for Riemann. we are not emitting metrics
fixed
> On March 14, 2014, 11:58 p.m., Kishore Gopalakrishna wrote:
> > helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java, line 48
> > <https://reviews.apache.org/r/19240/diff/1/?file=520033#file520033line48>
> >
> > why do we have batch boolean in the method. Should we not have the interval in the constructor ?
>
> Kanak Biscuitwala wrote:
> Since we have sendAndFlush, the batch argument is probably unnecessary.
>
> Zhen Zhang wrote:
> agree. the batch argument can be removed. if batchSize>1, use batchClient, otherwise use normal client
fixed
> On March 14, 2014, 11:58 p.m., Kishore Gopalakrishna wrote:
> > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java, line 31
> > <https://reviews.apache.org/r/19240/diff/1/?file=520037#file520037line31>
> >
> > this State will confuse with Helix State, can we make this private if its not used outside of this class or rename the enum
>
> Zhen Zhang wrote:
> will change to private
fixed
> On March 14, 2014, 11:58 p.m., Kishore Gopalakrishna wrote:
> > helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAgent.java, line 73
> > <https://reviews.apache.org/r/19240/diff/1/?file=520038#file520038line73>
> >
> > can we create constants for heartbeat, running ttl etc
>
> Zhen Zhang wrote:
> will do
fixed
- Zhen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19240/#review37279
-----------------------------------------------------------
On March 18, 2014, 11:39 p.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19240/
> -----------------------------------------------------------
>
> (Updated March 18, 2014, 11:39 p.m.)
>
>
> Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> another refactoring based on last iteration:
> 1) remove spectating on monitoring cluster, use a simple round-robin approach to find the first available monitoring server
> 2) add a sharding key in MonitoringEvent
> 3) refactor AlertProxy, using direct java function call from clj instead of jetty/http
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java 743f8b4
> helix-core/src/main/java/org/apache/helix/monitoring/MonitoringEvent.java 80006fb
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/riemann/ClientUtil.java e69de29
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/riemann/RawRiemannClient.java e69de29
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/riemann/RiemannClientWrapper.java e69de29
> helix-monitor-server/pom.xml 2cbddfd
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/HelixAlertMessenger.java 5fe8ebe
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/RiemannAgent.java 61e3d6c
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/RiemannConfigs.java 193b763
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/riemann/RiemannMonitoringServer.java d4f11a5
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/MonitoringTestHelper.java e69de29
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/IntegrationTest.java 9d28dc3
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestClientServerMonitoring.java 588bc35
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestHelixAlertMessenger.java 700682a
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestRiemannAgent.java 39ece64
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestRiemannClientWrapper.java e69de29
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/riemann/TestRiemannMonitoringServer.java f7d0585
>
> Diff: https://reviews.apache.org/r/19240/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 19240: [HELIX-319] refactor MonitoringClient to
accommodate distributed monitoring server
Posted by Zhen Zhang <ne...@gmail.com>.
> On March 14, 2014, 11:58 p.m., Kishore Gopalakrishna wrote:
> > shud we have a separate package under monitoring for Riemann. Are we emitting any metrics.
yes. we can have a separate package for Riemann. we are not emitting metrics
> On March 14, 2014, 11:58 p.m., Kishore Gopalakrishna wrote:
> > helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java, line 48
> > <https://reviews.apache.org/r/19240/diff/1/?file=520033#file520033line48>
> >
> > why do we have batch boolean in the method. Should we not have the interval in the constructor ?
>
> Kanak Biscuitwala wrote:
> Since we have sendAndFlush, the batch argument is probably unnecessary.
agree. the batch argument can be removed. if batchSize>1, use batchClient, otherwise use normal client
> On March 14, 2014, 11:58 p.m., Kishore Gopalakrishna wrote:
> > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java, line 31
> > <https://reviews.apache.org/r/19240/diff/1/?file=520037#file520037line31>
> >
> > this State will confuse with Helix State, can we make this private if its not used outside of this class or rename the enum
will change to private
> On March 14, 2014, 11:58 p.m., Kishore Gopalakrishna wrote:
> > helix-core/src/main/java/org/apache/helix/monitoring/MonitoringEvent.java, line 258
> > <https://reviews.apache.org/r/19240/diff/1/?file=520034#file520034line258>
> >
> > can we have only one shardingKey. Does it make sense to call this shardingKey. Instead user should probably think about the scopes (cluster, resource, partition, participant) ? ShardingKey is probably for advanced users. what do you think?
we can have a sharding strategy, user can specify a scope or a string. we can provide "cluster + resource" as the default sharding strategy.
> On March 14, 2014, 11:58 p.m., Kishore Gopalakrishna wrote:
> > helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAgent.java, line 73
> > <https://reviews.apache.org/r/19240/diff/1/?file=520038#file520038line73>
> >
> > can we create constants for heartbeat, running ttl etc
will do
> On March 14, 2014, 11:58 p.m., Kishore Gopalakrishna wrote:
> > helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAlertProxy.java, line 59
> > <https://reviews.apache.org/r/19240/diff/1/?file=520039#file520039line59>
> >
> > dont we have to call zkClient.waitUntilconnected or something ?
it's fine. zkclient operations are all wrapped with retryUntilConnected(...)
> On March 14, 2014, 11:58 p.m., Kishore Gopalakrishna wrote:
> > helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAlertProxy.java, line 72
> > <https://reviews.apache.org/r/19240/diff/1/?file=520039#file520039line72>
> >
> > what happens if zkClient is disconnected and how are the error conditions handled. Can we have some protection here so that it does not bombard ZK more than X writes per second?
we should add throttling here. if zkconnection is down, we can't send any alerts to Helix controller.
- Zhen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19240/#review37279
-----------------------------------------------------------
On March 14, 2014, 10:17 p.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19240/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 10:17 p.m.)
>
>
> Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> another refactoring based on last iteration:
> 1) remove spectating on monitoring cluster, use a simple round-robin approach to find the first available monitoring server
> 2) add a sharding key in MonitoringEvent
> 3) refactor AlertProxy, using direct java function call from clj instead of jetty/http
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java 743f8b4
> helix-core/src/main/java/org/apache/helix/monitoring/MonitoringEvent.java 80006fb
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/ClientUtil.java e69de29
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java e69de29
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java e69de29
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAgent.java 61e3d6c
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAlertProxy.java 5fe8ebe
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannConfigs.java 193b763
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/IntegrationTest.java 9d28dc3
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/MonitoringTestHelper.java e69de29
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestClientServerMonitoring.java 588bc35
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannAgent.java 39ece64
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannAlertProxy.java 700682a
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannClientWrapper.java e69de29
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannMonitoringServer.java f7d0585
>
> Diff: https://reviews.apache.org/r/19240/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 19240: [HELIX-319] refactor MonitoringClient to
accommodate distributed monitoring server
Posted by Kanak Biscuitwala <ka...@apache.org>.
> On March 14, 2014, 4:58 p.m., Kishore Gopalakrishna wrote:
> > helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java, line 48
> > <https://reviews.apache.org/r/19240/diff/1/?file=520033#file520033line48>
> >
> > why do we have batch boolean in the method. Should we not have the interval in the constructor ?
Since we have sendAndFlush, the batch argument is probably unnecessary.
> On March 14, 2014, 4:58 p.m., Kishore Gopalakrishna wrote:
> > helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java, line 66
> > <https://reviews.apache.org/r/19240/diff/1/?file=520033#file520033line66>
> >
> > what is the usage of this method
It runs the runnable [r] every [interval] [unit]s with initial delay of [delay] [unit]s. Riemann client provides a method with a similar signature, and this is just a wrapper on that.
- Kanak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19240/#review37279
-----------------------------------------------------------
On March 14, 2014, 3:17 p.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19240/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 3:17 p.m.)
>
>
> Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> another refactoring based on last iteration:
> 1) remove spectating on monitoring cluster, use a simple round-robin approach to find the first available monitoring server
> 2) add a sharding key in MonitoringEvent
> 3) refactor AlertProxy, using direct java function call from clj instead of jetty/http
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java 743f8b4
> helix-core/src/main/java/org/apache/helix/monitoring/MonitoringEvent.java 80006fb
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/ClientUtil.java e69de29
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java e69de29
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java e69de29
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAgent.java 61e3d6c
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAlertProxy.java 5fe8ebe
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannConfigs.java 193b763
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/IntegrationTest.java 9d28dc3
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/MonitoringTestHelper.java e69de29
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestClientServerMonitoring.java 588bc35
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannAgent.java 39ece64
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannAlertProxy.java 700682a
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannClientWrapper.java e69de29
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannMonitoringServer.java f7d0585
>
> Diff: https://reviews.apache.org/r/19240/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 19240: [HELIX-319] refactor MonitoringClient to
accommodate distributed monitoring server
Posted by Kishore Gopalakrishna <ki...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19240/#review37279
-----------------------------------------------------------
shud we have a separate package under monitoring for Riemann. Are we emitting any metrics.
helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java
<https://reviews.apache.org/r/19240/#comment68696>
why do we have batch boolean in the method. Should we not have the interval in the constructor ?
helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java
<https://reviews.apache.org/r/19240/#comment68716>
what is the usage of this method
helix-core/src/main/java/org/apache/helix/monitoring/MonitoringEvent.java
<https://reviews.apache.org/r/19240/#comment68717>
can we have only one shardingKey. Does it make sense to call this shardingKey. Instead user should probably think about the scopes (cluster, resource, partition, participant) ? ShardingKey is probably for advanced users. what do you think?
helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java
<https://reviews.apache.org/r/19240/#comment68718>
this State will confuse with Helix State, can we make this private if its not used outside of this class or rename the enum
helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAgent.java
<https://reviews.apache.org/r/19240/#comment68719>
can we create constants for heartbeat, running ttl etc
helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAlertProxy.java
<https://reviews.apache.org/r/19240/#comment68723>
dont we have to call zkClient.waitUntilconnected or something ?
helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAlertProxy.java
<https://reviews.apache.org/r/19240/#comment68724>
what happens if zkClient is disconnected and how are the error conditions handled. Can we have some protection here so that it does not bombard ZK more than X writes per second?
- Kishore Gopalakrishna
On March 14, 2014, 10:17 p.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19240/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 10:17 p.m.)
>
>
> Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> another refactoring based on last iteration:
> 1) remove spectating on monitoring cluster, use a simple round-robin approach to find the first available monitoring server
> 2) add a sharding key in MonitoringEvent
> 3) refactor AlertProxy, using direct java function call from clj instead of jetty/http
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java 743f8b4
> helix-core/src/main/java/org/apache/helix/monitoring/MonitoringEvent.java 80006fb
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/ClientUtil.java e69de29
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java e69de29
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java e69de29
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAgent.java 61e3d6c
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAlertProxy.java 5fe8ebe
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannConfigs.java 193b763
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/IntegrationTest.java 9d28dc3
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/MonitoringTestHelper.java e69de29
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestClientServerMonitoring.java 588bc35
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannAgent.java 39ece64
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannAlertProxy.java 700682a
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannClientWrapper.java e69de29
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannMonitoringServer.java f7d0585
>
> Diff: https://reviews.apache.org/r/19240/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 19240: [HELIX-319] refactor MonitoringClient to
accommodate distributed monitoring server
Posted by Zhen Zhang <ne...@gmail.com>.
> On March 15, 2014, 12:31 a.m., Kanak Biscuitwala wrote:
> > helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAgent.java, line 73
> > <https://reviews.apache.org/r/19240/diff/1/?file=520038#file520038line73>
> >
> > This should include some argument for host
host info is created automatically in EventDSL
> On March 15, 2014, 12:31 a.m., Kanak Biscuitwala wrote:
> > helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannClientWrapper.java, line 84
> > <https://reviews.apache.org/r/19240/diff/1/?file=520046#file520046line84>
> >
> > Remove these TODOs
fixed
> On March 15, 2014, 12:31 a.m., Kanak Biscuitwala wrote:
> > helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAgent.java, line 40
> > <https://reviews.apache.org/r/19240/diff/1/?file=520038#file520038line40>
> >
> > make these private
fixed
> On March 15, 2014, 12:31 a.m., Kanak Biscuitwala wrote:
> > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java, line 87
> > <https://reviews.apache.org/r/19240/diff/1/?file=520037#file520037line87>
> >
> > If all raw clients fail to connect immediately, this should be in a connecting state, right?
replace state with isConnected, which means if connect() has been called
> On March 15, 2014, 12:31 a.m., Kanak Biscuitwala wrote:
> > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java, line 57
> > <https://reviews.apache.org/r/19240/diff/1/?file=520037#file520037line57>
> >
> > private
fixed
> On March 15, 2014, 12:31 a.m., Kanak Biscuitwala wrote:
> > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java, line 38
> > <https://reviews.apache.org/r/19240/diff/1/?file=520037#file520037line38>
> >
> > Same comment. Make these private if possible.
fixed
> On March 15, 2014, 12:31 a.m., Kanak Biscuitwala wrote:
> > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java, line 30
> > <https://reviews.apache.org/r/19240/diff/1/?file=520037#file520037line30>
> >
> > Add some high-level javadocs
fixed
> On March 15, 2014, 12:31 a.m., Kanak Biscuitwala wrote:
> > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java, line 155
> > <https://reviews.apache.org/r/19240/diff/1/?file=520036#file520036line155>
> >
> > This should be something other than public
made it private
> On March 15, 2014, 12:31 a.m., Kanak Biscuitwala wrote:
> > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java, line 105
> > <https://reviews.apache.org/r/19240/diff/1/?file=520036#file520036line105>
> >
> > Javadocs would be nice, I think you can generally just copy/paste them from the MonitoringClient interface
fixed
> On March 15, 2014, 12:31 a.m., Kanak Biscuitwala wrote:
> > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java, line 88
> > <https://reviews.apache.org/r/19240/diff/1/?file=520036#file520036line88>
> >
> > Make the 3 a constant
fixed
> On March 15, 2014, 12:31 a.m., Kanak Biscuitwala wrote:
> > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java, line 62
> > <https://reviews.apache.org/r/19240/diff/1/?file=520036#file520036line62>
> >
> > private
fixed
> On March 15, 2014, 12:31 a.m., Kanak Biscuitwala wrote:
> > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java, line 44
> > <https://reviews.apache.org/r/19240/diff/1/?file=520036#file520036line44>
> >
> > Make these variables private unless package access is absolutely necessary.
fixed
> On March 15, 2014, 12:31 a.m., Kanak Biscuitwala wrote:
> > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java, line 34
> > <https://reviews.apache.org/r/19240/diff/1/?file=520036#file520036line34>
> >
> > If we're only using this internally, it should be package scope.
fixed
- Zhen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19240/#review37297
-----------------------------------------------------------
On March 14, 2014, 10:17 p.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19240/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 10:17 p.m.)
>
>
> Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> another refactoring based on last iteration:
> 1) remove spectating on monitoring cluster, use a simple round-robin approach to find the first available monitoring server
> 2) add a sharding key in MonitoringEvent
> 3) refactor AlertProxy, using direct java function call from clj instead of jetty/http
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java 743f8b4
> helix-core/src/main/java/org/apache/helix/monitoring/MonitoringEvent.java 80006fb
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/ClientUtil.java e69de29
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java e69de29
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java e69de29
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAgent.java 61e3d6c
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAlertProxy.java 5fe8ebe
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannConfigs.java 193b763
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/IntegrationTest.java 9d28dc3
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/MonitoringTestHelper.java e69de29
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestClientServerMonitoring.java 588bc35
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannAgent.java 39ece64
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannAlertProxy.java 700682a
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannClientWrapper.java e69de29
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannMonitoringServer.java f7d0585
>
> Diff: https://reviews.apache.org/r/19240/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 19240: [HELIX-319] refactor MonitoringClient to
accommodate distributed monitoring server
Posted by Kanak Biscuitwala <ka...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19240/#review37297
-----------------------------------------------------------
helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java
<https://reviews.apache.org/r/19240/#comment68733>
If we're only using this internally, it should be package scope.
helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java
<https://reviews.apache.org/r/19240/#comment68728>
Make these variables private unless package access is absolutely necessary.
helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java
<https://reviews.apache.org/r/19240/#comment68727>
Java volatile sometimes has unpredictable behavior. I would prefer an AtomicReference.
helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java
<https://reviews.apache.org/r/19240/#comment68730>
private
helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java
<https://reviews.apache.org/r/19240/#comment68734>
Make the 3 a constant
helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java
<https://reviews.apache.org/r/19240/#comment68731>
Javadocs would be nice, I think you can generally just copy/paste them from the MonitoringClient interface
helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java
<https://reviews.apache.org/r/19240/#comment68732>
This should be something other than public
helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java
<https://reviews.apache.org/r/19240/#comment68740>
Add some high-level javadocs
helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java
<https://reviews.apache.org/r/19240/#comment68729>
Same comment. Make these private if possible.
helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java
<https://reviews.apache.org/r/19240/#comment68737>
private
helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java
<https://reviews.apache.org/r/19240/#comment68739>
If all raw clients fail to connect immediately, this should be in a connecting state, right?
helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAgent.java
<https://reviews.apache.org/r/19240/#comment68743>
make these private
helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAgent.java
<https://reviews.apache.org/r/19240/#comment68742>
This should include some argument for host
helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannClientWrapper.java
<https://reviews.apache.org/r/19240/#comment68744>
Remove these TODOs
- Kanak Biscuitwala
On March 14, 2014, 3:17 p.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19240/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 3:17 p.m.)
>
>
> Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> another refactoring based on last iteration:
> 1) remove spectating on monitoring cluster, use a simple round-robin approach to find the first available monitoring server
> 2) add a sharding key in MonitoringEvent
> 3) refactor AlertProxy, using direct java function call from clj instead of jetty/http
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java 743f8b4
> helix-core/src/main/java/org/apache/helix/monitoring/MonitoringEvent.java 80006fb
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/ClientUtil.java e69de29
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/RawRiemannClient.java e69de29
> helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannClientWrapper.java e69de29
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAgent.java 61e3d6c
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannAlertProxy.java 5fe8ebe
> helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannConfigs.java 193b763
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/IntegrationTest.java 9d28dc3
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/MonitoringTestHelper.java e69de29
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestClientServerMonitoring.java 588bc35
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannAgent.java 39ece64
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannAlertProxy.java 700682a
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannClientWrapper.java e69de29
> helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestRiemannMonitoringServer.java f7d0585
>
> Diff: https://reviews.apache.org/r/19240/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>