Review Request 47283: SamzaCoordinator Refactor Part 2: UI related changes, dead code removal

2016-05-11 Thread Jagadish Venkatraman 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47283/
---

Review request for samza, Boris Shkolnik and Yi Pan (Data Infrastructure).


Repository: samza


Description
---

Continuation of the Refactoring from RB:44920:
1. SamzaAppState.finishedContainers was declared as a Set. The UI thread 
invokes the size() on the set, while others could modify the set causing a 
race. Since, the only place finishedContainers is read is from the UI, we can 
simply use an AtomicInteger to keep track of the counts. (instead of the Set).  
2. Changed all the UI and scaml files to work with the refactored code as in 
https://reviews.apache.org/r/44920
3. Modify the AMRest servlets, and AMWeb servlets to work with the refactoring 
as in  https://reviews.apache.org/r/44920 . We now have the AppMaster UI with 
the refactored patch :-)
3. Deleted several classes in the old code-path. Maintaining 2 code-paths 
increases tech debt.
4. Refactored out some MockClasses so that they can be re-used else-where too.
5. Refactored and Rewrote unit tests to work with the RB:44920


Diffs
-

  
samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ContainerProcessManager.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaApplicationState.java
 PRE-CREATION 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/TestContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/TestContainerProcessManager.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/TestHostAwareContainerAllocator.java
 PRE-CREATION 
  samza-shell/src/main/bash/run-am.sh 9545a96953baaff17ad14962e02bc12aadbb1101 
  
samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java
 b4789e62beb1120f11a8101664b10c34ae930e58 
  samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java 
24ac41067e34cf5a445bb036db9ea324eaafa7df 
  samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerFailure.java 
1d15651096e52a2e323a8e2f658fad3ea8e9c709 
  samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java 
57ce35099973b7fc3414c450e3246cb9f204289b 
  samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerUtil.java 
e8976bce934a324c48475bcd64d392119cc44b40 
  
samza-yarn/src/main/java/org/apache/samza/job/yarn/HostAwareContainerAllocator.java
 1d101fa80e6367d54f06455c24195e4c0091 
  samza-yarn/src/main/java/org/apache/samza/job/yarn/SamzaAppState.java 
77280bab8aeb242b34b5b780c84e6deab1a45f51 
  samza-yarn/src/main/java/org/apache/samza/job/yarn/SamzaContainerRequest.java 
4a04eb6b3b054ee85988e1b26ececc800bbc7861 
  samza-yarn/src/main/java/org/apache/samza/job/yarn/SamzaTaskManager.java 
caee6e6c182d3cf86bd4fe193f8b1797605b2480 
  samza-yarn/src/main/java/org/apache/samza/job/yarn/YarnAppState.java 
PRE-CREATION 
  samza-yarn/src/main/resources/scalate/WEB-INF/views/index.scaml 
93660c73e507b1a6076e743fcec56a41d22a5f39 
  samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMaster.scala 
80deb3b18c094d83af67535f9d0156f18ae3f5e4 
  
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterLifecycle.scala
 2a5c0d8092aa6f0a7f9b1f5fa56f9f4d4919d579 
  
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 
054d8b68033535ff0cab7cca84b71455e201a715 
  
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterService.scala 
3adf86f83123f6d66fc18ef9feed95b551c8398f 
  
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaYarnAppMasterService.scala
 PRE-CREATION 
  samza-yarn/src/main/scala/org/apache/samza/job/yarn/YarnJob.scala 
62ddb261f0b8f0f24468875d6d84da05908a3c62 
  
samza-yarn/src/main/scala/org/apache/samza/webapp/ApplicationMasterRestServlet.scala
 a40ab72a71c4fa82bd87aa03caec8936e609bf68 
  
samza-yarn/src/main/scala/org/apache/samza/webapp/ApplicationMasterWebServlet.scala
 605332a02f365248fc9ec3525ed9d3721db71e8d 
  
samza-yarn/src/test/java/org/apache/samza/job/yarn/TestContainerAllocator.java 
e21aded3c5e302593ed1cb5675da81aa6e3e743f 
  
samza-yarn/src/test/java/org/apache/samza/job/yarn/TestContainerAllocatorCommon.java
 5badd29af601ce0a046ef7dce4739e31514c4e63 
  
samza-yarn/src/test/java/org/apache/samza/job/yarn/TestContainerRequestState.java
 402fe784120ac40ed542d9fa60d6a6d7df9c8cda 
  
samza-yarn/src/test/java/org/apache/samza/job/yarn/TestHostAwareContainerAllocator.java
 ead7200487f27ed31e30576721ef16a064a28bc7 
  
