> On March 31, 2016, 10:43 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java, > > line 61 > > <https://reviews.apache.org/r/44920/diff/2/?file=1314315#file1314315line61> > > > > I think that this is the implementation of ClusterJobCoordinatorBase > > class in the design doc? Why there is no implementation of JobCoordinator > > interface as in the design doc?
Clarified offline. There was some misconception about the scope of the RB. This is just to do the inversion. I've renamed the classes for better clarity. > On March 31, 2016, 10:43 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java, > > line 98 > > <https://reviews.apache.org/r/44920/diff/2/?file=1314315#file1314315line98> > > > > This also seems to be a feature of taskManager, can we fold it in that > > object? This is the interval at which the JC polls the taskManager for shutdown. I've renamed classes to better reflect the roles: 1. ContainerProcessManager - This concrete class now handles and registers itself for callbacks from Yarn. 2. ClusterResourceManager - This is an abstract class which Yarn/Mesos implementations will extend. 3. Renamed the property as jobCoordinatorSleepMs, so that it's more obvious. > On March 31, 2016, 10:43 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java, > > line 131 > > <https://reviews.apache.org/r/44920/diff/2/?file=1314315#file1314315line131> > > > > Question: why isn't this JmxServer object's lifecycle == > > StreamProcessor? Or even, the whole user-defined process? > > Jagadish Venkatraman wrote: > Discussed offline. We agreed that JmxServer could be passsed externally. > It's still useful to distinguish isntantiation, and startup of the JmxServer > in the JmxServer class. Discussed offline. Currently, the JmxServer object is instantiated based on a config from coordinator stream (yarn.am.jmx.enabled). Ideally, this config should be a part of the Jvm command line config. We should not need to load the config from the coordinator stream to find out if we should instantiate a jmx server or not. > On March 31, 2016, 10:43 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java, > > line 158 > > <https://reviews.apache.org/r/44920/diff/2/?file=1314315#file1314315line158> > > > > Not sure why we need a separate SamzaTaskManager?? There was some misconception about names :-) I've renamed SamzaTaskManager to ContainerProcessManager. > On March 31, 2016, 10:43 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java, > > line 173 > > <https://reviews.apache.org/r/44920/diff/2/?file=1314315#file1314315line173> > > > > I think that this really should be at least in StreamProcessor. The reason this is in the JobCoordinator is because it's instantiated based on a config (which we read from the JobModelManager.) For now, we can keep this as is, and I'll create a separate Jira for that change. > On March 31, 2016, 10:43 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java, > > line 88 > > <https://reviews.apache.org/r/44920/diff/2/?file=1314315#file1314315line88> > > > > From the comments, it seems that this is mainly handle the "container > > failures/allocation", not task. Why can't this be part of > > ContainerProcessManager? I've renamed SamzaTaskManager to be the ContainerProcessManager. - Jagadish ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44920/#review126385 ----------------------------------------------------------- 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 > >
