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