----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44920/#review127192 -----------------------------------------------------------
samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java (line 205) <https://reviews.apache.org/r/44920/#comment190368> nit: double semi-colon at the EOL samza-core/src/main/java/org/apache/samza/clustermanager/ContainerProcessManager.java (line 102) <https://reviews.apache.org/r/44920/#comment190383> nit: Unused local variable samza-core/src/main/java/org/apache/samza/clustermanager/ContainerProcessManager.java (line 109) <https://reviews.apache.org/r/44920/#comment190385> Why not move this class loader helper method into ClusterManagerConfig? It can return the ResourceManagerFactory class directly and the config API will be re-usable. samza-core/src/main/scala/org/apache/samza/coordinator/JobModelManager.scala (line 55) <https://reviews.apache.org/r/44920/#comment190364> What was the reasoning behind keeping the "JobCooridnator" class around and adding a dupe of the class as JobModelManager? You may have explained it to me previously. Can you please refresh my memory? I see 2 drawbacks with this: * Your compilation will not catch errors, if components outside of samza-core or samza-yarn are referencing JobCoordinator. Compilation will always succeed. That doesn't guarantee correct behavior of Samza. For example, StreamAppender in samza-log4j tries to access "JobCoordinator.currentJobCoordinator()". With you patch, this won't work right? * You have to continue catching-up with upstream on both copies of this file. It's tedious and more likely to miss bug-fixes or even features. For example, JobModelManager doesn't have the StreamPartitionCountMonitor, which I believe got committed after your made this copy. Since we don't have sufficient test coverage in samza, it makes me really nervous to make such dual copies. We can talk offline if needed. Please correct me if I have misunderstood something. Thanks! samza-shell/src/main/bash/run-am.sh (line 28) <https://reviews.apache.org/r/44920/#comment190361> It's kind of confusing when you set samza.container.name to be "samza-application-master" and then, run "ClusterBasedJobCoordinator" that is meant to be cluster-agnostic. Can you please clarify why this has to remain samza-application-master? - Navina Ramesh On April 5, 2016, 12:35 a.m., Jagadish Venkatraman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44920/ > ----------------------------------------------------------- > > (Updated April 5, 2016, 12:35 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 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-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/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 > >
