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