samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yi Pan \(Data Infrastructure\)" <yi...@linkedin.com>
Subject Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers
Date Tue, 26 Apr 2016 00:23:27 GMT

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

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

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

    let's remove the 'refactor'.



samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala (line 144)
<https://reviews.apache.org/r/44920/#comment194275>

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



samza-shell/src/main/bash/run-am.sh (line 26)
<https://reviews.apache.org/r/44920/#comment194286>

    nit: remove the empty line change.



samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java 
<https://reviews.apache.org/r/44920/#comment194287>

    nit: unnecessary change?



samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnAppState.java (line 1)
<https://reviews.apache.org/r/44920/#comment194288>

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

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

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

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

    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
> -----
> 
>   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
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/container/TestSamzaContainer.scala 9df12d2974c686c07457b29d873fd3e9dab72e21

>   samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala 110c3a910aa0bae77dfe5eebbf82286b56dc4654

>   samza-core/src/test/scala/org/apache/samza/job/local/TestProcessJob.scala 3a710a82f6104dcbdcfcb9f166fe05003074c4b0

>   samza-log4j/src/main/java/org/apache/samza/logging/log4j/StreamAppender.java 0c6329ede9b3df4dc05125729b5b44ba2c98803a

>   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/SamzaAppState.java 77280bab8aeb242b34b5b780c84e6deab1a45f51

>   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/java/org/apache/samza/validation/YarnJobValidationTool.java 70f1e4f7663560e61b7aaa757946adbe03d2bd76

>   samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMaster.scala 80deb3b18c094d83af67535f9d0156f18ae3f5e4

>   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/TestSamzaTaskManager.java faa697db49ec1c11d76c88d919a356a5ae409a15

>   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 
>   samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterLifecycle.scala
30cf34fe1fd3f74537d16e8a51b467cd50835357 
>   samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterService.scala
7f5d9f4af088589d4287c26737bae25567c157d7 
> 
> Diff: https://reviews.apache.org/r/44920/diff/
> 
> 
> Testing
> -------
> 
> Tested with sample jobs on clusters of varying sizes / loads for host affinity. Also
tested locally on a local yarn cluster.
> 
> Added Unit tests.
> 
> 
> Thanks,
> 
> Jagadish Venkatraman
> 
>


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