subject:"Review Request 44920\: SAMZA\-680 Refactor the Samza AppMaster to support other cluster managers"

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-05-10 Thread Jagadish Venkatraman


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

(Updated May 10, 2016, 11:50 p.m.)


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


Repository: samza


Description
---

1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
   - Defined the ClusterResourceManager abstraction. This will abstract out 
Yarn and Mesos's interaction with Samza.
   - Defined the SamzaResource, SamzaResourceRequest. 
   - Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
2.Defined a ClusterManagerConfig to handle configurations independent of 
Yarn/Mesos.
3.Made Samza completely independent of Yarn. This cleanly separates Samza 
specific components and Yarn
specific components.
4.Readability improvements to the existing code base.
   -Added explicity documentation for every method, member and class (including 
on thread-safety)
   - Made internal variables final to document intent, visibility across 
threads. (trivially by adding modifiers, or by changing where they're 
initialized.)
5.Refactored JobCoordinator into a JobModelReader.

== Diff2 ==
Address Chriss review feedback.

Design Doc: 
https://issues.apache.org/jira/secure/attachment/12800342/Samza%20JobCoordinator%20Re-design%20Proposal_1.pdf

== Diff 3 ==
Address Yi's feedback

== Diff 4 ==
Sync with current master


Diffs (updated)
-

  checkstyle/import-control.xml fad7b55a540a2f9886a3bfdf7631d9661b602d29 
  
samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/HostAwareContainerAllocator.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceManagerFactory.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceRequestState.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaApplicationState.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaContainerLaunchException.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResource.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceRequest.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceStatus.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 
a3281c2c5f481a78cc4ae791c77d0e9805202477 
  
samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
 0cbdec8ac050de18c2fea191e3ef38273f1dbab1 
  samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 
0324e90a20c2fd31d1b7c6a0707aa3685a1cec20 
  samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala 
31b208f47b12a4e9ef1134b1c0bfe532f6c07a80 
  samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
03f48db7f42b2617995b14cf51248b82b6cc2636 
  samza-core/src/main/scala/org/apache/samza/job/local/ProcessJob.scala 
95d01dd15d369dcb2255154163bc2cd50d7b84e0 
  samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 
81ef59a8e2fd2745aa37a65074400b64c406bc28 
  samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala 
5acfe875d3a3d9842497e646f0f04ea2861ae950 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManagerCallback.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerListener.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerRequestState.java
 PRE-CREATION 
  samza-core/src/test/java/org/apache/samza/clustermanager/MockHttpServer.java 
PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/TestContainerAllocator.java
 PRE-CREATION

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-29 Thread Jagadish Venkatraman


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

(Updated April 29, 2016, 8:36 p.m.)


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


Repository: samza


Description
---

1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
   - Defined the ClusterResourceManager abstraction. This will abstract out 
Yarn and Mesos's interaction with Samza.
   - Defined the SamzaResource, SamzaResourceRequest. 
   - Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
2.Defined a ClusterManagerConfig to handle configurations independent of 
Yarn/Mesos.
3.Made Samza completely independent of Yarn. This cleanly separates Samza 
specific components and Yarn
specific components.
4.Readability improvements to the existing code base.
   -Added explicity documentation for every method, member and class (including 
on thread-safety)
   - Made internal variables final to document intent, visibility across 
threads. (trivially by adding modifiers, or by changing where they're 
initialized.)
5.Refactored JobCoordinator into a JobModelReader.

== Diff2 ==
Address Chriss review feedback.

Design Doc: 
https://issues.apache.org/jira/secure/attachment/12800342/Samza%20JobCoordinator%20Re-design%20Proposal_1.pdf

== Diff 3 ==
Address Yi's feedback

== Diff 4 ==
Sync with current master


Diffs (updated)
-

  checkstyle/import-control.xml c15b8e74de8e5aac5ac83278c52ab3dba1630e50 
  
samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/HostAwareContainerAllocator.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceManagerFactory.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceRequestState.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaApplicationState.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaContainerLaunchException.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResource.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceRequest.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceStatus.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 
a3281c2c5f481a78cc4ae791c77d0e9805202477 
  
samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
 ec5cf3da4d1967cf586cdf074262a1f42f1efb75 
  samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 
0324e90a20c2fd31d1b7c6a0707aa3685a1cec20 
  samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala 
31b208f47b12a4e9ef1134b1c0bfe532f6c07a80 
  samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
384b2e777c73fc1e4bc8a29312c9ea5372162ca1 
  samza-core/src/main/scala/org/apache/samza/job/local/ProcessJob.scala 
95d01dd15d369dcb2255154163bc2cd50d7b84e0 
  samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 
81ef59a8e2fd2745aa37a65074400b64c406bc28 
  samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala 
5acfe875d3a3d9842497e646f0f04ea2861ae950 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManagerCallback.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerListener.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerRequestState.java
 PRE-CREATION 
  samza-core/src/test/java/org/apache/samza/clustermanager/MockHttpServer.java 
PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/TestContainerAllocator.java
 PRE-CREATION

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-29 Thread Jagadish Venkatraman


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

(Updated April 29, 2016, 8:19 p.m.)


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


Changes
---

rebased with current master


Repository: samza


Description
---

1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
   - Defined the ClusterResourceManager abstraction. This will abstract out 
Yarn and Mesos's interaction with Samza.
   - Defined the SamzaResource, SamzaResourceRequest. 
   - Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
2.Defined a ClusterManagerConfig to handle configurations independent of 
Yarn/Mesos.
3.Made Samza completely independent of Yarn. This cleanly separates Samza 
specific components and Yarn
specific components.
4.Readability improvements to the existing code base.
   -Added explicity documentation for every method, member and class (including 
on thread-safety)
   - Made internal variables final to document intent, visibility across 
threads. (trivially by adding modifiers, or by changing where they're 
initialized.)
5.Refactored JobCoordinator into a JobModelReader.

== Diff2 ==
Address Chriss review feedback.

Design Doc: 
https://issues.apache.org/jira/secure/attachment/12800342/Samza%20JobCoordinator%20Re-design%20Proposal_1.pdf

== Diff 3 ==
Address Yi's feedback

== Diff 4 ==
Sync with current master


Diffs (updated)
-

  checkstyle/import-control.xml c15b8e74de8e5aac5ac83278c52ab3dba1630e50 
  
samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/HostAwareContainerAllocator.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceManagerFactory.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceRequestState.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaApplicationState.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaContainerLaunchException.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResource.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceRequest.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceStatus.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 
a3281c2c5f481a78cc4ae791c77d0e9805202477 
  
samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
 ec5cf3da4d1967cf586cdf074262a1f42f1efb75 
  samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 
0324e90a20c2fd31d1b7c6a0707aa3685a1cec20 
  samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala 
31b208f47b12a4e9ef1134b1c0bfe532f6c07a80 
  samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
