> On Sept. 11, 2015, 1:47 a.m., Yan Fang wrote: > > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java, > > line 80 > > <https://reviews.apache.org/r/37817/diff/5/?file=1065677#file1065677line80> > > > > this getId is for the global container Id, right? > > Navina Ramesh wrote: > What do you mean global container Id? This will be the containerId > assigned by Yarn
yes, when I say "global container Id", I mean the id assigned by Yarn. You fixed it by changing another one to samzaContainerId :) > On Sept. 11, 2015, 1:47 a.m., Yan Fang wrote: > > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java, > > lines 96-99 > > <https://reviews.apache.org/r/37817/diff/5/?file=1065679#file1065679line96> > > > > 1. format the comment a little. :) using > > /* > > * > > */ > > > > 2. this comment does not explain the code following it. Maybe this > > comment should be part of the allocatedContainers variable javadoc. > > Navina Ramesh wrote: > Fixed the formatting. > The comment was intented to explain why we update the allocatedContainers > when only a containerRequest is made. So, I think it is better to leave it > here. I have re-worded it to explain better. sure. Leaving it there sounds good. > On Sept. 11, 2015, 1:47 a.m., Yan Fang wrote: > > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java, > > line 113 > > <https://reviews.apache.org/r/37817/diff/5/?file=1065679#file1065679line113> > > > > just curious if it is possible that, the hostname of the machine is > > different from the before-":"-part of the HttpAddress? > > Navina Ramesh wrote: > what do you mean by different? the format of HttpAddress is > "$hostname:$port". We are only interested in the hostname here. ok. I see. > On Sept. 11, 2015, 1:47 a.m., Yan Fang wrote: > > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java, > > line 116 > > <https://reviews.apache.org/r/37817/diff/5/?file=1065679#file1065679line116> > > > > can line 116 to 163 be refactored a little? Now it looks very confusing > > to me. (Too many nested if) > > > > Is something like: > > > > if (requestCountOnThisHost > 0 && (allocatedContainersOnThisHost == > > null || allocatedContainersOnThisHost.size() < requestCountOnThisHost)) > > > > addToAllocatedContainerList(hostName, container); > > > > } > > else { > > addToAllocatedContainerList(ANY_HOST, container); > > } > > > > sufficient for the logic? > > Navina Ramesh wrote: > Yeah. It should be sufficient. This is how I had it before. It became > hard to debug because you didn't why a container was allocated to a buffer. > That's why I simplified the logic. I think for now it adds value to have > detailed logging until we can make sure that the feature is stable. Do you > think it is ok? yes, it sounds ok to me. > On Sept. 11, 2015, 1:47 a.m., Yan Fang wrote: > > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java, > > line 280 > > <https://reviews.apache.org/r/37817/diff/5/?file=1065679#file1065679line280> > > > > this seems not safe to me. > > Navina Ramesh wrote: > why? how do you suggest I can change it? how about Collections.unmodifiableMap() and Collections.unmodifiableList() ? > On Sept. 11, 2015, 1:47 a.m., Yan Fang wrote: > > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerUtil.java, > > lines 79-80 > > <https://reviews.apache.org/r/37817/diff/5/?file=1065680#file1065680line79> > > > > it is worth differenciating the containerId and container.getId > > Navina Ramesh wrote: > Ok. I will rename containerId to samzaContainerId.. Does that sound ok? yes, definitely. > On Sept. 11, 2015, 1:47 a.m., Yan Fang wrote: > > samza-yarn/src/main/java/org/apache/samza/job/yarn/SamzaTaskManager.java, > > lines 157-161 > > <https://reviews.apache.org/r/37817/diff/5/?file=1065684#file1065684line157> > > > > can this be simplified as > > if > > (state.runningContainers.containsValue(containerStatus.getContainerId()), > > then ... > > Navina Ramesh wrote: > Yeah. but we are interested in the key for that entry. If you use an > if-condition to check whether the value is present, how will you return the > key? You won't have any handle/reference to the map entry. Also, we are not > directly comparing the value. We are checking the value of a field in the > entry's value. Hence, if statement is insufficient. you are right. > On Sept. 11, 2015, 1:47 a.m., Yan Fang wrote: > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/YarnJob.scala, line 74 > > <https://reviews.apache.org/r/37817/diff/5/?file=1065693#file1065693line74> > > > > it should be amJavaHome.isEmpty() , because we set "" as the default > > value in YarnConfig. > > Navina Ramesh wrote: > We are setting default as "null" in YarnConfig in Line 149, aren't we ? oh, yes, you are right. I thought it was "AmOpt", which is "". :) - Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37817/#review98316 ----------------------------------------------------------- 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 > >
