----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46856/#review131525 -----------------------------------------------------------
Fix it, then Ship it! Looks good. Mostly minor stuff that you can choose to take or ignore. samza-core/src/main/java/org/apache/samza/coordinator/StreamPartitionCountMonitor.java (line 53) <https://reviews.apache.org/r/46856/#comment195535> cosmetic: extra WS. samza-core/src/main/java/org/apache/samza/coordinator/StreamPartitionCountMonitor.java (line 93) <https://reviews.apache.org/r/46856/#comment195536> cosmetic: maybe call this initialPartitionCount? Initially when I saw prevPartitionCount I thought that you might be saving the current count for the next loop, which would be problematic (e.g. non-zero delta missed due to polling interval). This approach looks good, but naming could be slightly clearer. samza-core/src/main/java/org/apache/samza/coordinator/StreamPartitionCountMonitor.java (line 107) <https://reviews.apache.org/r/46856/#comment195538> 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? samza-core/src/main/java/org/apache/samza/coordinator/StreamPartitionCountMonitor.java (line 127) <https://reviews.apache.org/r/46856/#comment195540> This looks like it could be made static (e.g. as a static helper function) as it doesn't require any state from the class. samza-core/src/main/java/org/apache/samza/coordinator/StreamPartitionCountMonitor.java (line 184) <https://reviews.apache.org/r/46856/#comment195544> state should be volatile otherwise you're not guaranteed visibility. I believe the test code is accessing this in a thread-safe way, but if this ever gets promoted to a public method it could lead to surprising behavior. Minor note: usually state == State.RUNNING is sufficient. I think you're checking the terminated flag for test, but the test code is already doing a timed wait, so you could switch it to `assertTrue(monitor.awaitTermination(...))` for the same effect. - Chris Pettitt 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 > >
