> On Feb. 25, 2014, 8:59 a.m., Sandeep Nayak wrote: > > helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannMonitoringServer.java, > > line 123 > > <https://reviews.apache.org/r/18455/diff/1/?file=503199#file503199line123> > > > > Why synchronize the whole method if the idea is to control creation of > > the files why not guard it with a semaphore?
removed this method from MonitoringServer > On Feb. 25, 2014, 8:59 a.m., Sandeep Nayak wrote: > > helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java, > > line 47 > > <https://reviews.apache.org/r/18455/diff/1/?file=503193#file503193line47> > > > > If you add it to the event it is a "uses" contract which does not break > > most users. If you add it to the send API its a "provides" contract which > > breaks most implementers. So I would suggest adding it to the event. since we are switching to distributed monitoring service, it requires a way to shard the clients. Since we have to add a ResourceId argument to MonitoringClient#every(), it would be more consistent to have ResourceId argument to MonitoringClient#send() methods also. > On Feb. 25, 2014, 8:59 a.m., Sandeep Nayak wrote: > > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannMonitoringClient.java, > > line 66 > > <https://reviews.apache.org/r/18455/diff/1/?file=503198#file503198line66> > > > > This is being added to a collection should this not implement hashcode > > and equals? it's index by ResourceId which implements hashcode and equals already > On Feb. 25, 2014, 8:59 a.m., Sandeep Nayak wrote: > > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannMonitoringClient.java, > > line 332 > > <https://reviews.apache.org/r/18455/diff/1/?file=503198#file503198line332> > > > > Could host be empty? RiemannMonitoringClient#connectInternal() does an actual connection. If the host is empty of illegal, it will be caught there. > On Feb. 25, 2014, 8:59 a.m., Sandeep Nayak wrote: > > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannMonitoringClient.java, > > line 237 > > <https://reviews.apache.org/r/18455/diff/1/?file=503198#file503198line237> > > > > Can this not be tightened do we need to synchronize the whole method? the whole method is basically adding an item to a scheduling list which this synchronized is protecting. > On Feb. 25, 2014, 8:59 a.m., Sandeep Nayak wrote: > > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannMonitoringClient.java, > > line 83 > > <https://reviews.apache.org/r/18455/diff/1/?file=503198#file503198line83> > > > > Why use a concurrentmap when we are not using any of the concurrency > > variants on the map? The code seems to be using synchronized methods to > > access the map. we might have a writer and a reader concurrently. not sure if normal HashMap works in this situation. - Zhen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18455/#review35379 ----------------------------------------------------------- On Feb. 25, 2014, 6:01 a.m., Zhen Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18455/ > ----------------------------------------------------------- > > (Updated Feb. 25, 2014, 6:01 a.m.) > > > Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna. > > > Repository: helix-git > > > Description > ------- > > [HELIX-319] refactor MonitoringClient to accommodate distributed monitoring > server > > > Diffs > ----- > > helix-core/src/main/java/org/apache/helix/HelixAutoController.java fdab2a6 > helix-core/src/main/java/org/apache/helix/HelixController.java 098fd96 > helix-core/src/main/java/org/apache/helix/HelixManager.java 17c94e5 > > helix-core/src/main/java/org/apache/helix/manager/zk/HelixConnectionAdaptor.java > 65a192a > helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java > 83eba53 > > helix-core/src/main/java/org/apache/helix/manager/zk/ZkHelixAutoController.java > 3967ed9 > helix-core/src/main/java/org/apache/helix/manager/zk/ZkHelixController.java > 9da59b9 > > helix-core/src/main/java/org/apache/helix/manager/zk/ZkHelixLeaderElection.java > 3c5b3db > helix-core/src/main/java/org/apache/helix/monitoring/MonitoringClient.java > a794eab > helix-core/src/main/java/org/apache/helix/monitoring/MonitoringServer.java > 3168bb2 > helix-core/src/test/java/org/apache/helix/Mocks.java 87d4e68 > > helix-core/src/test/java/org/apache/helix/controller/stages/DummyClusterManager.java > e07f0b5 > > helix-core/src/test/java/org/apache/helix/participant/MockZKHelixManager.java > 0b8395e > > helix-monitor-client/src/main/java/org/apache/helix/monitoring/RiemannMonitoringClient.java > 20b0825 > > helix-monitor-server/src/main/java/org/apache/helix/monitoring/RiemannMonitoringServer.java > 36719aa > > helix-monitor-server/src/test/java/org/apache/helix/monitoring/BasicMonitoringTest.java > 6680f00 > > helix-monitor-server/src/test/java/org/apache/helix/monitoring/TestClientServerMonitoring.java > 8b7f839 > > Diff: https://reviews.apache.org/r/18455/diff/ > > > Testing > ------- > > > Thanks, > > Zhen Zhang > >
