> 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? > > Chris Pettitt wrote: > 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.
Ahh, no, it wasn't about cost. I just wasn't clear that this was about future-proofing. I thought maybe you saw a bug that I was missing. I'm all for it. Stay tuned. - Jake ----------------------------------------------------------- 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 > >