384b2e777c73fc1e4bc8a29312c9ea5372162ca1 
  samza-core/src/main/scala/org/apache/samza/job/local/ProcessJob.scala 
95d01dd15d369dcb2255154163bc2cd50d7b84e0 
  samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 
81ef59a8e2fd2745aa37a65074400b64c406bc28 
  samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala 
5acfe875d3a3d9842497e646f0f04ea2861ae950 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManagerCallback.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerListener.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerRequestState.java
 PRE-CREATION 
  samza-core/src/test/java/org/apache/samza/clustermanager/MockHttpServer.java 
PRE-CREATION

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-28 Thread Yi Pan (Data Infrastructure)


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




samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 (line 38)


nit: I thought that the formatting of function signature we are following 
the ones in the previous code?


- Yi Pan (Data Infrastructure)


On April 27, 2016, 6:29 p.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44920/
> ---
> 
> (Updated April 27, 2016, 6:29 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Yi Pan 
> (Data Infrastructure), Navina Ramesh, and Xinyu Liu.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> 1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
>- Defined the ClusterResourceManager abstraction. This will abstract out 
> Yarn and Mesos's interaction with Samza.
>- Defined the SamzaResource, SamzaResourceRequest. 
>- Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
> 2.Defined a ClusterManagerConfig to handle configurations independent of 
> Yarn/Mesos.
> 3.Made Samza completely independent of Yarn. This cleanly separates Samza 
> specific components and Yarn
> specific components.
> 4.Readability improvements to the existing code base.
>-Added explicity documentation for every method, member and class 
> (including on thread-safety)
>- Made internal variables final to document intent, visibility across 
> threads. (trivially by adding modifiers, or by changing where they're 
> initialized.)
> 5.Refactored JobCoordinator into a JobModelReader.
> 
> == Diff2 ==
> Address Chriss review feedback.
> 
> Design Doc: 
> https://issues.apache.org/jira/secure/attachment/12800342/Samza%20JobCoordinator%20Re-design%20Proposal_1.pdf
> 
> == Diff 3 ==
> Address Yi's feedback
> 
> == Diff 4 ==
> Sync with current master
> 
> 
> Diffs
> -
> 
>   checkstyle/import-control.xml c15b8e74de8e5aac5ac83278c52ab3dba1630e50 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/HostAwareContainerAllocator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
> PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ResourceManagerFactory.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ResourceRequestState.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/SamzaApplicationState.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/SamzaContainerLaunchException.java
>  PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResource.java 
> PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceRequest.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceStatus.java
>  PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
> PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 
> a3281c2c5f481a78cc4ae791c77d0e9805202477 
>   
> samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
>  ec5cf3da4d1967cf586cdf074262a1f42f1efb75 
>   samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 
> 0324e90a20c2fd31d1b7c6a0707aa3685a1cec20 
>   samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala 
> 31b208f47b12a4e9ef1134b1c0bfe532f6c07a80 
>   samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
> 384b2e777c73fc1e4bc8a29312c9ea5372162ca1 
>   samza-core/src/main/scala/org/apache/samza/job/local/ProcessJob.scala 
> 66618165d27aa916238cc86b27631c5db3435c6a 
>   
> samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 
> 17c2e5be9ee216c88e3c07784c4f9c05cd92e9c9 
>   samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala 
> 5acfe875d3a3d9842497e646f0f04ea2861ae950 
>   
>

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-27 Thread Jagadish Venkatraman


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

(Updated April 27, 2016, 6:29 p.m.)


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


Changes
---

Adddress all of Yi's feedback.


Repository: samza


Description
---

1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
   - Defined the ClusterResourceManager abstraction. This will abstract out 
Yarn and Mesos's interaction with Samza.
   - Defined the SamzaResource, SamzaResourceRequest. 
   - Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
2.Defined a ClusterManagerConfig to handle configurations independent of 
Yarn/Mesos.
3.Made Samza completely independent of Yarn. This cleanly separates Samza 
specific components and Yarn
specific components.
4.Readability improvements to the existing code base.
   -Added explicity documentation for every method, member and class (including 
on thread-safety)
   - Made internal variables final to document intent, visibility across 
threads. (trivially by adding modifiers, or by changing where they're 
initialized.)
5.Refactored JobCoordinator into a JobModelReader.

== Diff2 ==
Address Chriss review feedback.

Design Doc: 
https://issues.apache.org/jira/secure/attachment/12800342/Samza%20JobCoordinator%20Re-design%20Proposal_1.pdf

== Diff 3 ==
Address Yi's feedback

== Diff 4 ==
Sync with current master


Diffs (updated)
-

  checkstyle/import-control.xml c15b8e74de8e5aac5ac83278c52ab3dba1630e50 
  
samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/HostAwareContainerAllocator.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceManagerFactory.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceRequestState.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaApplicationState.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaContainerLaunchException.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResource.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceRequest.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceStatus.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 
a3281c2c5f481a78cc4ae791c77d0e9805202477 
  
samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
 ec5cf3da4d1967cf586cdf074262a1f42f1efb75 
  samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 
0324e90a20c2fd31d1b7c6a0707aa3685a1cec20 
  samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala 
31b208f47b12a4e9ef1134b1c0bfe532f6c07a80 
  samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
384b2e777c73fc1e4bc8a29312c9ea5372162ca1 
  samza-core/src/main/scala/org/apache/samza/job/local/ProcessJob.scala 
66618165d27aa916238cc86b27631c5db3435c6a 
  samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 
17c2e5be9ee216c88e3c07784c4f9c05cd92e9c9 
  samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala 
5acfe875d3a3d9842497e646f0f04ea2861ae950 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManagerCallback.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerListener.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerRequestState.java
 PRE-CREATION 
  samza-core/src/test/java/org/apache/samza/clustermanager/MockHttpServer.java 
PRE-CREATION

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-26 Thread Jagadish Venkatraman



> On April 25, 2016, 7:03 a.m., Yi Pan (Data Infrastructure) wrote:
> > samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java,
> >  line 147
> > 
> >
> > nit: I think that I commented on this before (either on your or on 
> > Jake's previous RBs). Why do we name it as expectedContainerId? Is there 
> > any case that the launched containerId is not the expected one? If not, 
> > can't we simply name it as containerId?

Yeah, good point. I actually refactored the name in the SamzaResourceRequest 
class. But, this local variable was missed. I've fixed this one.


> On April 25, 2016, 7:03 a.m., Yi Pan (Data Infrastructure) wrote:
> > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java,
> >  line 85
> > 
> >
> > nit: *JobModelManager*

Fixed.


> On April 25, 2016, 7:03 a.m., Yi Pan (Data Infrastructure) wrote:
> > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java,
> >  line 189
> > 
> >
> > Maybe not now. But prefer to keep this outside the life cycle of 
> > JobCoordinator, since this is really applied to the life cycle of the whole 
> > JVM process.

I agree. This is currently a cyclic dependency. The config whether jmx is 
enabled or not is read from the coordinator stream, for which we need to 
instantiate the JC. (So, we would n't know if jmx is enabled or not before 
this). I agree, that this has to be fixed in the way we suggested. I'll fix 
this in another RB.


- Jagadish


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


On April 23, 2016, 12:21 a.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44920/
> ---
> 
> (Updated April 23, 2016, 12:21 a.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Yi Pan 
> (Data Infrastructure), Navina Ramesh, and Xinyu Liu.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> 1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
>- Defined the ClusterResourceManager abstraction. This will abstract out 
> Yarn and Mesos's interaction with Samza.
>- Defined the SamzaResource, SamzaResourceRequest. 
>- Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
> 2.Defined a ClusterManagerConfig to handle configurations independent of 
> Yarn/Mesos.
> 3.Made Samza completely independent of Yarn. This cleanly separates Samza 
> specific components and Yarn
> specific components.
> 4.Readability improvements to the existing code base.
>-Added explicity documentation for every method, member and class 
> (including on thread-safety)
>- Made internal variables final to document intent, visibility across 
> threads. (trivially by adding modifiers, or by changing where they're 
> initialized.)
> 5.Refactored JobCoordinator into a JobModelReader.
> 
> == Diff2 ==
> Address Chriss review feedback.
> 
> Design Doc: 
> https://issues.apache.org/jira/secure/attachment/12800342/Samza%20JobCoordinator%20Re-design%20Proposal_1.pdf
> 
> == Diff 3 ==
> Address Yi's feedback
> 
> == Diff 4 ==
> Sync with current master
> 
> 
> Diffs
> -
> 
>   checkstyle/import-control.xml c15b8e74de8e5aac5ac83278c52ab3dba1630e50 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/HostAwareContainerAllocator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
> PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ResourceManagerFactory.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ResourceRequestState.java
>  PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/clustermanager/SamzaAppState.java 
> PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/SamzaContainerLaunchException.java
>  PRE-CREATION 
>

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-25 Thread Yi Pan (Data Infrastructure)


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


Fix it, then Ship it!




Overall, ltgm. Only a few comments to be addressed. One high level comment is 
that we don't need a refactor in the package path to indicate that we are 
refactoring. :) Thanks!


samza-core/src/main/java/org/apache/samza/clustermanager/ContainerProcessManager.java
 (line 385)


It seems that we are only logging the exceptionOccurred here to set 
shouldShutdown() to true. We need to be able to tell whether the shutdown is 
normal shutdown or caused by exception. Shouldn't we also keep the throwable 
s.t. we know the exception caused shutdown here?



samza-core/src/main/java/org/apache/samza/clustermanager/ResourceRequestState.java
 (line 65)


nit: it seems that we can remove this and use synchronized keyword in the 
public methods that this lock object is used.



samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
(line 38)


let's remove the 'refactor'.



samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala 
(line 144)


Weird to see the val name is coordinator and the constructor is 
JobModelManager()



samza-shell/src/main/bash/run-am.sh (line 26)


nit: remove the empty line change.



samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java
 


nit: unnecessary change?



samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnAppState.java 
(line 1)


nit: I don't think that we need explicit 'refactor' in the package name.



samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnClusterResourceManager.java
 (line 1)


same here. No need for the extra level of "refactor".



samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnClusterResourceManager.java
 (line 372)


nit: it would be nice to add a note here that these unimplemented 
interfaces are methods that are specific to YARN AMRMClient.



samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnContainerRunner.java
 (line 93)


Why? I thought that it is useful, at least for logging? If we truly don't 
need it, let's remove it.

BTW, how does container launched on the remote NM know about the 
containerId if it is not passed via command line here?



samza-yarn/src/main/scala/org/apache/samza/job/yarn/refactor/SamzaAppMasterService.scala
 (line 1)


Maybe change it to YarnAppMasterWebService and remove the 'refactor' in the 
path?


- Yi Pan (Data Infrastructure)


On April 23, 2016, 12:21 a.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44920/
> ---
> 
> (Updated April 23, 2016, 12:21 a.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Yi Pan 
> (Data Infrastructure), Navina Ramesh, and Xinyu Liu.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> 1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
>- Defined the ClusterResourceManager abstraction. This will abstract out 
> Yarn and Mesos's interaction with Samza.
>- Defined the SamzaResource, SamzaResourceRequest. 
>- Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
> 2.Defined a ClusterManagerConfig to handle configurations independent of 
> Yarn/Mesos.
> 3.Made Samza completely independent of Yarn. This cleanly separates Samza 
> specific components and Yarn
> specific components.
> 4.Readability improvements to the existing code base.
>-Added explicity documentation for every method, member and class 
> (including on thread-safety)
>- Made internal variables final to document intent, visibility across 
> threads. (trivially by adding modifiers, or by changing where they're 
> initialized.)
> 5.Refactored JobCoordinator into a JobModelReader.
> 
> == Diff2 ==
> Address Chriss review feedback.
> 
> Design Doc: 
> https://issues.apache.org/jira/secure/attachment/12800342/Samza%20JobCoordinator%20Re-design%20Proposal_1.pdf
> 
> == Diff 3 ==
> Address Yi's feedback
> 
> == Diff 4 ==
> Sync with current master
> 
> 
> Diffs
> -
> 
>

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-22 Thread Jagadish Venkatraman


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

(Updated April 22, 2016, 7:58 p.m.)


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


Repository: samza


Description (updated)
---

1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
   - Defined the ClusterResourceManager abstraction. This will abstract out 
Yarn and Mesos's interaction with Samza.
   - Defined the SamzaResource, SamzaResourceRequest. 
   - Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
2.Defined a ClusterManagerConfig to handle configurations independent of 
Yarn/Mesos.
3.Made Samza completely independent of Yarn. This cleanly separates Samza 
specific components and Yarn
specific components.
4.Readability improvements to the existing code base.
   -Added explicity documentation for every method, member and class (including 
on thread-safety)
   - Made internal variables final to document intent, visibility across 
threads. (trivially by adding modifiers, or by changing where they're 
initialized.)
5.Refactored JobCoordinator into a JobModelReader.

== Diff2 ==
Address Chriss review feedback.

Design Doc: 
https://issues.apache.org/jira/secure/attachment/12790540/SamzaJobCoordinatorRe-designProposal.pdf

== Diff 3 ==
Address Yi's feedback

== Diff 4 ==
Sync with current master


Diffs
-

  checkstyle/import-control.xml c15b8e74de8e5aac5ac83278c52ab3dba1630e50 
  
samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/HostAwareContainerAllocator.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceManagerFactory.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceRequestState.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/SamzaAppState.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaContainerLaunchException.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResource.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceRequest.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceStatus.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 
a3281c2c5f481a78cc4ae791c77d0e9805202477 
  
samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
 ec5cf3da4d1967cf586cdf074262a1f42f1efb75 
  samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 
0324e90a20c2fd31d1b7c6a0707aa3685a1cec20 
  samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala 
31b208f47b12a4e9ef1134b1c0bfe532f6c07a80 
  samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
384b2e777c73fc1e4bc8a29312c9ea5372162ca1 
  samza-core/src/main/scala/org/apache/samza/job/local/ProcessJob.scala 
66618165d27aa916238cc86b27631c5db3435c6a 
  samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 
17c2e5be9ee216c88e3c07784c4f9c05cd92e9c9 
  samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala 
5acfe875d3a3d9842497e646f0f04ea2861ae950 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManagerCallback.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerListener.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerRequestState.java
 PRE-CREATION 
  samza-core/src/test/java/org/apache/samza/clustermanager/MockHttpServer.java 
PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/TestContainerAllocator.java
 PRE-CREATION

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-21 Thread Jagadish Venkatraman


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

(Updated April 21, 2016, 10:19 p.m.)


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


Repository: samza


Description
---

1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
   - Defined the ContainerProcessManager abstraction.
   - Defined the SamzaResource, SamzaResourceRequest. 
   - Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
2.Defined a ClusterManagerConfig to handle configurations independent of 
Yarn/Mesos.
3.Made Samza completely independent of Yarn. This cleanly separates Samza 
specific components and Yarn
specific components.
4.Readability improvements to the existing code base.
   -Added explicity documentation for every method, member and class (including 
on thread-safety)
   - Made internal variables final to document intent, visibility across 
threads. (trivially by adding modifiers, or by changing where they're 
initialized.)
5.Refactored JobCoordinator into a JobModelReader.

== Diff2 ==
Address Chriss review feedback.

Design Doc: 
https://issues.apache.org/jira/secure/attachment/12790540/SamzaJobCoordinatorRe-designProposal.pdf

== Diff 3 ==
Address Yi's feedback

== Diff 4 ==
Sync with current master


Diffs (updated)
-

  checkstyle/import-control.xml c15b8e74de8e5aac5ac83278c52ab3dba1630e50 
  
samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/HostAwareContainerAllocator.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceManagerFactory.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceRequestState.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/SamzaAppState.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaContainerLaunchException.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResource.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceRequest.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceStatus.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 
a3281c2c5f481a78cc4ae791c77d0e9805202477 
  
samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
 ec5cf3da4d1967cf586cdf074262a1f42f1efb75 
  samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 
0324e90a20c2fd31d1b7c6a0707aa3685a1cec20 
  samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala 
31b208f47b12a4e9ef1134b1c0bfe532f6c07a80 
  samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
384b2e777c73fc1e4bc8a29312c9ea5372162ca1 
  samza-core/src/main/scala/org/apache/samza/job/local/ProcessJob.scala 
66618165d27aa916238cc86b27631c5db3435c6a 
  samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 
17c2e5be9ee216c88e3c07784c4f9c05cd92e9c9 
  samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala 
5acfe875d3a3d9842497e646f0f04ea2861ae950 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManagerCallback.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerListener.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerRequestState.java
 PRE-CREATION 
  samza-core/src/test/java/org/apache/samza/clustermanager/MockHttpServer.java 
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

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-14 Thread Chris Pettitt



> On April 11, 2016, 5:08 p.m., Jake Maes wrote:
> > samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java,
> >  line 114
> > 
> >
> > Will this accomplish anything if we don't exit the loop?
> > 
> > If I'm reading this correctly, it will just set the flag, which will 
> > cause an InterruptedException in the next Thread.sleep, which will again 
> > get caught here. So it turns into a busy wait. 
> > 
> > I think rethrowing the InterruptedException or just returning are 
> > better options

I like the idea of returning (perhaps via isRunning = false, if its necessary). 
I would suggest keeping the interrupt here, though - it may not be required 
today but keeps the code more resilient to refactoring. InterruptedException is 
checked so you can't throw it from run. In any case it is a less direct way to 
manage the thread (it would kill the thread, but may involve uncaught exception 
handlers).


- Chris


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


On April 12, 2016, 11:45 p.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44920/
> ---
> 
> (Updated April 12, 2016, 11:45 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Yi Pan 
> (Data Infrastructure), Navina Ramesh, and Xinyu Liu.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> 1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
>- Defined the ContainerProcessManager abstraction.
>- Defined the SamzaResource, SamzaResourceRequest. 
>- Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
> 2.Defined a ClusterManagerConfig to handle configurations independent of 
> Yarn/Mesos.
> 3.Made Samza completely independent of Yarn. This cleanly separates Samza 
> specific components and Yarn
> specific components.
> 4.Readability improvements to the existing code base.
>-Added explicity documentation for every method, member and class 
> (including on thread-safety)
>- Made internal variables final to document intent, visibility across 
> threads. (trivially by adding modifiers, or by changing where they're 
> initialized.)
> 5.Refactored JobCoordinator into a JobModelReader.
> 
> == Diff2 ==
> Address Chriss review feedback.
> 
> Design Doc: 
> https://issues.apache.org/jira/secure/attachment/12790540/SamzaJobCoordinatorRe-designProposal.pdf
> 
> == Diff 3 ==
> Address Yi's feedback
> 
> == Diff 4 ==
> Sync with current master
> 
> 
> Diffs
> -
> 
>   checkstyle/import-control.xml c15b8e74de8e5aac5ac83278c52ab3dba1630e50 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/HostAwareContainerAllocator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
> PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ResourceManagerFactory.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ResourceRequestState.java
>  PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/clustermanager/SamzaAppState.java 
> PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/SamzaContainerLaunchException.java
>  PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResource.java 
> PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceRequest.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceStatus.java
>  PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
> PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 
> a3281c2c5f481a78cc4ae791c77d0e9805202477 
>   
> samza-core/src/main/java/org/apache/samza/container/grouper/task/TaskAssignmentManager.java
>  ec5cf3da4d1967cf586cdf074262a1f42f1efb75 
>   samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 
>

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-11 Thread Jake Maes


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


Fix it, then Ship it!




Looks like you've been very careful rebasing with all the recent commits. Nice 
work!

My last round of feedback below.


samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
 (line 43)


nit: remove blank line between doc and class



samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
 (line 114)


Will this accomplish anything if we don't exit the loop?

If I'm reading this correctly, it will just set the flag, which will cause 
an InterruptedException in the next Thread.sleep, which will again get caught 
here. So it turns into a busy wait. 

I think rethrowing the InterruptedException or just returning are better 
options



samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
 (line 163)


Why is this flag needed? Could we just break; if we catch an 
InterrruptedException?



samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
 (line 204)


Looks like this could be private. If kept public, it should at least 
include javadoc



samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
(lines 36 - 48)


nit: Everything public should have javadoc



samza-core/src/main/java/org/apache/samza/clustermanager/ResourceRequestState.java
 (line 217)


Whoever wrote this comment did not have foresight to make it generic to the 
inversion. Sorry. :-)

Possible reprhasing:
"because of a ConnectException while invoking the container on a remote 
host."



samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceRequest.java
 (lines 76 - 98)


nit: public methods should have doc. Many of these are pretty self 
explanatory, but a little doc can still help.



samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
(line 107)


Same nit: javadoc for public methods, just to help those that implement 
their own cluster manager adapters.


- Jake Maes


On April 8, 2016, 10:01 p.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44920/
> ---
> 
> (Updated April 8, 2016, 10:01 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Yi Pan 
> (Data Infrastructure), Navina Ramesh, and Xinyu Liu.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> 1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
>- Defined the ContainerProcessManager abstraction.
>- Defined the SamzaResource, SamzaResourceRequest. 
>- Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
> 2.Defined a ClusterManagerConfig to handle configurations independent of 
> Yarn/Mesos.
> 3.Made Samza completely independent of Yarn. This cleanly separates Samza 
> specific components and Yarn
> specific components.
> 4.Readability improvements to the existing code base.
>-Added explicity documentation for every method, member and class 
> (including on thread-safety)
>- Made internal variables final to document intent, visibility across 
> threads. (trivially by adding modifiers, or by changing where they're 
> initialized.)
> 5.Refactored JobCoordinator into a JobModelReader.
> 
> == Diff2 ==
> Address Chriss review feedback.
> 
> Design Doc: 
> https://issues.apache.org/jira/secure/attachment/12790540/SamzaJobCoordinatorRe-designProposal.pdf
> 
> == Diff 3 ==
> Address Yi's feedback
> 
> == Diff 4 ==
> Sync with current master
> 
> 
> Diffs
> -
> 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ContainerProcessManager.java
>  PRE-CREATION 
>   
>

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-08 Thread Jagadish Venkatraman


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

(Updated April 8, 2016, 10:01 p.m.)


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


Changes
---

Added a run-jc.sh script


Repository: samza


Description
---

1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
   - Defined the ContainerProcessManager abstraction.
   - Defined the SamzaResource, SamzaResourceRequest. 
   - Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
2.Defined a ClusterManagerConfig to handle configurations independent of 
Yarn/Mesos.
3.Made Samza completely independent of Yarn. This cleanly separates Samza 
specific components and Yarn
specific components.
4.Readability improvements to the existing code base.
   -Added explicity documentation for every method, member and class (including 
on thread-safety)
   - Made internal variables final to document intent, visibility across 
threads. (trivially by adding modifiers, or by changing where they're 
initialized.)
5.Refactored JobCoordinator into a JobModelReader.

== Diff2 ==
Address Chriss review feedback.

Design Doc: 
https://issues.apache.org/jira/secure/attachment/12790540/SamzaJobCoordinatorRe-designProposal.pdf

== Diff 3 ==
Address Yi's feedback

== Diff 4 ==
Sync with current master


Diffs (updated)
-

  
samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/HostAwareContainerAllocator.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceManagerFactory.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceRequestState.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/SamzaAppState.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaContainerLaunchException.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResource.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceRequest.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceStatus.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/coordinator/JobModelManager.scala 
PRE-CREATION 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManagerCallback.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerListener.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerRequestState.java
 PRE-CREATION 
  samza-core/src/test/java/org/apache/samza/clustermanager/MockHttpServer.java 
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/TestContainerRequestState.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/TestHostAwareContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala 
110c3a910aa0bae77dfe5eebbf82286b56dc4654 
  samza-shell/src/main/bash/run-am.sh 9545a96953baaff17ad14962e02bc12aadbb1101 
  samza-shell/src/main/bash/run-jc.sh PRE-CREATION 
  
samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java
 b4789e62beb1120f11a8101664b10c34ae930e58 
  samza-yarn/src/main/java/org/apache/samza/job/yarn/SamzaTaskManager.java 
caee6e6c182d3cf86bd4fe193f8b1797605b2480 
  samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnAppState.java 
PRE-CREATION

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-08 Thread Jagadish Venkatraman



> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > Overall, the class structure looks good for the first iteration. For 
> > duplicate classes in the new namespace, I didn't look into great detail as 
> > I expect that you will sync-up with the master before committing. 
> > Please fix javadoc warnings and errors. It gets noisy pretty quickly and 
> > with JDK8, it fails compilation. 
> > 
> > As discussed offline, I strongly urge you to do branch-based development. 
> > Otherwise, it becomes really difficult/messy to test for parity. Let me 
> > know if you need help with this. 
> > 
> > Thanks!

Thanks for the feedback, I've fixed javadoc typos with the revised patch. I 
really appreciate the detail in your reviews :)


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java,
> >  line 81
> > 
> >
> > Since you have separate namespace for all the "cluster" and "request" 
> > management classes, can you please rename this to "ResourceRequestState" 
> > instead of "ContainerRequestState" ?

Thanks for the feedback. Fixed!


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java, 
> > line 32
> > 
> >
> > I am sure you have this on your list. Document how the old configs map 
> > to the new ones and also, update the configuration.html page

All existing configs will continue to work as is. (with a recommended message 
to use the new property).  Yes, documenting new configs is on my list, and will 
be a separate RB.


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java, 
> > line 192
> > 
> >
> > nit: Identation is off from Line 192 to 200

Fixed! Thanks for the careful review :)


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala,
> >  line 66
> > 
> >
> > I suspect this is going to be part of the AM UI and metrics 
> > refactoring? please correct me if I am wrong.

Yup, Some of these fields may not make sense for metrics. As long as the UI 
reports it, we should be good.


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnContainerRunner.java,
> >  line 94
> > 
> >
> > nit: unused variable
> > I suspect you have to rebase with Jake's changes.

Fixed. Relevant Jake's changes have been rebased. (including tests).


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnResourcemanagerFactory.java,
> >  line 1
> > 
> >
> > File name has a typo - It should be YarnResourceManagerFactory.java

Fixed. nice find! :)


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > samza-yarn/src/main/scala/org/apache/samza/job/yarn/refactor/SamzaYarnAppMasterLifecycle.scala,
> >  line 27
> > 
> >
> > nit: Unused import
> > 
> > What was the reasoning to get rid of YarnAppMasterListener? 
> > It was tied to a "cluster based" approach. But I think it will be good 
> > to have a common interface for components that we expect to have a 
> > "lifecycle" - initialize and shutdown are the only 2 things I see as 
> > relevant. Afaik, onContainerCompleted and onContainerAllocated were more of 
> > an after-thought. 
> > Perhaps we can refactor this later. But did you have any strong 
> > reasoning for not using such an interface?

Currently the callbacks for onContainerAllocator, onContainerCompleted are 
handled by the ContainerProcessManager. Also, classes for metrics and 
taskmanager were moved to samza-core. (these used to implement the listener 
interface before). So, I did not feel particularly compelled to retain the 
interface for init/shutdown for just 2 components (amservice and lifecycle). I 
do not have a strong preference either way and I'm happy to add it back. We 
would just replace invocations to 

service.onInit();
lifecycle.onInit();

with

instantiate a listener list 
add both service,lifecycle to the list
for (listener in listeners)
  listener.onInit();


- Jagadish


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

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-07 Thread Jagadish Venkatraman



> On April 5, 2016, 9:55 p.m., Navina Ramesh wrote:
> > samza-core/src/main/java/org/apache/samza/clustermanager/ContainerProcessManager.java,
> >  line 109
> > 
> >
> > Why not move this class loader helper method into ClusterManagerConfig? 
> > It can return the ResourceManagerFactory class directly and the config API 
> > will be re-usable.
> 
> Jagadish Venkatraman wrote:
> I recommend that an implementation of Config should return/validate the 
> *value* of the property that the user configured. Instantiation of it using 
> ClassLoaders / deciding what to do with it may be done higher up the stack 
> (as it's done currently). This is done consistently in all other parts of the 
> Samza code base. Happy to chat in person if you think otherwise.
> 
> Navina Ramesh wrote:
> Yeah. I agree it is up to the higher up stack. My point was to make 
> "Class loading" as a utility function (at the minimum, if not a method in 
> ClusterManagerConfig) so that you don't have to do exception handling 
> everytime you load a class. We have this convenience class in scala. It will 
> be tremendously useful to do the same with Java utils as well. The upstream 
> callers should only be responsible for casting it to the correct type.

I agree it makes sense to have it as a public static method in a separate Utils 
class.

My preference is to *not* include a method to do Class loading as a part of a 
public API of a *Config* class (ClusterManagerConfig in this case). All public 
APIs exposed by a *Config* implementation IMHO may just validate/return config 
values. It's not clear to me how an API to instantiate a class (given its full 
canonical name) will be useful there. It will also have an unnecessary 
dependency on ClusterManagerConfig for someone doing casting :)


