> 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 > >