samza-yarn/src/test/java/org/apache/samza/job/yarn/TestSamzaContainerRequest.java
 ad0f4d3d9db9111a7c0087b6da9c724dcc736726

Re: Review Request 47251: SAMZA-852 - Better logging when system can not be created Always log the exception when we cant instantiate a producer or consumer

2016-05-11 Thread Yi Pan (Data Infrastructure) 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47251/#review132796
---


Ship it!




Ship It!

- Yi Pan (Data Infrastructure)


On May 11, 2016, 6:53 p.m., Jake Maes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47251/
> ---
> 
> (Updated May 11, 2016, 6:53 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
> Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).
> 
> 
> Bugs: SAMZA-852
> https://issues.apache.org/jira/browse/SAMZA-852
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-852 - Better logging when system can not be created Always log the 
> exception when we cant instantiate a producer or consumer
> 
> 
> Diffs
> -
> 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> 5462208c08cddbfd30d886daffa8c02c82447059 
> 
> Diff: https://reviews.apache.org/r/47251/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Jake Maes
> 
>

Re: Review Request 47197: SAMZA-948 CoordinatorSystemStreamConsumer is not threadsafe

2016-05-11 Thread Jake Maes 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47197/
---

(Updated May 11, 2016, 11:55 p.m.)


Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).


Bugs: SAMZA-948
https://issues.apache.org/jira/browse/SAMZA-948


Repository: samza


Description
---

SAMZA-948 CoordinatorSystemStreamConsumer is not threadsafe

See the stack traces in the JIRA for more context. Essentially the consumer can 
bootstrap concurrently from multiple code paths (AM UI, RM Client callbacks, 
etc) and with the remove() logic that was added in SAMZA-913, we can get 
ConcurrentModificationExceptions. 

Fix:
* Use an AtomicReference to swap in the updated messages when they are ready 
* In bootstrap()
* Acquire a lock
* Make a copy of the messages
* Append the new messages
* Set the atomic reference to the copy
* Release lock

Also sneaking in a log message fix for JobCoordinator. It previously didn't 
include the task names.


Diffs (updated)
-

  
samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java
 8e1057b4d055159acb49d2cc60d3acad7665a532 
  samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
03f48db7f42b2617995b14cf51248b82b6cc2636 

Diff: https://reviews.apache.org/r/47197/diff/


Testing
---

./gradlew build


Thanks,

Jake Maes

Re: Review Request 47197: SAMZA-948 CoordinatorSystemStreamConsumer is not threadsafe

2016-05-11 Thread Jake Maes 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47197/
---

(Updated May 11, 2016, 11:49 p.m.)


Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).


Bugs: SAMZA-948
https://issues.apache.org/jira/browse/SAMZA-948


Repository: samza


Description
---

SAMZA-948 CoordinatorSystemStreamConsumer is not threadsafe

See the stack traces in the JIRA for more context. Essentially the consumer can 
bootstrap concurrently from multiple code paths (AM UI, RM Client callbacks, 
etc) and with the remove() logic that was added in SAMZA-913, we can get 
ConcurrentModificationExceptions. 

Fix:
* Use an AtomicReference to swap in the updated messages when they are ready 
* In bootstrap()
* Acquire a lock
* Make a copy of the messages
* Append the new messages
* Set the atomic reference to the copy
* Release lock

Also sneaking in a log message fix for JobCoordinator. It previously didn't 
include the task names.


Diffs (updated)
-

  
samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java
 8e1057b4d055159acb49d2cc60d3acad7665a532 
  samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
03f48db7f42b2617995b14cf51248b82b6cc2636 

Diff: https://reviews.apache.org/r/47197/diff/


Testing
---

./gradlew build


Thanks,

Jake Maes

Re: Review Request 47247: SAMZA-947 - TaskAssignmentManager registration exception when partition count changes.

2016-05-11 Thread Jake Maes 


> On May 11, 2016, 9:18 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java,
> >  line 42
> > 
> >
> > I guess that currently TaskAssignmentManager is still accessed via 
> > single thread? It is fine for now. But we should evaluate the need to make 
> > this thread safe when integrating with Xinyu's change.

TaskAssignmentManager should only be used in the JobCoordinator to 
refreshJobModel. So unless we have multiple threads refreshing the job model, 
it should be fine. And if we do, we should consider whether the job model 
refresh should be synchronous.


- Jake


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47247/#review132749
---


