You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Jake Maes <ja...@gmail.com> on 2016/04/30 00:06:37 UTC
Review Request 46856: SAMZA-943 Occasional test failure:
TestStreamPartitionCountMonitor.testStartStopBehavior
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46856/
-----------------------------------------------------------
Review request for samza, Boris Shkolnik, Navina Ramesh, Jagadish Venkatraman, and Yi Pan (Data Infrastructure).
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
Re: Review Request 46856: SAMZA-943 Occasional test failure:
TestStreamPartitionCountMonitor.testStartStopBehavior
Posted by Jake Maes <ja...@gmail.com>.
> 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
>
>
Re: Review Request 46856: SAMZA-943 Occasional test failure:
TestStreamPartitionCountMonitor.testStartStopBehavior
Posted by Jake Maes <ja...@gmail.com>.
> On May 3, 2016, 5:23 p.m., Chris Pettitt wrote:
> > Looks good. Mostly minor stuff that you can choose to take or ignore.
Appreciate the thoughtful comments. Implemented all of them.
- Jake
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46856/#review131525
-----------------------------------------------------------
On May 4, 2016, 12:18 a.m., Jake Maes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46856/
> -----------------------------------------------------------
>
> (Updated May 4, 2016, 12:18 a.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
>
>
Re: Review Request 46856: SAMZA-943 Occasional test failure:
TestStreamPartitionCountMonitor.testStartStopBehavior
Posted by Chris Pettitt <cp...@linkedin.com>.
> 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
>
>
Re: Review Request 46856: SAMZA-943 Occasional test failure:
TestStreamPartitionCountMonitor.testStartStopBehavior
Posted by Jake Maes <ja...@gmail.com>.
> 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?
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?
- 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
>
>
Re: Review Request 46856: SAMZA-943 Occasional test failure:
TestStreamPartitionCountMonitor.testStartStopBehavior
Posted by Chris Pettitt <cp...@linkedin.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 46856: SAMZA-943 Occasional test failure:
TestStreamPartitionCountMonitor.testStartStopBehavior
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46856/#review132079
-----------------------------------------------------------
Ship it!
Looks good ! +1
- Navina Ramesh
On May 5, 2016, 8:41 p.m., Jake Maes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46856/
> -----------------------------------------------------------
>
> (Updated May 5, 2016, 8:41 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 8d774864fe19418867da3a724c02939bb1e3e615
> 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
>
>
Re: Review Request 46856: SAMZA-943 Occasional test failure:
TestStreamPartitionCountMonitor.testStartStopBehavior
Posted by Jake Maes <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46856/
-----------------------------------------------------------
(Updated May 5, 2016, 8:41 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 (updated)
-----
checkstyle/import-control.xml 8d774864fe19418867da3a724c02939bb1e3e615
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
Re: Review Request 46856: SAMZA-943 Occasional test failure:
TestStreamPartitionCountMonitor.testStartStopBehavior
Posted by Jake Maes <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46856/
-----------------------------------------------------------
(Updated May 4, 2016, 12:30 a.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 (updated)
-----
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
Re: Review Request 46856: SAMZA-943 Occasional test failure:
TestStreamPartitionCountMonitor.testStartStopBehavior
Posted by Jake Maes <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46856/
-----------------------------------------------------------
(Updated May 4, 2016, 12:18 a.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 (updated)
-----
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
Re: Review Request 46856: SAMZA-943 Occasional test failure:
TestStreamPartitionCountMonitor.testStartStopBehavior
Posted by Jake Maes <ja...@gmail.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 46856: SAMZA-943 Occasional test failure:
TestStreamPartitionCountMonitor.testStartStopBehavior
Posted by Jake Maes <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46856/
-----------------------------------------------------------
(Updated April 29, 2016, 10:06 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