You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com> on 2015/03/23 18:39:55 UTC
Review Request 32407: SAMZA-571: add suppression interface for
uncaught exceptions
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32407/
-----------------------------------------------------------
Review request for samza, Yan Fang, Chinmay Soman, Chris Riccomini, Navina Ramesh, and Naveen Somasundaram.
Bugs: SAMZA-571
https://issues.apache.org/jira/browse/SAMZA-571
Repository: samza
Description
-------
[SAMZA-571] Adding task interface to allow customized handling of exceptions from user code in tasks
Just to add the first part: add ExceptionTask interface to allow user to add code to handle the exceptions from user code in the task.
commit is also wrapped w/ the ExceptionTask since some errors such as MessageSizeTooLargeException is not seen when user code calls send() but will be returned by Kafka broker and thrown back in commit()
Diffs
-----
samza-api/src/main/java/org/apache/samza/task/ExceptionTask.java PRE-CREATION
samza-api/src/main/java/org/apache/samza/task/exception/TaskExceptionContext.java PRE-CREATION
samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 1ca9e2cc5673c537b6a48224809847e94da81fca
samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala 4098235c8c13fad68c8aded3b2a8a4ef07c216e7
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 9fc3b557bdcc2756a0ddfed6642deb529936b7a9
samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala be0b55ace5b4b9d29f42da17fabac93bb6a25605
samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala 125d37602e2c0a9da75674f37580a1ac02f94796
samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala 54b4df84f47f818d62ac0361196567ad1f430fde
Diff: https://reviews.apache.org/r/32407/diff/
Testing
-------
Unit test added. Pass with ./gradlew clean build
Thanks,
Yi Pan (Data Infrastructure)
Re: Review Request 32407: SAMZA-571: add suppression interface for
uncaught exceptions
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
> On March 23, 2015, 11:29 p.m., Chris Riccomini wrote:
> > * Nit: 2 space, not 4 space for indentation.
> > * It's kind of odd to have exceptionHandler.maybeHandle { maybeHandle(coordinator, envelope, tryBlock = { ... I had a comment in the RB about consolidating this.
> > * TestTaskInstance.scala has a bunch of red-highlighted (in RB) white space indents that should be removed.
@Chris, I agree. I was working on another patch to remove the TaskInstanceExceptionHandler completely to consolidate this.
> On March 23, 2015, 11:29 p.m., Chris Riccomini wrote:
> > samza-api/src/main/java/org/apache/samza/task/ExceptionTask.java, line 25
> > <https://reviews.apache.org/r/32407/diff/1/?file=903332#file903332line25>
> >
> > Javadoc
Ooops. Forgot that. Will add it.
> On March 23, 2015, 11:29 p.m., Chris Riccomini wrote:
> > samza-api/src/main/java/org/apache/samza/task/exception/TaskExceptionContext.java, line 28
> > <https://reviews.apache.org/r/32407/diff/1/?file=903333#file903333line28>
> >
> > Rather than adding more contexts, I think we should just keep method signatures like we have with init() and process():
> >
> > exception(Exception e, MessageCollector collector, TaskCoordinator coordinator)
> >
> > Also, rather than having a lot of potentially null parameters, what do you think about having multiple callbacks in ExceptionTask, each for different points in the lifecycle (e.g. a callback with no coordinator for init, a callback with no incoming message envelope for window/commit, etc)?
> >
> > I'm a little concerned about usability if any param can be null at any given time. It seems more intuitive to make the callbacks explicit by having different params for different points in the lifecycle. Could even have methods names like: initException(), processException(), etc.
Good point. Let me try to refactor it w/ multiple specific APIs to avoid the null parameters here. I was not really sure what we would need in each of the exceptional cases.
> On March 23, 2015, 11:29 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala, line 95
> > <https://reviews.apache.org/r/32407/diff/1/?file=903334#file903334line95>
> >
> > Is this ever used?
Good catch. I forgot to remove it from another experimental patch. Will remove it.
> On March 23, 2015, 11:29 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala, line 83
> > <https://reviews.apache.org/r/32407/diff/1/?file=903335#file903335line83>
> >
> > Seems like white space got out of whack here.
will do.
> On March 23, 2015, 11:29 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala, line 153
> > <https://reviews.apache.org/r/32407/diff/1/?file=903337#file903337line153>
> >
> > It seems like we shouldn't have two of these here. I think TaskInstanceExceptionHandler should implement ExceptionTask, and be used when isExceptionTask is false.
I am concerned that when some user implements the ExceptionTask *and* have a configured suppression, he will lose the configured suppression by surprise. I am working on removing the exceptionHandler and handle both default configured suppression and the invocation of ExceptionTask methods in the single place. Then, we can deprecate the configured suppression w/o actually changing the behavior of the suppressed exception, and retire it later. If we can live w/ the fact that if user implements ExceptionTask automatically disables all configured suppressions, I would go ahead w/ your suggestion.
Thanks!
- Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32407/#review77470
-----------------------------------------------------------
On March 23, 2015, 5:39 p.m., Yi Pan (Data Infrastructure) wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32407/
> -----------------------------------------------------------
>
> (Updated March 23, 2015, 5:39 p.m.)
>
>
> Review request for samza, Yan Fang, Chinmay Soman, Chris Riccomini, Navina Ramesh, and Naveen Somasundaram.
>
>
> Bugs: SAMZA-571
> https://issues.apache.org/jira/browse/SAMZA-571
>
>
> Repository: samza
>
>
> Description
> -------
>
> [SAMZA-571] Adding task interface to allow customized handling of exceptions from user code in tasks
>
> Just to add the first part: add ExceptionTask interface to allow user to add code to handle the exceptions from user code in the task.
>
> commit is also wrapped w/ the ExceptionTask since some errors such as MessageSizeTooLargeException is not seen when user code calls send() but will be returned by Kafka broker and thrown back in commit()
>
>
> Diffs
> -----
>
> samza-api/src/main/java/org/apache/samza/task/ExceptionTask.java PRE-CREATION
> samza-api/src/main/java/org/apache/samza/task/exception/TaskExceptionContext.java PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 1ca9e2cc5673c537b6a48224809847e94da81fca
> samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala 4098235c8c13fad68c8aded3b2a8a4ef07c216e7
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 9fc3b557bdcc2756a0ddfed6642deb529936b7a9
> samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala be0b55ace5b4b9d29f42da17fabac93bb6a25605
> samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala 125d37602e2c0a9da75674f37580a1ac02f94796
> samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala 54b4df84f47f818d62ac0361196567ad1f430fde
>
> Diff: https://reviews.apache.org/r/32407/diff/
>
>
> Testing
> -------
>
> Unit test added. Pass with ./gradlew clean build
>
>
> Thanks,
>
> Yi Pan (Data Infrastructure)
>
>
Re: Review Request 32407: SAMZA-571: add suppression interface for
uncaught exceptions
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32407/#review77470
-----------------------------------------------------------
* Nit: 2 space, not 4 space for indentation.
* It's kind of odd to have exceptionHandler.maybeHandle { maybeHandle(coordinator, envelope, tryBlock = { ... I had a comment in the RB about consolidating this.
* TestTaskInstance.scala has a bunch of red-highlighted (in RB) white space indents that should be removed.
samza-api/src/main/java/org/apache/samza/task/ExceptionTask.java
<https://reviews.apache.org/r/32407/#comment125559>
Javadoc
samza-api/src/main/java/org/apache/samza/task/exception/TaskExceptionContext.java
<https://reviews.apache.org/r/32407/#comment125561>
Rather than adding more contexts, I think we should just keep method signatures like we have with init() and process():
exception(Exception e, MessageCollector collector, TaskCoordinator coordinator)
Also, rather than having a lot of potentially null parameters, what do you think about having multiple callbacks in ExceptionTask, each for different points in the lifecycle (e.g. a callback with no coordinator for init, a callback with no incoming message envelope for window/commit, etc)?
I'm a little concerned about usability if any param can be null at any given time. It seems more intuitive to make the callbacks explicit by having different params for different points in the lifecycle. Could even have methods names like: initException(), processException(), etc.
samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala
<https://reviews.apache.org/r/32407/#comment125577>
Is this ever used?
samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala
<https://reviews.apache.org/r/32407/#comment125578>
Seems like white space got out of whack here.
samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala
<https://reviews.apache.org/r/32407/#comment125579>
It seems like we shouldn't have two of these here. I think TaskInstanceExceptionHandler should implement ExceptionTask, and be used when isExceptionTask is false.
samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala
<https://reviews.apache.org/r/32407/#comment125580>
Nit: white space.
samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala
<https://reviews.apache.org/r/32407/#comment125581>
Nit: white space
- Chris Riccomini
On March 23, 2015, 5:39 p.m., Yi Pan (Data Infrastructure) wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32407/
> -----------------------------------------------------------
>
> (Updated March 23, 2015, 5:39 p.m.)
>
>
> Review request for samza, Yan Fang, Chinmay Soman, Chris Riccomini, Navina Ramesh, and Naveen Somasundaram.
>
>
> Bugs: SAMZA-571
> https://issues.apache.org/jira/browse/SAMZA-571
>
>
> Repository: samza
>
>
> Description
> -------
>
> [SAMZA-571] Adding task interface to allow customized handling of exceptions from user code in tasks
>
> Just to add the first part: add ExceptionTask interface to allow user to add code to handle the exceptions from user code in the task.
>
> commit is also wrapped w/ the ExceptionTask since some errors such as MessageSizeTooLargeException is not seen when user code calls send() but will be returned by Kafka broker and thrown back in commit()
>
>
> Diffs
> -----
>
> samza-api/src/main/java/org/apache/samza/task/ExceptionTask.java PRE-CREATION
> samza-api/src/main/java/org/apache/samza/task/exception/TaskExceptionContext.java PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 1ca9e2cc5673c537b6a48224809847e94da81fca
> samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala 4098235c8c13fad68c8aded3b2a8a4ef07c216e7
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 9fc3b557bdcc2756a0ddfed6642deb529936b7a9
> samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala be0b55ace5b4b9d29f42da17fabac93bb6a25605
> samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala 125d37602e2c0a9da75674f37580a1ac02f94796
> samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala 54b4df84f47f818d62ac0361196567ad1f430fde
>
> Diff: https://reviews.apache.org/r/32407/diff/
>
>
> Testing
> -------
>
> Unit test added. Pass with ./gradlew clean build
>
>
> Thanks,
>
> Yi Pan (Data Infrastructure)
>
>
Re: Review Request 32407: SAMZA-571: add suppression interface for
uncaught 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/32407/
-----------------------------------------------------------
(Updated March 25, 2015, 2:24 a.m.)
Review request for samza, Yan Fang, Chinmay Soman, Chris Riccomini, Navina Ramesh, and Naveen Somasundaram.
Changes
-------
Revised: add more exception callback methods to ExceptionTask interface for each specific task lifecycle stage. Consolidate invocation of ExceptionTask.exception() and the configured suppression in a single place.
Bugs: SAMZA-571
https://issues.apache.org/jira/browse/SAMZA-571
Repository: samza
Description (updated)
-------
[SAMZA-571] Adding task interface to allow customized handling of exceptions from user code in tasks
Just to add the first part: add ExceptionTask interface to allow user to add code to handle the exceptions from user code in the task's lifecycle: init(), process(), window(), and close().
Diffs (updated)
-----
samza-api/src/main/java/org/apache/samza/task/ExceptionTask.java PRE-CREATION
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 9fc3b557bdcc2756a0ddfed6642deb529936b7a9
samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala be0b55ace5b4b9d29f42da17fabac93bb6a25605
samza-core/src/main/scala/org/apache/samza/container/TaskInstanceExceptionHandler.scala 99b729f129344dee7d324d5889a3964c331f6521
samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala 54b4df84f47f818d62ac0361196567ad1f430fde
Diff: https://reviews.apache.org/r/32407/diff/
Testing (updated)
-------
Unit tests added. Pass with ./bin/check-all.sh
Thanks,
Yi Pan (Data Infrastructure)