On May 11, 2016, 8:26 p.m., Jake Maes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47247/
> ---
> 
> (Updated May 11, 2016, 8:26 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
> Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).
> 
> 
> Bugs: SAMZA-947
> https://issues.apache.org/jira/browse/SAMZA-947
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-947 - TaskAssignmentManager registration exception when partition count 
> changes.
> 
> * Register the producer once at constructor time
> * Don't bother registering the consumer. It's lifecycle is outside the 
> TaskAssignmentManager and registering from the TAM ends up being a no-op.
> 
> 
> Diffs
> -
> 
>   
> samza-core/src/main/java/org/apache/samza/container/grouper/task/GroupByContainerCount.java
>  286ea1b3d0d39ebd7d9923a81c02c1d0842b1291 
>   
> samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
>  0cbdec8ac050de18c2fea191e3ef38273f1dbab1 
>   
> samza-core/src/test/java/org/apache/samza/container/grouper/task/TestTaskAssignmentManager.java
>  19ab78e891ca22b6fba430ded6b9382c860a212d 
> 
> Diff: https://reviews.apache.org/r/47247/diff/
> 
> 
> Testing
> ---
> 
> Manually tested with a job. Adjusted the input topics to induce a partition 
> change and verified no exceptions and the TaskAssignmentManager cleanup ran 
> appropriately.
> 
> 2016-05-11 17:34:33 GroupByContainerCount [WARN] Current task count 32 does 
> not match saved task count 512. Stateful jobs may observe misalignment of 
> keys!
> ...
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 15" moved 
> from container 57 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 14" moved 
> from container 46 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 13" moved 
> from container 35 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 19" moved 
> from container 101 to container null
> ...
> 
> 
> Thanks,
> 
> Jake Maes
> 
>

Re: Review Request 47197: SAMZA-948 CoordinatorSystemStreamConsumer is not threadsafe

2016-05-11 Thread Chris Pettitt 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47197/#review132785
---


Ship it!





samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java
 (line 66)


You actually don't need to wrap emptySet because it's already immutable.


- Chris Pettitt


On May 11, 2016, 8:13 p.m., Jake Maes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47197/
> ---
> 
> (Updated May 11, 2016, 8:13 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
> Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).
> 
> 
> Bugs: SAMZA-948
> https://issues.apache.org/jira/browse/SAMZA-948
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-948 CoordinatorSystemStreamConsumer is not threadsafe
> 
> See the stack traces in the JIRA for more context. Essentially the consumer 
> can bootstrap concurrently from multiple code paths (AM UI, RM Client 
> callbacks, etc) and with the remove() logic that was added in SAMZA-913, we 
> can get ConcurrentModificationExceptions. 
> 
> Fix:
> * Use an AtomicReference to swap in the updated messages when they are ready 
> * In bootstrap()
> * Acquire a lock
> * Make a copy of the messages
> * Append the new messages
> * Set the atomic reference to the copy
> * Release lock
> 
> Also sneaking in a log message fix for JobCoordinator. It previously didn't 
> include the task names.
> 
> 
> Diffs
> -
> 
>   
> samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java
>  8e1057b4d055159acb49d2cc60d3acad7665a532 
>   samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
> 03f48db7f42b2617995b14cf51248b82b6cc2636 
> 
> Diff: https://reviews.apache.org/r/47197/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Jake Maes
> 
>

Re: Review Request 47247: SAMZA-947 - TaskAssignmentManager registration exception when partition count changes.

2016-05-11 Thread Yi Pan (Data Infrastructure) 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47247/#review132749
---


Ship it!




lgtm. +1.


samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
 (line 42)


I guess that currently TaskAssignmentManager is still accessed via single 
thread? It is fine for now. But we should evaluate the need to make this thread 
safe when integrating with Xinyu's change.


- Yi Pan (Data Infrastructure)


On May 11, 2016, 8:26 p.m., Jake Maes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47247/
> ---
> 
> (Updated May 11, 2016, 8:26 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
> Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).
> 
> 
> Bugs: SAMZA-947
> https://issues.apache.org/jira/browse/SAMZA-947
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-947 - TaskAssignmentManager registration exception when partition count 
> changes.
> 
> * Register the producer once at constructor time
> * Don't bother registering the consumer. It's lifecycle is outside the 
> TaskAssignmentManager and registering from the TAM ends up being a no-op.
> 
> 
> Diffs
> -
> 
>   
> samza-core/src/main/java/org/apache/samza/container/grouper/task/GroupByContainerCount.java
>  286ea1b3d0d39ebd7d9923a81c02c1d0842b1291 
>   
> samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
>  0cbdec8ac050de18c2fea191e3ef38273f1dbab1 
>   
> samza-core/src/test/java/org/apache/samza/container/grouper/task/TestTaskAssignmentManager.java
>  19ab78e891ca22b6fba430ded6b9382c860a212d 
> 
> Diff: https://reviews.apache.org/r/47247/diff/
> 
> 
> Testing
> ---
> 
> Manually tested with a job. Adjusted the input topics to induce a partition 
> change and verified no exceptions and the TaskAssignmentManager cleanup ran 
> appropriately.
> 
> 2016-05-11 17:34:33 GroupByContainerCount [WARN] Current task count 32 does 
> not match saved task count 512. Stateful jobs may observe misalignment of 
> keys!
> ...
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 15" moved 
> from container 57 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 14" moved 
> from container 46 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 13" moved 
> from container 35 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 19" moved 
> from container 101 to container null
> ...
> 
> 
> Thanks,
> 
> Jake Maes
> 
>

