> On May 3, 2016, 5:23 p.m., Chris Pettitt wrote: > > samza-core/src/main/java/org/apache/samza/coordinator/StreamPartitionCountMonitor.java, > > line 107 > > <https://reviews.apache.org/r/46856/diff/2/?file=1367775#file1367775line107> > > > > This is not thread-safe, but I believe it could be made so without > > requiring volatile / atomics / locks. Could we initialize all of the guages > > in the constructor and place them in an immutable map? > > Jake Maes wrote: > Does it need to be thread-safe? The executor is single-threaded, start() > is idempotent, and this method is private. Is there a hole I'm not seeing?
Yes, that is a fair argument to make. However, those assumptions may not hold over time. Assuming that we can construct these in the constructor and make the map immutable at no cost, I think we should make this thread safe. There are also assumptions about how the tests are run. For example, getGauges exposes the map, which would require that test code does enough to guarantee visibility (this holds now, may not later). If there is a cost to make this thread-safe then this is not as clear cut (though I know how I'd bias). Would be interested in the details if this is the case. - Chris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46856/#review131525 ----------------------------------------------------------- On April 29, 2016, 11:38 p.m., Jake Maes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46856/ > ----------------------------------------------------------- > > (Updated April 29, 2016, 11:38 p.m.) > > > Review request for samza, Boris Shkolnik, Navina Ramesh, Jagadish > Venkatraman, and Yi Pan (Data Infrastructure). > > > Bugs: SAMZA-943 > https://issues.apache.org/jira/browse/SAMZA-943 > > > Repository: samza > > > Description > ------- > > SAMZA-943 Occasional test failure: > TestStreamPartitionCountMonitor.testStartStopBehavior > > * Rewrote the monitor in Java following the pattern of the > PollingScanDiskSpaceMonitor in SAMZA-924 > ** The main difference is that it uses a ScheduledExecutorService to > cleanly run the monitor in a loop and provide determinism around startup and > shutdown > * Got rid of the sleep() in the unit test > * Added a unit test to verify the scheduler calls the monitor method > * Enforced that the monitor isn't restarted (which is a problem for the > scheduler service) > ** This required that the reference to the monitor not be static (defined > in the JobCoordinator object) and instead instantiated whenever the > JobCoordinator is instantiated. > > > Diffs > ----- > > checkstyle/import-control.xml c15b8e74de8e5aac5ac83278c52ab3dba1630e50 > > samza-core/src/main/java/org/apache/samza/coordinator/StreamPartitionCountMonitor.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > 384b2e777c73fc1e4bc8a29312c9ea5372162ca1 > > samza-core/src/main/scala/org/apache/samza/coordinator/StreamPartitionCountMonitor.scala > 6aeff5787a0018ca2cae7d901c25537fbc7dea23 > > samza-core/src/test/scala/org/apache/samza/coordinator/TestStreamPartitionCountMonitor.scala > f47f8189bd92c4071ae76ae323e066823f3a6f61 > > Diff: https://reviews.apache.org/r/46856/diff/ > > > Testing > ------- > > Added a test. > > Ran check-all.sh > > > Thanks, > > Jake Maes > >
