> 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)
>
>