Re: Review Request 47247: SAMZA-947 - TaskAssignmentManager registration exception when partition count changes.

2016-05-11 Thread Yi Pan (Data Infrastructure) 


> On May 11, 2016, 8:07 p.m., Yi Pan (Data Infrastructure) wrote:
> > It seems that all we needed to fix the issue is to remove the line to 
> > register the coordinator stream consumer? Did I miss any other things?
> 
> Jake Maes wrote:
> The key change is to remove the register() call from 
> GroupByContainerCount.save() and move it to the constructor. 
> 
> It was problematic because when the partition count would change, it 
> deletes the existing task mapping, which seems to register the producer, then 
> when it recalculates and saves the new mapping, it tries to register again 
> and this causes the registration exception. 
> 
> To me, it's just flimsy to pass (and REUSE!!!) 
> CoordinatorStreamSystemProducer and CoordinatorStreamSystemConsumer instances 
> into various implementations of AbstractCoordinatorStreamManager and expect 
> each of them to register the same producer and consumer, especially when each 
> implementation (after the first) that registers the consumer has no effect. 
> So, now the TaskAssignmentManager registers itself with the producer 
> immediately and only once. It doesn't bother registering for consumer, since 
> that's done first thing in the JobCoordinator.

Agreed. That remind me of some old memory on this issue. The better approach is 
to have a service maintaining the whole lifecycle of coordinator stream 
consumer and producer and simply provide the consumer/producer to the 
coordinator stream managers. Let's do the refactor later.


- Yi


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47247/#review132737
---


On May 11, 2016, 8:26 p.m., Jake Maes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47247/
> ---
> 
> (Updated May 11, 2016, 8:26 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
> Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).
> 
> 
> Bugs: SAMZA-947
> https://issues.apache.org/jira/browse/SAMZA-947
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-947 - TaskAssignmentManager registration exception when partition count 
> changes.
> 
> * Register the producer once at constructor time
> * Don't bother registering the consumer. It's lifecycle is outside the 
> TaskAssignmentManager and registering from the TAM ends up being a no-op.
> 
> 
> Diffs
> -
> 
>   
> samza-core/src/main/java/org/apache/samza/container/grouper/task/GroupByContainerCount.java
>  286ea1b3d0d39ebd7d9923a81c02c1d0842b1291 
>   
> samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
>  0cbdec8ac050de18c2fea191e3ef38273f1dbab1 
>   
> samza-core/src/test/java/org/apache/samza/container/grouper/task/TestTaskAssignmentManager.java
>  19ab78e891ca22b6fba430ded6b9382c860a212d 
> 
> Diff: https://reviews.apache.org/r/47247/diff/
> 
> 
> Testing
> ---
> 
> Manually tested with a job. Adjusted the input topics to induce a partition 
> change and verified no exceptions and the TaskAssignmentManager cleanup ran 
> appropriately.
> 
> 2016-05-11 17:34:33 GroupByContainerCount [WARN] Current task count 32 does 
> not match saved task count 512. Stateful jobs may observe misalignment of 
> keys!
> ...
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 15" moved 
> from container 57 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 14" moved 
> from container 46 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 13" moved 
> from container 35 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 19" moved 
> from container 101 to container null
> ...
> 
> 
> Thanks,
> 
> Jake Maes
> 
>

Re: Review Request 47247: SAMZA-947 - TaskAssignmentManager registration exception when partition count changes.

2016-05-11 Thread Jake Maes 


> On May 11, 2016, 8:07 p.m., Yi Pan (Data Infrastructure) wrote:
> > It seems that all we needed to fix the issue is to remove the line to 
> > register the coordinator stream consumer? Did I miss any other things?

The key change is to remove the register() call from 
GroupByContainerCount.save() and move it to the constructor. 

It was problematic because when the partition count would change, it deletes 
the existing task mapping, which seems to register the producer, then when it 
recalculates and saves the new mapping, it tries to register again and this 
causes the registration exception. 

