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