You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Chris Riccomini <cr...@apache.org> on 2014/08/19 21:46:20 UTC
Review Request 24863: SAMZA-382
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24863/
-----------------------------------------------------------
Review request for samza.
Bugs: SAMZA-382
https://issues.apache.org/jira/browse/SAMZA-382
Repository: samza
Description
-------
move main method into a separate method to allow a mock-able JmxServer. write a test to validate that the jmx server is not properly called on certain exceptions. fix by moving the try block to include all logic in the main method.
fix random typo in comments
Diffs
-----
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala d574ac413c0ec81e12eb44b2d0cc0d9843aad434
samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala e3c7fe3e2d329b0767eb439144b1ba419848bb96
Diff: https://reviews.apache.org/r/24863/diff/
Testing
-------
Thanks,
Chris Riccomini
Re: Review Request 24863: SAMZA-382
Posted by Chris Riccomini <cr...@apache.org>.
> On Aug. 19, 2014, 7:58 p.m., Chinmay Soman wrote:
> > samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala, line 58
> > <https://reviews.apache.org/r/24863/diff/1/?file=664733#file664733line58>
> >
> > I guess when the NPE gets fixed, we will have to modify this test.
> >
> > How about recreating the same problem : No task for a given SSP - to make safeMain throw an exception ?
I opted to just catch exception. This seems to be the most robust way to handle it. I'm reluctant to mess with environment variables in a unit test since it is not thread safe, and could get leaked beyond the one test. I'm also reluctant to make the main method further injectible since the code is barely readable as it is.
I think we really need to either expand, or have a similar class as, the CommandLine.scala class, which lets us properly handle this kind of stuff in a unit-testable/mockable way. Both SamzaAppMaster and SamzaContainer could benefit from a bit of refactoring.
I'll open a separate ticket for that.
- Chris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24863/#review51026
-----------------------------------------------------------
On Aug. 19, 2014, 8:50 p.m., Chris Riccomini wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24863/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2014, 8:50 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-382
> https://issues.apache.org/jira/browse/SAMZA-382
>
>
> Repository: samza
>
>
> Description
> -------
>
> switching to catch any exception
>
>
> Revert "do environment variable validation in SamzaContainer. clean up JMX test in SamzaContainer to use container name exception."
>
> This reverts commit efe56adbf5ac9327f8781da4186f8bb438bd5eb8.
>
> do environment variable validation in SamzaContainer. clean up JMX test in SamzaContainer to use container name exception.
>
>
> move main method into a separate method to allow a mock-able JmxServer. write a test to validate that the jmx server is not properly called on certain exceptions. fix by moving the try block to include all logic in the main method.
>
>
> fix random typo in comments
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala d574ac413c0ec81e12eb44b2d0cc0d9843aad434
> samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala e3c7fe3e2d329b0767eb439144b1ba419848bb96
>
> Diff: https://reviews.apache.org/r/24863/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chris Riccomini
>
>
Re: Review Request 24863: SAMZA-382
Posted by Chinmay Soman <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24863/#review51026
-----------------------------------------------------------
samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala
<https://reviews.apache.org/r/24863/#comment88907>
I guess when the NPE gets fixed, we will have to modify this test.
How about recreating the same problem : No task for a given SSP - to make safeMain throw an exception ?
- Chinmay Soman
On Aug. 19, 2014, 7:46 p.m., Chris Riccomini wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24863/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2014, 7:46 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-382
> https://issues.apache.org/jira/browse/SAMZA-382
>
>
> Repository: samza
>
>
> Description
> -------
>
> move main method into a separate method to allow a mock-able JmxServer. write a test to validate that the jmx server is not properly called on certain exceptions. fix by moving the try block to include all logic in the main method.
>
>
> fix random typo in comments
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala d574ac413c0ec81e12eb44b2d0cc0d9843aad434
> samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala e3c7fe3e2d329b0767eb439144b1ba419848bb96
>
> Diff: https://reviews.apache.org/r/24863/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chris Riccomini
>
>
Re: Review Request 24863: SAMZA-382
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24863/
-----------------------------------------------------------
(Updated Aug. 19, 2014, 8:50 p.m.)
Review request for samza.
Bugs: SAMZA-382
https://issues.apache.org/jira/browse/SAMZA-382
Repository: samza
Description (updated)
-------
switching to catch any exception
Revert "do environment variable validation in SamzaContainer. clean up JMX test in SamzaContainer to use container name exception."
This reverts commit efe56adbf5ac9327f8781da4186f8bb438bd5eb8.
do environment variable validation in SamzaContainer. clean up JMX test in SamzaContainer to use container name exception.
move main method into a separate method to allow a mock-able JmxServer. write a test to validate that the jmx server is not properly called on certain exceptions. fix by moving the try block to include all logic in the main method.
fix random typo in comments
Diffs (updated)
-----
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala d574ac413c0ec81e12eb44b2d0cc0d9843aad434
samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala e3c7fe3e2d329b0767eb439144b1ba419848bb96
Diff: https://reviews.apache.org/r/24863/diff/
Testing
-------
Thanks,
Chris Riccomini