To me, it's just flimsy to pass (and REUSE!!!) CoordinatorStreamSystemProducer 
and CoordinatorStreamSystemConsumer instances into various implementations of 
AbstractCoordinatorStreamManager and expect each of them to register the same 
producer and consumer, especially when each implementation (after the first) 
that registers the consumer has no effect. So, now the TaskAssignmentManager 
registers itself with the producer immediately and only once. It doesn't bother 
registering for consumer, since that's done first thing in the JobCoordinator.


> On May 11, 2016, 8:07 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java,
> >  line 53
> > 
> >
> > nit: Is this to avoid the call to register() with null input argument? 
> > It does not seem to be absolutely needed and makes this 
> > CoordinatorStreamManager's lifecycle different from others.

Explained above


- Jake


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47247/#review132737
---


On May 11, 2016, 8:26 p.m., Jake Maes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47247/
> ---
> 
> (Updated May 11, 2016, 8:26 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
> Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).
> 
> 
> Bugs: SAMZA-947
> https://issues.apache.org/jira/browse/SAMZA-947
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-947 - TaskAssignmentManager registration exception when partition count 
> changes.
> 
> * Register the producer once at constructor time
> * Don't bother registering the consumer. It's lifecycle is outside the 
> TaskAssignmentManager and registering from the TAM ends up being a no-op.
> 
> 
> Diffs
> -
> 
>   
> samza-core/src/main/java/org/apache/samza/container/grouper/task/GroupByContainerCount.java
>  286ea1b3d0d39ebd7d9923a81c02c1d0842b1291 
>   
> samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
>  0cbdec8ac050de18c2fea191e3ef38273f1dbab1 
>   
> samza-core/src/test/java/org/apache/samza/container/grouper/task/TestTaskAssignmentManager.java
>  19ab78e891ca22b6fba430ded6b9382c860a212d 
> 
> Diff: https://reviews.apache.org/r/47247/diff/
> 
> 
> Testing
> ---
> 
> Manually tested with a job. Adjusted the input topics to induce a partition 
> change and verified no exceptions and the TaskAssignmentManager cleanup ran 
> appropriately.
> 
> 2016-05-11 17:34:33 GroupByContainerCount [WARN] Current task count 32 does 
> not match saved task count 512. Stateful jobs may observe misalignment of 
> keys!
> ...
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 15" moved 
> from container 57 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 14" moved 
> from container 46 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 13" moved 
> from container 35 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 19" moved 
> from container 101 to container null
> ...
> 
> 
> Thanks,
> 
> Jake Maes
> 
>

Re: Review Request 47197: SAMZA-948 CoordinatorSystemStreamConsumer is not threadsafe

2016-05-11 Thread Yi Pan (Data Infrastructure) 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47197/#review132742
---


Ship it!




lgtm.


samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java
 (line 146)


nit: you can do a double check on isBootstraped here again to avoid doing 
another round of bootstrap in case two thread are contending on bootstrapLock().


- Yi Pan (Data Infrastructure)


On May 11, 2016, 8:13 p.m., Jake Maes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47197/
> ---
> 
> (Updated May 11, 2016, 8:13 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
> Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).
> 
> 
> Bugs: SAMZA-948
> https://issues.apache.org/jira/browse/SAMZA-948
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-948 CoordinatorSystemStreamConsumer is not threadsafe
> 
> See the stack traces in the JIRA for more context. Essentially the consumer 
> can bootstrap concurrently from multiple code paths (AM UI, RM Client 
> callbacks, etc) and with the remove() logic that was added in SAMZA-913, we 
> can get ConcurrentModificationExceptions. 
> 
> Fix:
> * Use an AtomicReference to swap in the updated messages when they are ready 
> * In bootstrap()
> * Acquire a lock
> * Make a copy of the messages
> * Append the new messages
> * Set the atomic reference to the copy
> * Release lock
> 
> Also sneaking in a log message fix for JobCoordinator. It previously didn't 
> include the task names.
> 
> 
> Diffs
> -
> 
>   
> samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java
>  8e1057b4d055159acb49d2cc60d3acad7665a532 
>   samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
> 03f48db7f42b2617995b14cf51248b82b6cc2636 
> 
> Diff: https://reviews.apache.org/r/47197/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Jake Maes
> 
>

Re: Review Request 47197: SAMZA-948 CoordinatorSystemStreamConsumer is not threadsafe

2016-05-11 Thread Jake Maes 


> On May 11, 2016, 6:28 p.m., Chris Pettitt wrote:
> >

Yes, and yes. Thanks!


- Jake


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47197/#review132710
---


