samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jagadish Venkatraman <jagadish1...@gmail.com>
Subject Re: Review Request 44920: SAMZA-680 Refactor the Samza AppMaster to support other cluster managers
Date Tue, 05 Apr 2016 00:58:45 GMT


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


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