- Jagadish


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


On April 8, 2016, 3:30 a.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44920/
> ---
> 
> (Updated April 8, 2016, 3:30 a.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Yi Pan 
> (Data Infrastructure), Navina Ramesh, and Xinyu Liu.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> 1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
>- Defined the ContainerProcessManager abstraction.
>- Defined the SamzaResource, SamzaResourceRequest. 
>- Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
> 2.Defined a ClusterManagerConfig to handle configurations independent of 
> Yarn/Mesos.
> 3.Made Samza completely independent of Yarn. This cleanly separates Samza 
> specific components and Yarn
> specific components.
> 4.Readability improvements to the existing code base.
>-Added explicity documentation for every method, member and class 
> (including on thread-safety)
>- Made internal variables final to document intent, visibility across 
> threads. (trivially by adding modifiers, or by changing where they're 
> initialized.)
> 5.Refactored JobCoordinator into a JobModelReader.
> 
> == Diff2 ==
> Address Chriss review feedback.
> 
> Design Doc: 
> https://issues.apache.org/jira/secure/attachment/12790540/SamzaJobCoordinatorRe-designProposal.pdf
> 
> == Diff 3 ==
> Address Yi's feedback
> 
> == Diff 4 ==
> Sync with current master
> 
> 
> Diffs
> -
> 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/ContainerRequestState.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/HostAwareContainerAllocator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
> PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ResourceManagerFactory.java
>  PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/clustermanager/SamzaAppState.java 
> PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/SamzaContainerLaunchException.java
>  PRE-CREATION 
>

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-07 Thread Navina Ramesh

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

Overall, the class structure looks good for the first iteration. For duplicate 
classes in the new namespace, I didn't look into great detail as I expect that 
you will sync-up with the master before committing. 
Please fix javadoc warnings and errors. It gets noisy pretty quickly and with 
JDK8, it fails compilation. 

As discussed offline, I strongly urge you to do branch-based development. 
Otherwise, it becomes really difficult/messy to test for parity. Let me know if 
you need help with this. 

Thanks!

samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
 (line 81)

Since you have separate namespace for all the "cluster" and "request" 
management classes, can you please rename this to "ResourceRequestState" 
instead of "ContainerRequestState" ?

samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
 (line 172)

nit: javadoc fix: replace '<' with  and '>' with 

samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
 (line 112)

nit: javadoc fix - No ending curly paratheses

samza-core/src/main/java/org/apache/samza/clustermanager/ContainerProcessManager.java
 (line 44)

nit: javadoc fix - replace '&' with 

samza-core/src/main/java/org/apache/samza/clustermanager/ContainerProcessManager.java
 (line 175)

nit: Indentation is off from Line 175 to 380

samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
(line 32)

I am sure you have this on your list. Document how the old configs map to 
the new ones and also, update the configuration.html page

samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
(line 192)

nit: Identation is off from Line 192 to 200

samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 (line 66)

I suspect this is going to be part of the AM UI and metrics refactoring? 
please correct me if I am wrong.

samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnContainerRunner.java
 (line 94)

nit: unused variable
I suspect you have to rebase with Jake's changes.

samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnResourcemanagerFactory.java
 (line 1)

File name has a typo - It should be YarnResourceManagerFactory.java

samza-yarn/src/main/scala/org/apache/samza/job/yarn/refactor/SamzaYarnAppMasterLifecycle.scala
 (line 27)

nit: Unused import

What was the reasoning to get rid of YarnAppMasterListener? 
It was tied to a "cluster based" approach. But I think it will be good to 
have a common interface for components that we expect to have a "lifecycle" - 
initialize and shutdown are the only 2 things I see as relevant. Afaik, 
onContainerCompleted and onContainerAllocated were more of an after-thought. 
Perhaps we can refactor this later. But did you have any strong reasoning 
for not using such an interface?

- Navina Ramesh

On April 6, 2016, 5:31 p.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44920/
> ---
> 
> (Updated April 6, 2016, 5:31 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Yi Pan 
> (Data Infrastructure), Navina Ramesh, and Xinyu Liu.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> 1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
>- Defined the ContainerProcessManager abstraction.
>- Defined the SamzaResource, SamzaResourceRequest. 
>- Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
> 2.Defined a ClusterManagerConfig to handle configurations independent of 
> Yarn/Mesos.
> 3.Made Samza completely independent of Yarn. This cleanly separates Samza 
> specific components and Yarn
> specific components.
> 4.Readability improvements to the existing code base.
>-Added explicity documentation for every method, member and class 
> (including on thread-safety)
>- Made internal variables final to

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-06 Thread Jagadish Venkatraman



> On April 5, 2016, 9:55 p.m., Navina Ramesh wrote:
> > samza-shell/src/main/bash/run-am.sh, line 28
> > 
> >
> > It's kind of confusing when you set samza.container.name to be 
> > "samza-application-master" and then, run "ClusterBasedJobCoordinator" that 
> > is meant to be cluster-agnostic.
> > 
> > Can you please clarify why this has to remain samza-application-master?

Thanks for the feedback! Actually the run-am.sh script should just run the AM. 
I'll write a separate run-jc.sh script with the main class as ClusterBasedJC.


> On April 5, 2016, 9:55 p.m., Navina Ramesh wrote:
> > samza-core/src/main/java/org/apache/samza/clustermanager/ContainerProcessManager.java,
> >  line 109
> > 
> >
> > Why not move this class loader helper method into ClusterManagerConfig? 
> > It can return the ResourceManagerFactory class directly and the config API 
> > will be re-usable.

I recommend that an implementation of Config should return/validate the *value* 
of the property that the user configured. Instantiation of it using 
ClassLoaders / deciding what to do with it may be done higher up the stack (as 
it's done currently). This is done consistently in all other parts of the Samza 
code base. Happy to chat in person if you think otherwise.


- Jagadish


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


On April 6, 2016, 5:31 p.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44920/
> ---
> 
> (Updated April 6, 2016, 5:31 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Yi Pan 
> (Data Infrastructure), Navina Ramesh, and Xinyu Liu.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> 1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
>- Defined the ContainerProcessManager abstraction.
>- Defined the SamzaResource, SamzaResourceRequest. 
>- Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
> 2.Defined a ClusterManagerConfig to handle configurations independent of 
> Yarn/Mesos.
> 3.Made Samza completely independent of Yarn. This cleanly separates Samza 
> specific components and Yarn
> specific components.
> 4.Readability improvements to the existing code base.
>-Added explicity documentation for every method, member and class 
> (including on thread-safety)
>- Made internal variables final to document intent, visibility across 
> threads. (trivially by adding modifiers, or by changing where they're 
> initialized.)
> 5.Refactored JobCoordinator into a JobModelReader.
> 
> == Diff2 ==
> Address Chriss review feedback.
> 
> Design Doc: 
> https://issues.apache.org/jira/secure/attachment/12790540/SamzaJobCoordinatorRe-designProposal.pdf
> 
> == Diff 3 ==
> Address Yi's feedback
> 
> 
> Diffs
> -
> 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/ContainerRequestState.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/HostAwareContainerAllocator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
> PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ResourceManagerFactory.java
>  PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/clustermanager/SamzaAppState.java 
> PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/SamzaContainerLaunchException.java
>  PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResource.java 
> PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceRequest.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceStatus.java
>  PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
> PRE-CREATION 
>   
> samza-core/src/main/scala/org/apache/samza/coordinator/JobModelManager.scala 
> PRE-CREATION 
>   
>

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-06 Thread Jagadish Venkatraman


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

(Updated April 6, 2016, 5:31 p.m.)


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


Repository: samza


Description
---

1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
   - Defined the ContainerProcessManager abstraction.
   - Defined the SamzaResource, SamzaResourceRequest. 
   - Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
2.Defined a ClusterManagerConfig to handle configurations independent of 
Yarn/Mesos.
3.Made Samza completely independent of Yarn. This cleanly separates Samza 
specific components and Yarn
specific components.
4.Readability improvements to the existing code base.
   -Added explicity documentation for every method, member and class (including 
on thread-safety)
   - Made internal variables final to document intent, visibility across 
threads. (trivially by adding modifiers, or by changing where they're 
initialized.)
5.Refactored JobCoordinator into a JobModelReader.

== Diff2 ==
Address Chriss review feedback.

Design Doc: 
https://issues.apache.org/jira/secure/attachment/12790540/SamzaJobCoordinatorRe-designProposal.pdf

== Diff 3 ==
Address Yi's feedback


Diffs (updated)
-

  
samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/ContainerRequestState.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/HostAwareContainerAllocator.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceManagerFactory.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/SamzaAppState.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaContainerLaunchException.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResource.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceRequest.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceStatus.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/coordinator/JobModelManager.scala 
PRE-CREATION 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockClusterResourceManagerCallback.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerListener.java
 PRE-CREATION 
  
samza-core/src/test/java/org/apache/samza/clustermanager/MockContainerRequestState.java
 PRE-CREATION 
  samza-core/src/test/java/org/apache/samza/clustermanager/MockHttpServer.java 
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/TestContainerRequestState.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/HostAwareContainerAllocator.java
 979719687be864bf24354aea0a7dc51b5f11a712 
  samza-yarn/src/main/java/org/apache/samza/job/yarn/SamzaTaskManager.java 
caee6e6c182d3cf86bd4fe193f8b1797605b2480 
  samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnAppState.java 
PRE-CREATION 
  
samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnClusterResourceManager.java
 PRE-CREATION 
  
samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnContainerRunner.java
 PRE-CREATION 
  
samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnResourcemanagerFactory.java
 PRE-CREATION

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-05 Thread Navina Ramesh

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

samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
 (line 205)

nit: double semi-colon at the EOL

samza-core/src/main/java/org/apache/samza/clustermanager/ContainerProcessManager.java
 (line 102)

nit: Unused local variable

samza-core/src/main/java/org/apache/samza/clustermanager/ContainerProcessManager.java
 (line 109)

Why not move this class loader helper method into ClusterManagerConfig? It 
can return the ResourceManagerFactory class directly and the config API will be 
re-usable.

samza-core/src/main/scala/org/apache/samza/coordinator/JobModelManager.scala 
(line 55)

What was the reasoning behind keeping the "JobCooridnator" class around and 
adding a dupe of the class as JobModelManager? You may have explained it to me 
previously. Can you please refresh my memory?

I see 2 drawbacks with this:
* Your compilation will not catch errors, if components outside of 
samza-core or samza-yarn are referencing JobCoordinator. Compilation will 
always succeed. That doesn't guarantee correct behavior of Samza. For example, 
StreamAppender in samza-log4j tries to access 
"JobCoordinator.currentJobCoordinator()". With you patch, this won't work right?
* You have to continue catching-up with upstream on both copies of this 
file. It's tedious and more likely to miss bug-fixes or even features. For 
example, JobModelManager doesn't have the StreamPartitionCountMonitor, which I 
believe got committed after your made this copy.

Since we don't have sufficient test coverage in samza, it makes me really 
nervous to make such dual copies. We can talk offline if needed. Please correct 
me if I have misunderstood something. Thanks!

samza-shell/src/main/bash/run-am.sh (line 28)

It's kind of confusing when you set samza.container.name to be 
"samza-application-master" and then, run "ClusterBasedJobCoordinator" that is 
meant to be cluster-agnostic.

Can you please clarify why this has to remain samza-application-master?

- Navina Ramesh

On April 5, 2016, 12:35 a.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44920/
> ---
> 
> (Updated April 5, 2016, 12:35 a.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Yi Pan 
> (Data Infrastructure), Navina Ramesh, and Xinyu Liu.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> 1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
>- Defined the ContainerProcessManager abstraction.
>- Defined the SamzaResource, SamzaResourceRequest. 
>- Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
> 2.Defined a ClusterManagerConfig to handle configurations independent of 
> Yarn/Mesos.
> 3.Made Samza completely independent of Yarn. This cleanly separates Samza 
> specific components and Yarn
> specific components.
> 4.Readability improvements to the existing code base.
>-Added explicity documentation for every method, member and class 
> (including on thread-safety)
>- Made internal variables final to document intent, visibility across 
> threads. (trivially by adding modifiers, or by changing where they're 
> initialized.)
> 5.Refactored JobCoordinator into a JobModelReader.
> 
> == Diff2 ==
> Address Chriss review feedback.
> 
> Design Doc: 
> https://issues.apache.org/jira/secure/attachment/12790540/SamzaJobCoordinatorRe-designProposal.pdf
> 
> == Diff 3 ==
> Address Yi's feedback
> 
> 
> Diffs
> -
> 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/ContainerRequestState.java
>  PRE-CREATION 
>   
> samza-core/src/main/java/org/apache/samza/clustermanager/HostAwareContainerAllocator.java
>  PRE-CREATION 
>   
>

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-04 Thread Jagadish Venkatraman



> On March 31, 2016, 10:43 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java,
> >  line 61
> > 
> >
> > I think that this is the implementation of ClusterJobCoordinatorBase 
> > class in the design doc? Why there is no implementation of JobCoordinator 
> > interface as in the design doc?

Clarified offline. There was some misconception about the scope of the RB. This 
is just to do the inversion. I've renamed the classes for better clarity.


> On March 31, 2016, 10:43 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java,
> >  line 98
> > 
> >
> > This also seems to be a feature of taskManager, can we fold it in that 
> > object?

This is the interval at which the JC polls the taskManager for shutdown. I've 
renamed classes to better reflect the roles:
1. ContainerProcessManager - This concrete class now handles and registers 
itself for callbacks from Yarn.
2. ClusterResourceManager - This is an abstract class which Yarn/Mesos 
implementations will extend.
3. Renamed the property as jobCoordinatorSleepMs, so that it's more obvious.


> On March 31, 2016, 10:43 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java,
> >  line 131
> > 
> >
> > Question: why isn't this JmxServer object's lifecycle == 
> > StreamProcessor? Or even, the whole user-defined process?
> 
> Jagadish Venkatraman wrote:
> Discussed offline. We agreed that JmxServer could be passsed externally. 
> It's still useful to distinguish isntantiation, and startup of the JmxServer 
> in the JmxServer class.

Discussed offline. Currently, the JmxServer object is instantiated based on a 
config from coordinator stream (yarn.am.jmx.enabled). Ideally, this config 
should be a part of the Jvm command line config. We should not need to load the 
config from the coordinator stream to find out if we should instantiate a jmx 
server or not.


> On March 31, 2016, 10:43 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java,
> >  line 158
> > 
> >
> > Not sure why we need a separate SamzaTaskManager??

There was some misconception about names :-) I've renamed SamzaTaskManager to 
ContainerProcessManager.


> On March 31, 2016, 10:43 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java,
> >  line 173
> > 
> >
> > I think that this really should be at least in StreamProcessor.

The reason this is in the JobCoordinator is because it's instantiated based on 
a config (which we read from the JobModelManager.) For now, we can keep this as 
is, and I'll create a separate Jira for that change.


> On March 31, 2016, 10:43 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java,
> >  line 88
> > 
> >
> > From the comments, it seems that this is mainly handle the "container 
> > failures/allocation", not task. Why can't this be part of 
> > ContainerProcessManager?

I've renamed SamzaTaskManager to be the ContainerProcessManager.


- Jagadish


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


On April 5, 2016, 12:35 a.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44920/
> ---
> 
> (Updated April 5, 2016, 12:35 a.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Yi Pan 
> (Data Infrastructure), Navina Ramesh, and Xinyu Liu.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> 1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
>- Defined the ContainerProcessManager abstraction.
>- Defined the SamzaResource, SamzaResourceRequest. 
>- Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
> 2.Defined a ClusterManagerConfig to handle configurations independent of 
> Yarn/Mesos.
> 3.Made Samza completely independent of Yarn. This cleanly separates Samza 
> specific

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

2016-04-04 Thread Jagadish Venkatraman


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

(Updated April 5, 2016, 12:35 a.m.)


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


Summary (updated)
-

SAMZA-680 Refactor the Samza AppMaster to support other cluster managers


Repository: samza


Description (updated)
---

1.Proposed new APIs for running Samza without Yarn. (SAMZA-881)
   - Defined the ContainerProcessManager abstraction.
   - Defined the SamzaResource, SamzaResourceRequest. 
   - Re-wrote the SamzaAppMaster logic into a ClusterBasedJobCoordinator.
2.Defined a ClusterManagerConfig to handle configurations independent of 
Yarn/Mesos.
3.Made Samza completely independent of Yarn. This cleanly separates Samza 
specific components and Yarn
specific components.
4.Readability improvements to the existing code base.
   -Added explicity documentation for every method, member and class (including 
on thread-safety)
   - Made internal variables final to document intent, visibility across 
threads. (trivially by adding modifiers, or by changing where they're 
initialized.)
5.Refactored JobCoordinator into a JobModelReader.

== Diff2 ==
Address Chriss review feedback.

Design Doc: 
https://issues.apache.org/jira/secure/attachment/12790540/SamzaJobCoordinatorRe-designProposal.pdf

== Diff 3 ==
Address Yi's feedback


Diffs (updated)
-

  
samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterResourceManager.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.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/ContainerRequestState.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/HostAwareContainerAllocator.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/ResourceFailure.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/ResourceManagerFactory.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/SamzaAppState.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaContainerLaunchException.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResource.java 
PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceRequest.java
 PRE-CREATION 
  
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaResourceStatus.java
 PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/coordinator/JobModelManager.scala 
PRE-CREATION 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 PRE-CREATION 
  samza-shell/src/main/bash/run-am.sh 9545a96953baaff17ad14962e02bc12aadbb1101 
  
samza-yarn/src/main/java/org/apache/samza/job/yarn/HostAwareContainerAllocator.java
 979719687be864bf24354aea0a7dc51b5f11a712 
  samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnAppState.java 
PRE-CREATION 
  
samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnClusterResourceManager.java
 PRE-CREATION 
  
samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnContainerRunner.java
 PRE-CREATION 
  
samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnResourcemanagerFactory.java
 PRE-CREATION 
  
samza-yarn/src/main/scala/org/apache/samza/job/yarn/refactor/SamzaAppMasterService.scala
 PRE-CREATION 
  
samza-yarn/src/main/scala/org/apache/samza/job/yarn/refactor/SamzaYarnAppMasterLifecycle.scala
 PRE-CREATION 
  
samza-yarn/src/test/java/org/apache/samza/job/yarn/TestHostAwareContainerAllocator.java
 0c7a09f3e4c4c2ce6788be729d0bf4a294243c68 

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


Testing
---

Tested with sample jobs on clusters of varying sizes. Tested locally. TODO: 
Unit tests.


Thanks,

Jagadish Venkatraman

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers

20 matches

Site Navigation

Mail list logo

Footer information