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

Reply via email to