----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37817/#review100063 -----------------------------------------------------------
samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java (line 57) <https://reviews.apache.org/r/37817/#comment157163> nit: wouldn't it make more sense to set the init value to false? samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java (line 110) <https://reviews.apache.org/r/37817/#comment157167> I realized that isRunning is not really a state variable of the Runnable. It is more like a "stop" flag that is set by another thread (maybe the main/admin thread). It would read better if we rename it to isStopped. samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java (line 207) <https://reviews.apache.org/r/37817/#comment157173> nit: this seems should be logged at the same level as the non-host affinity case (where we are using log.info). samza-yarn/src/main/java/org/apache/samza/job/yarn/SamzaTaskManager.java (line 116) <https://reviews.apache.org/r/37817/#comment157233> nit: per the comment, should we remove? :) samza-yarn/src/main/java/org/apache/samza/job/yarn/SamzaTaskManager.java (line 150) <https://reviews.apache.org/r/37817/#comment157234> nit: It seems to me that it would make more sense to move it to line 209, instead of in the method's javadoc. samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterLifecycle.scala (line 29) <https://reviews.apache.org/r/37817/#comment157249> I think that this is also YARN 2.6+ only. So, it seems that we will need to drop the support for YARN 2.4 and 2.5 to get this patch in. I will ping Yan to close the open source mailing list vote on this. Overall look very good to me, except for a few nits. The only thing that I would recommend is to hold on this check-in until SAMZA-563 is settled s.t. we officially drop the support for YARN 2.4 and 2.5 and don't break ./bin/check-all.sh - Yi Pan (Data Infrastructure) On Sept. 20, 2015, 4:45 a.m., Navina Ramesh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37817/ > ----------------------------------------------------------- > > (Updated Sept. 20, 2015, 4:45 a.m.) > > > Review request for samza, Yan Fang, Chinmay Soman, Chris Riccomini, and Yi > Pan (Data Infrastructure). > > > Bugs: SAMZA-619 > https://issues.apache.org/jira/browse/SAMZA-619 > > > Repository: samza > > > Description > ------- > > This a major change to how we request and assign containers to resources. It > uses a threaded model for request and allocation. More comments in the > javadoc. > > Major changes to note: > 1. Changed yarn dependency to 2.6 > 2. Moved YarnConfig to java config class > 2. Removed SamzaAppMasterTaskManager > 3. SamzaAppState replaces SamzaAppMasterState > 4. SamzaTaskManager is very similar to SamzaAppMasterTaskManager, except that > it delegates onContainerAllocated and requestContainers to the thread in > ContainerAllocator > 5. Removed state.unclaimedContainers > 6. Allocator thread sleep time and request timeout is configurable > 7. host-affinity can be enabled by using the config > "yarn.samza.host-affinity.enabled". Host affinity is guaranteed to work with > FairScheduler that has continuous scheduling enabled. Details on this config > can be found at SAMZA-617 > [design](https://issues.apache.org/jira/secure/attachment/12726945/DESIGN-SAMZA-617-2.pdf). > 8. Added unit tests for TaskManager & ContainerAllocator > 9. Updated config documentation > 10. Removed TestSamzaAppMasterTaskManager.scala > > Pending items: > 1. Update web-site with info on this feature (SAMZA-668) > > > Diffs > ----- > > build.gradle 3a7fabcd4f4e5c8db950c3a33bba33618c5565b4 > docs/learn/documentation/versioned/jobs/configuration-table.html > c23d7d37f151a0dbdd71f52588773bc67edf88c8 > gradle/dependency-versions.gradle 36d564b6ca895f042ee4802643e49180f4947b62 > samza-core/src/main/java/org/apache/samza/container/LocalityManager.java > c567bf4a1747a7a6aff23be74cf25b1a44e57b42 > > samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetContainerHostMapping.java > 5455881e2a4a9c865137f2708f655641b93c2bfb > samza-core/src/main/java/org/apache/samza/job/model/JobModel.java > 7b592740c082225fc217a24188bc874f883db63b > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 61e228b1ff8619da4ad2e11b15a5afb7999c4996 > samza-core/src/main/scala/org/apache/samza/util/Util.scala > c8438bd6ed01a95860867505d84c04c493a5a197 > samza-yarn/src/main/java/org/apache/samza/config/YarnConfig.java > PRE-CREATION > > samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java > PRE-CREATION > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java > PRE-CREATION > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerFailure.java > PRE-CREATION > > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java > PRE-CREATION > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerUtil.java > PRE-CREATION > > samza-yarn/src/main/java/org/apache/samza/job/yarn/HostAwareContainerAllocator.java > PRE-CREATION > samza-yarn/src/main/java/org/apache/samza/job/yarn/SamzaAppState.java > PRE-CREATION > > samza-yarn/src/main/java/org/apache/samza/job/yarn/SamzaContainerRequest.java > PRE-CREATION > samza-yarn/src/main/java/org/apache/samza/job/yarn/SamzaTaskManager.java > PRE-CREATION > samza-yarn/src/main/resources/scalate/WEB-INF/views/index.scaml > ce2145adde09b0e8097a1c711bc296ee20392e63 > samza-yarn/src/main/scala/org/apache/samza/config/YarnConfig.scala > 1f51551893c42bb13d7941c8b0e5594ac7f42585 > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMaster.scala > af42c6a6636953a95f79837fe372e0dbd735df70 > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterLifecycle.scala > d475c4c566f17f88a04db5fbc84cc2e27eb333d2 > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala > 03acfe1bbbabf8f54be9f36fdae785476da45135 > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterService.scala > 060538623e4d67b986bc635518e7fe8ebdde9e24 > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterState.scala > f667c83a7fff43a5efdc64cde019b2cf35f38cb9 > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterTaskManager.scala > 1743c8611a94fa1c7f7dafd8ff8c713039f3df8c > samza-yarn/src/main/scala/org/apache/samza/job/yarn/YarnJob.scala > 8dd70c9977473e083e463d01b049c40e15b21f4a > > samza-yarn/src/main/scala/org/apache/samza/webapp/ApplicationMasterRestServlet.scala > 09f4dc32a4b18aeb3accb856c360ea2f95c82673 > > samza-yarn/src/main/scala/org/apache/samza/webapp/ApplicationMasterWebServlet.scala > 7fd51221149ba59e7ea1bc0b51d58726ec17a7d7 > > samza-yarn/src/test/java/org/apache/samza/job/yarn/TestContainerAllocator.java > PRE-CREATION > > samza-yarn/src/test/java/org/apache/samza/job/yarn/TestContainerRequestState.java > PRE-CREATION > > samza-yarn/src/test/java/org/apache/samza/job/yarn/TestHostAwareContainerAllocator.java > PRE-CREATION > > samza-yarn/src/test/java/org/apache/samza/job/yarn/TestSamzaContainerRequest.java > PRE-CREATION > > samza-yarn/src/test/java/org/apache/samza/job/yarn/TestSamzaTaskManager.java > PRE-CREATION > > samza-yarn/src/test/java/org/apache/samza/job/yarn/util/MockContainerAllocator.java > PRE-CREATION > > samza-yarn/src/test/java/org/apache/samza/job/yarn/util/MockContainerUtil.java > PRE-CREATION > samza-yarn/src/test/java/org/apache/samza/job/yarn/util/MockHttpServer.java > PRE-CREATION > samza-yarn/src/test/java/org/apache/samza/job/yarn/util/MockNMClient.java > PRE-CREATION > > samza-yarn/src/test/java/org/apache/samza/job/yarn/util/TestAMRMClientImpl.java > PRE-CREATION > samza-yarn/src/test/java/org/apache/samza/job/yarn/util/TestUtil.java > PRE-CREATION > > samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMaster.scala > 7b7d86a43c69e72c47eaa91f68be24e0f4022891 > > samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterLifecycle.scala > df5992e659302d2918c4e2c30b6122ed51ab9fe8 > > samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterService.scala > 6f4bfaf916d687426f746fd3b09cd56490bb500e > > samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala > 2eec65f02826de40493925c08ff344a8cc4feecb > > Diff: https://reviews.apache.org/r/37817/diff/ > > > Testing > ------- > > Tested with hello-samza locally. Tested with a sample job on a 3 node Yarn > cluster. Works as expected. > > > Thanks, > > Navina Ramesh > >
