----------------------------------------------------------- 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 < and '>' with > 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 & 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 > >