On May 11, 2016, 8:13 p.m., Jake Maes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47197/
> ---
> 
> (Updated May 11, 2016, 8:13 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
> Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).
> 
> 
> Bugs: SAMZA-948
> https://issues.apache.org/jira/browse/SAMZA-948
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-948 CoordinatorSystemStreamConsumer is not threadsafe
> 
> See the stack traces in the JIRA for more context. Essentially the consumer 
> can bootstrap concurrently from multiple code paths (AM UI, RM Client 
> callbacks, etc) and with the remove() logic that was added in SAMZA-913, we 
> can get ConcurrentModificationExceptions. 
> 
> Fix:
> * Use an AtomicReference to swap in the updated messages when they are ready 
> * In bootstrap()
> * Acquire a lock
> * Make a copy of the messages
> * Append the new messages
> * Set the atomic reference to the copy
> * Release lock
> 
> Also sneaking in a log message fix for JobCoordinator. It previously didn't 
> include the task names.
> 
> 
> Diffs
> -
> 
>   
> samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java
>  8e1057b4d055159acb49d2cc60d3acad7665a532 
>   samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
> 03f48db7f42b2617995b14cf51248b82b6cc2636 
> 
> Diff: https://reviews.apache.org/r/47197/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Jake Maes
> 
>

Re: Review Request 47197: SAMZA-948 CoordinatorSystemStreamConsumer is not threadsafe

2016-05-11 Thread Jake Maes 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47197/
---

(Updated May 11, 2016, 8:13 p.m.)


Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).


Bugs: SAMZA-948
https://issues.apache.org/jira/browse/SAMZA-948


Repository: samza


Description
---

SAMZA-948 CoordinatorSystemStreamConsumer is not threadsafe

See the stack traces in the JIRA for more context. Essentially the consumer can 
bootstrap concurrently from multiple code paths (AM UI, RM Client callbacks, 
etc) and with the remove() logic that was added in SAMZA-913, we can get 
ConcurrentModificationExceptions. 

Fix:
* Use an AtomicReference to swap in the updated messages when they are ready 
* In bootstrap()
* Acquire a lock
* Make a copy of the messages
* Append the new messages
* Set the atomic reference to the copy
* Release lock

Also sneaking in a log message fix for JobCoordinator. It previously didn't 
include the task names.


Diffs (updated)
-

  
samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java
 8e1057b4d055159acb49d2cc60d3acad7665a532 
  samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
03f48db7f42b2617995b14cf51248b82b6cc2636 

Diff: https://reviews.apache.org/r/47197/diff/


Testing
---

./gradlew build


Thanks,

Jake Maes

Re: Review Request 47247: SAMZA-947 - TaskAssignmentManager registration exception when partition count changes.

2016-05-11 Thread Yi Pan (Data Infrastructure) 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47247/#review132737
---



It seems that all we needed to fix the issue is to remove the line to register 
the coordinator stream consumer? Did I miss any other things?


samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
 (line 53)


nit: Is this to avoid the call to register() with null input argument? It 
does not seem to be absolutely needed and makes this CoordinatorStreamManager's 
lifecycle different from others.



samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
 (line 58)


There is no place that we change the registered flag. Why do we need to 
have it if registered is always false?


- Yi Pan (Data Infrastructure)


On May 11, 2016, 6:19 p.m., Jake Maes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47247/
> ---
> 
> (Updated May 11, 2016, 6:19 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
> Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).
> 
> 
> Bugs: SAMZA-947
> https://issues.apache.org/jira/browse/SAMZA-947
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-947 - TaskAssignmentManager registration exception when partition count 
> changes.
> 
> * Register the producer once at constructor time
> * Don't bother registering the consumer. It's lifecycle is outside the 
> TaskAssignmentManager and registering from the TAM ends up being a no-op.
> 
> 
> Diffs
> -
> 
>   
> samza-core/src/main/java/org/apache/samza/container/grouper/task/GroupByContainerCount.java
>  286ea1b3d0d39ebd7d9923a81c02c1d0842b1291 
>   
> samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
>  0cbdec8ac050de18c2fea191e3ef38273f1dbab1 
>   
> samza-core/src/test/java/org/apache/samza/container/grouper/task/TestTaskAssignmentManager.java
>  19ab78e891ca22b6fba430ded6b9382c860a212d 
> 
> Diff: https://reviews.apache.org/r/47247/diff/
> 
> 
> Testing
> ---
> 
> Manually tested with a job. Adjusted the input topics to induce a partition 
> change and verified no exceptions and the TaskAssignmentManager cleanup ran 
> appropriately.
> 
> 2016-05-11 17:34:33 GroupByContainerCount [WARN] Current task count 32 does 
> not match saved task count 512. Stateful jobs may observe misalignment of 
> keys!
> ...
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 15" moved 
> from container 57 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 14" moved 
> from container 46 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 13" moved 
> from container 35 to container null
> 2016-05-11 17:34:34 TaskAssignmentManager [INFO] Task "Partition 19" moved 
> from container 101 to container null
> ...
> 
> 
> Thanks,
> 
> Jake Maes
> 
>

