You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Aleksandar Bircakovic <a....@levi9.com> on 2015/08/19 10:14:17 UTC
Review Request 37604: SAMZA-760 Samza Container should catch
Throwables instead of just catching Exceptions
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37604/
-----------------------------------------------------------
Review request for samza.
Repository: samza
Description
-------
Added a catch for Throwables in Samza container. Catching Throwables can cause problems in specific situations so I also added a partial function 'safely' that should take care of that specific situations.
Diffs
-----
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 85b012b
Diff: https://reviews.apache.org/r/37604/diff/
Testing
-------
Thanks,
Aleksandar Bircakovic
Re: Review Request 37604: SAMZA-760 Samza Container should catch
Throwables instead of just catching Exceptions
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37604/#review101686
-----------------------------------------------------------
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 599)
<https://reviews.apache.org/r/37604/#comment159142>
Cool! Thanks for explanation!
- Yi Pan (Data Infrastructure)
On Aug. 25, 2015, 11:48 a.m., Aleksandar Bircakovic wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37604/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2015, 11:48 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> Added a catch for Throwables in Samza container. Catching Throwables can cause problems in specific situations so I also added a partial function 'safely' that should take care of that specific situations.
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 85b012b
> samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala 4db6d5c
>
> Diff: https://reviews.apache.org/r/37604/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aleksandar Bircakovic
>
>
Re: Review Request 37604: SAMZA-760 Samza Container should catch
Throwables instead of just catching Exceptions
Posted by Aleksandar Bircakovic <a....@levi9.com>.
> On Oct. 1, 2015, 7:44 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, line 599
> > <https://reviews.apache.org/r/37604/diff/2/?file=1051095#file1051095line599>
> >
> > Forgive me on ignorance on this ControlThrowable. Why should we skip logging this one? You mentioned in the RB description that there are some nuance involved by catching all Throwables. Could you elaborate a bit more here?
> >
> > Thanks!
Thank you for review. I found some articles saying that catching Throwables in Scala isn't so wise (like this one https://www.sumologic.com/2014/05/05/why-you-should-never-catch-throwable-in-scala/). They say it can have negative impact on JVM.
> On Oct. 1, 2015, 7:44 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, line 582
> > <https://reviews.apache.org/r/37604/diff/2/?file=1051095#file1051095line582>
> >
> > The bug description states that making the Throwable as a cause of the SamzaException, I would perfer to use SamzaException(String s, Throwable t) s.t. more detailed cause info would be print out.
Agree with that.
- Aleksandar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37604/#review101272
-----------------------------------------------------------
On Aug. 25, 2015, 11:48 a.m., Aleksandar Bircakovic wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37604/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2015, 11:48 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> Added a catch for Throwables in Samza container. Catching Throwables can cause problems in specific situations so I also added a partial function 'safely' that should take care of that specific situations.
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 85b012b
> samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala 4db6d5c
>
> Diff: https://reviews.apache.org/r/37604/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aleksandar Bircakovic
>
>
Re: Review Request 37604: SAMZA-760 Samza Container should catch
Throwables instead of just catching Exceptions
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37604/#review101272
-----------------------------------------------------------
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 582)
<https://reviews.apache.org/r/37604/#comment158652>
The bug description states that making the Throwable as a cause of the SamzaException, I would perfer to use SamzaException(String s, Throwable t) s.t. more detailed cause info would be print out.
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 599)
<https://reviews.apache.org/r/37604/#comment158651>
Forgive me on ignorance on this ControlThrowable. Why should we skip logging this one? You mentioned in the RB description that there are some nuance involved by catching all Throwables. Could you elaborate a bit more here?
Thanks!
- Yi Pan (Data Infrastructure)
On Aug. 25, 2015, 11:48 a.m., Aleksandar Bircakovic wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37604/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2015, 11:48 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> Added a catch for Throwables in Samza container. Catching Throwables can cause problems in specific situations so I also added a partial function 'safely' that should take care of that specific situations.
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 85b012b
> samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala 4db6d5c
>
> Diff: https://reviews.apache.org/r/37604/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aleksandar Bircakovic
>
>
Re: Review Request 37604: SAMZA-760 Samza Container should catch
Throwables instead of just catching Exceptions
Posted by Aleksandar Bircakovic <a....@levi9.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37604/
-----------------------------------------------------------
(Updated Oct. 8, 2015, 9:42 a.m.)
Review request for samza.
Changes
-------
Used SamzaException(String s, Throwable t) instead of SamzaException(String s) as Yi Pan suggested.
Repository: samza
Description
-------
Added a catch for Throwables in Samza container. Catching Throwables can cause problems in specific situations so I also added a partial function 'safely' that should take care of that specific situations.
Diffs (updated)
-----
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala f351ad6
samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala 6de8710
Diff: https://reviews.apache.org/r/37604/diff/
Testing
-------
Thanks,
Aleksandar Bircakovic
Re: Review Request 37604: SAMZA-760 Samza Container should catch
Throwables instead of just catching Exceptions
Posted by Aleksandar Bircakovic <a....@levi9.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37604/
-----------------------------------------------------------
(Updated Aug. 25, 2015, 11:48 a.m.)
Review request for samza.
Changes
-------
Added a unit test for verification.
Repository: samza
Description
-------
Added a catch for Throwables in Samza container. Catching Throwables can cause problems in specific situations so I also added a partial function 'safely' that should take care of that specific situations.
Diffs (updated)
-----
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 85b012b
samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala 4db6d5c
Diff: https://reviews.apache.org/r/37604/diff/
Testing
-------
Thanks,
Aleksandar Bircakovic
Re: Review Request 37604: SAMZA-760 Samza Container should catch
Throwables instead of just catching Exceptions
Posted by Yan Fang <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37604/#review96145
-----------------------------------------------------------
Overall, LGTM. Could you also add a unit test to verify this? Thank you.
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 581)
<https://reviews.apache.org/r/37604/#comment151430>
throwable, not exception in the log msg
- Yan Fang
On Aug. 19, 2015, 8:14 a.m., Aleksandar Bircakovic wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37604/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2015, 8:14 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> Added a catch for Throwables in Samza container. Catching Throwables can cause problems in specific situations so I also added a partial function 'safely' that should take care of that specific situations.
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 85b012b
>
> Diff: https://reviews.apache.org/r/37604/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aleksandar Bircakovic
>
>