samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jake Maes <jacob.m...@gmail.com>
Subject Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers
Date Mon, 11 Apr 2016 17:08:34 GMT

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

    nit: remove blank line between doc and class



samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java (line
114)
<https://reviews.apache.org/r/44920/#comment191514>

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

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

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

    nit: Everything public should have javadoc



samza-core/src/main/java/org/apache/samza/clustermanager/ResourceRequestState.java (line 217)
<https://reviews.apache.org/r/44920/#comment191524>

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

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

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

>   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