Re: Review Request 47251: SAMZA-852 - Better logging when system can not be created Always log the exception when we cant instantiate a producer or consumer

2016-05-11 Thread Jagadish Venkatraman 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47251/#review132730
---


Ship it!




Thanks for getting the fix in! lgtm

- Jagadish

- Jagadish Venkatraman


On May 11, 2016, 6:53 p.m., Jake Maes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47251/
> ---
> 
> (Updated May 11, 2016, 6:53 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
> Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).
> 
> 
> Bugs: SAMZA-852
> https://issues.apache.org/jira/browse/SAMZA-852
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-852 - Better logging when system can not be created Always log the 
> exception when we cant instantiate a producer or consumer
> 
> 
> Diffs
> -
> 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> 5462208c08cddbfd30d886daffa8c02c82447059 
> 
> Diff: https://reviews.apache.org/r/47251/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Jake Maes
> 
>

Review Request 47251: SAMZA-852 - Better logging when system can not be created Always log the exception when we cant instantiate a producer or consumer

2016-05-11 Thread Jake Maes 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47251/
---

Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).


Bugs: SAMZA-852
https://issues.apache.org/jira/browse/SAMZA-852


Repository: samza


Description
---

SAMZA-852 - Better logging when system can not be created Always log the 
exception when we cant instantiate a producer or consumer


Diffs
-

  samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
5462208c08cddbfd30d886daffa8c02c82447059 

Diff: https://reviews.apache.org/r/47251/diff/


Testing
---

./gradlew build


Thanks,

Jake Maes

Re: Review Request 47197: SAMZA-948 CoordinatorSystemStreamConsumer is not threadsafe

2016-05-11 Thread Chris Pettitt 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47197/#review132710
---


Fix it, then Ship it!





samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java
 (line 62)


If this is really code that can be run from multiple threads, as opposed to 
code that was blowing up due to ConcurrentModificationException (which is 
sometimes a misleading name), then this needs to be volatile.

isStarted might also need to be volatile, but I didn't look at how it was 
being used.



samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java
 (line 65)


You only need volatile here (vs. AtomicReference) since you're not using 
any CAS operation.

For full safety, you need to wrap the set in an unmodifiable wrapper. 
Otherwise it would be possible to modify the set via "read only" methods like 
getBootstrappedStream.


- Chris Pettitt


On May 10, 2016, 11:07 p.m., Jake Maes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47197/
> ---
> 
> (Updated May 10, 2016, 11:07 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina 
> Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).
> 
> 
> Bugs: SAMZA-948
> https://issues.apache.org/jira/browse/SAMZA-948
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-948 CoordinatorSystemStreamConsumer is not threadsafe
> 
> See the stack traces in the JIRA for more context. Essentially the consumer 
> can bootstrap concurrently from multiple code paths (AM UI, RM Client 
> callbacks, etc) and with the remove() logic that was added in SAMZA-913, we 
> can get ConcurrentModificationExceptions. 
> 
> Fix:
> * Use an AtomicReference to swap in the updated messages when they are ready 
> * In bootstrap()
> * Acquire a lock
> * Make a copy of the messages
> * Append the new messages
> * Set the atomic reference to the copy
> * Release lock
> 
> Also sneaking in a log message fix for JobCoordinator. It previously didn't 
> include the task names.
> 
> 
> Diffs
> -
> 
>   
> samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java
>  8e1057b4d055159acb49d2cc60d3acad7665a532 
>   samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
> 03f48db7f42b2617995b14cf51248b82b6cc2636 
> 
> Diff: https://reviews.apache.org/r/47197/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Jake Maes
> 
>

Re: Review Request 40934: SAMZA-827: null offsets when writing state store OFFSET file cause container stalls

2016-05-11 Thread Jagadish Venkatraman 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40934/
---

(Updated May 11, 2016, 6:25 p.m.)


Review request for samza.


Summary (updated)
-

SAMZA-827: null offsets when writing state store OFFSET file cause container 
stalls


Repository: samza


Description
---

Currently, we don't handle null offsets when writing state store OFFSET file. 

