samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jagadish Venkatraman <jagadish1...@gmail.com>
Subject Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers
Date Wed, 06 Apr 2016 17:47:09 GMT


> On April 5, 2016, 9:55 p.m., Navina Ramesh wrote:
> > samza-shell/src/main/bash/run-am.sh, line 28
> > <https://reviews.apache.org/r/44920/diff/3/?file=1325334#file1325334line28>
> >
> >     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
> > <https://reviews.apache.org/r/44920/diff/3/?file=1325321#file1325321line109>
> >
> >     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

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message