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 Fri, 08 Apr 2016 04:14:48 GMT


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

>   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/TestContainerAllocator.java b253f98f7258bb611e1ad6672f74b07ab7e20b70

>   samza-yarn/src/test/java/org/apache/samza/job/yarn/TestHostAwareContainerAllocator.java
93e430b6ee986b06ecdac4979552d774724a1fbd 
>   samza-yarn/src/test/java/org/apache/samza/job/yarn/util/MockContainerListener.java
cb82cccf75b54cfbefd586700e8283cb41173833 
>   samza-yarn/src/test/java/org/apache/samza/job/yarn/util/MockContainerRequestState.java
879a7d0d06b087cfe0417f3fa5801b43ac7fc458 
> 
> 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