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 Fri, 08 Apr 2016 21:39:05 GMT


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > 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!

Thanks for the feedback, I've fixed javadoc typos with the revised patch. I really appreciate
the detail in your reviews :)


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java,
line 81
> > <https://reviews.apache.org/r/44920/diff/4/?file=1328379#file1328379line81>
> >
> >     Since you have separate namespace for all the "cluster" and "request" management
classes, can you please rename this to "ResourceRequestState" instead of "ContainerRequestState"
?

Thanks for the feedback. Fixed!


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java, line
32
> > <https://reviews.apache.org/r/44920/diff/4/?file=1328393#file1328393line32>
> >
> >     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

All existing configs will continue to work as is. (with a recommended message to use the new
property).  Yes, documenting new configs is on my list, and will be a separate RB.


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > samza-core/src/main/java/org/apache/samza/config/ClusterManagerConfig.java, line
192
> > <https://reviews.apache.org/r/44920/diff/4/?file=1328393#file1328393line192>
> >
> >     nit: Identation is off from Line 192 to 200

Fixed! Thanks for the careful review :)


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala,
line 66
> > <https://reviews.apache.org/r/44920/diff/4/?file=1328395#file1328395line66>
> >
> >     I suspect this is going to be part of the AM UI and metrics refactoring? please
correct me if I am wrong.

Yup, Some of these fields may not make sense for metrics. As long as the UI reports it, we
should be good.


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnContainerRunner.java,
line 94
> > <https://reviews.apache.org/r/44920/diff/4/?file=1328411#file1328411line94>
> >
> >     nit: unused variable
> >     I suspect you have to rebase with Jake's changes.

Fixed. Relevant Jake's changes have been rebased. (including tests).


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > samza-yarn/src/main/java/org/apache/samza/job/yarn/refactor/YarnResourcemanagerFactory.java,
line 1
> > <https://reviews.apache.org/r/44920/diff/4/?file=1328412#file1328412line1>
> >
> >     File name has a typo - It should be YarnResourceManagerFactory.java

Fixed. nice find! :)


> On April 7, 2016, 11:13 p.m., Navina Ramesh wrote:
> > samza-yarn/src/main/scala/org/apache/samza/job/yarn/refactor/SamzaYarnAppMasterLifecycle.scala,
line 27
> > <https://reviews.apache.org/r/44920/diff/4/?file=1328414#file1328414line27>
> >
> >     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?

Currently the callbacks for onContainerAllocator, onContainerCompleted are handled by the
ContainerProcessManager. Also, classes for metrics and taskmanager were moved to samza-core.
(these used to implement the listener interface before). So, I did not feel particularly compelled
to retain the interface for init/shutdown for just 2 components (amservice and lifecycle).
I do not have a strong preference either way and I'm happy to add it back. We would just replace
invocations to 

service.onInit();
lifecycle.onInit();

with

instantiate a listener list 
add both service,lifecycle to the list
for (listener in listeners)
  listener.onInit();


- Jagadish


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44920/#review127658
-----------------------------------------------------------


On April 8, 2016, 3:30 a.m., Jagadish Venkatraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44920/
> -----------------------------------------------------------
> 
> (Updated April 8, 2016, 3:30 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
> 
> == Diff 4 ==
> Sync with current master
> 
> 
> 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-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala 110c3a910aa0bae77dfe5eebbf82286b56dc4654

>   samza-shell/src/main/bash/run-am.sh 9545a96953baaff17ad14962e02bc12aadbb1101 
>   samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java
b4789e62beb1120f11a8101664b10c34ae930e58 
>   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/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/util/MockContainerListener.java
cb82cccf75b54cfbefd586700e8283cb41173833 
>   samza-yarn/src/test/java/org/apache/samza/job/yarn/util/MockContainerRequestState.java
879a7d0d06b087cfe0417f3fa5801b43ac7fc458 
> 
> 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