samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Navina Ramesh <nram...@linkedin.com>
Subject Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers
Date Thu, 07 Apr 2016 23:13:35 GMT

-----------------------------------------------------------
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)
<https://reviews.apache.org/r/44920/#comment191059>

    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)
<https://reviews.apache.org/r/44920/#comment191075>

    nit: javadoc fix: replace '<' with &lt; and '>' with &gt;



samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java (line
112)
<https://reviews.apache.org/r/44920/#comment191076>

    nit: javadoc fix - No ending curly paratheses



samza-core/src/main/java/org/apache/samza/clustermanager/ContainerProcessManager.java (line
44)
<https://reviews.apache.org/r/44920/#comment191078>

    nit: javadoc fix - replace '&' with &amp;



samza-core/src/main/java/org/apache/samza/clustermanager/ContainerProcessManager.java (line
175)
<https://reviews.apache.org/r/44920/#comment191069>

    nit: Indentation is off from Line 175 to 380



samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java (line 32)
<https://reviews.apache.org/r/44920/#comment191046>

    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)
<https://reviews.apache.org/r/44920/#comment191045>

    nit: Identation is off from Line 192 to 200



samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala (line
66)
<https://reviews.apache.org/r/44920/#comment191071>

    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)
<https://reviews.apache.org/r/44920/#comment191072>

    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)
<https://reviews.apache.org/r/44920/#comment191043>

    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)
<https://reviews.apache.org/r/44920/#comment191051>

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