Impact:
1.Without the fix, writing a null offset to a file will fail Samza in its 
shutdown sequence when taskStorageManager.stop is invoked. 
2.An exception in the shut-down sequence will invoke the 
UncaughtExceptionHandler calling System.exit.
3.However, the Samzacontainer's JVM shutdown hook waits for task.shutdown.ms 
for all tasks to shutdown. 
4.It turns out that the shutdown hook, and the System.exit (called by *1*) lock 
the same object as a part of the JVM shutdown sequence. 
(causing an unnecessary stall of task.shutdown.ms even if there was no 
exception in user code. This stall is effectively downtime for critical jobs 
which need to be always up.)
5.Often people configure very large values of task.shutdown.ms aggravating the 
problem.


Diffs
-

  samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala 
9588dcde2e698942097eb8ae1e3afcb8192dcfb5 
  
samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala 
6491d095461b9181d5bf371ab12956086ff695d4 

Diff: https://reviews.apache.org/r/40934/diff/


Testing
---


Thanks,

Jagadish Venkatraman

Re: Review Request 40934: SAMZA-827: Handle null offsets when writing state store OFFSET file

2016-05-11 Thread Jagadish Venkatraman 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40934/
---

(Updated May 11, 2016, 6:21 p.m.)


Review request for samza.


Repository: samza


Description (updated)
---

Currently, we don't handle null offsets when writing state store OFFSET file. 

Impact:
1.Without the fix, writing a null offset to a file will fail Samza in its 
shutdown sequence when taskStorageManager.stop is invoked. 
2.An exception in the shut-down sequence will invoke the 
UncaughtExceptionHandler calling System.exit.
3.However, the Samzacontainer's JVM shutdown hook waits for task.shutdown.ms 
for all tasks to shutdown. 
4.It turns out that the shutdown hook, and the System.exit (called by *1*) lock 
the same object as a part of the JVM shutdown sequence. 
(causing an unnecessary stall of task.shutdown.ms even if there was no 
exception in user code. This stall is effectively downtime for critical jobs 
which need to be always up.)
5.Often people configure very large values of task.shutdown.ms aggravating the 
problem.


Diffs
-

  samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala 
9588dcde2e698942097eb8ae1e3afcb8192dcfb5 
  
samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala 
6491d095461b9181d5bf371ab12956086ff695d4 

Diff: https://reviews.apache.org/r/40934/diff/


Testing
---


Thanks,

Jagadish Venkatraman

Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

2016-05-11 Thread Jagadish Venkatraman 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/
---

(Updated May 11, 2016, 5:51 p.m.)


Review request for samza, Jake Maes, Navina Ramesh, and Yi Pan (Data 
Infrastructure).


Bugs: SAMZA-911
https://issues.apache.org/jira/browse/SAMZA-911


Repository: samza


Description (updated)
---

Currently, the KafkaSystemProducer's producer loop keeps retrying indefinitely 
when there is an exception in the retryBackOff loop. This is problematic 
because it will completely stall the Samza container (currently 
single-threaded). We've observed multiple jobs being affected as a result of 
this because of transient kafka-broker side errors.

If there are repeated exceptions, then it makes sense to retry for awhile, and 
then fail the container.

Long term fix: We should focus on getting rid off the retryBackOff loop, and 
close the producer object in the callback during failure. Closing the producer 
object in the callback-handler thread will guarantee in-order delivery. 
(SAMZA-934)

1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set to 
30).
2.Add tests to verify retry in case of RetriableExceptions.


Diffs
-

  
samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
 9a44d46d29a1997958a9d2bbf7be0bde860fff64 
  
samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
 39426d8cf64516ec4fdc0cb4ff60b1df3a757470 

Diff: https://reviews.apache.org/r/45258/diff/


Testing
---

Added unit tests to verify functionality.


Thanks,

Jagadish Venkatraman

Re: Review Request 46287: Add a double serde.

2016-05-11 Thread Yi Pan (Data Infrastructure) 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46287/#review132689
---



@Jon, forgot to mention: it would be good to add this new build-in serde class 
to the document. One example is in the configure table.

- Yi Pan (Data Infrastructure)


On April 15, 2016, 11:17 p.m., Jon Bringhurst wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46287/
> ---
> 
> (Updated April 15, 2016, 11:17 p.m.)
> 
> 
> Review request for samza.
> 
> 
> Bugs: SAMZA-936
> https://issues.apache.org/jira/browse/SAMZA-936
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> Add a simple double serde.
> 
> 
> Diffs
> -
> 
>   samza-core/src/main/scala/org/apache/samza/serializers/DoubleSerde.scala 
> PRE-CREATION 
>   
> samza-core/src/test/scala/org/apache/samza/serializers/TestDoubleSerde.scala 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46287/diff/
> 
> 
> Testing
> ---
> 
> A simple unit test was added.
> 
> 
> Thanks,
> 
> Jon Bringhurst
> 
>