----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43074/#review117555 -----------------------------------------------------------
samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java (line 94) <https://reviews.apache.org/r/43074/#comment178787> Great job on the refactoring. It's more readable when the overriden method is not run() but a more concrete method name - assignContainerRequests! Also, it's cleaner to handle exceptions in the parent. samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java (line 45) <https://reviews.apache.org/r/43074/#comment178786> I think the javadoc about the run() method may be moved to the parent class. The javadoc for this abstract method can simply talk about allocator implementation - that is either host aware or not. samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java (line 55) <https://reviews.apache.org/r/43074/#comment178788> nit: Did IDE mess up with some formatting here? - Jagadish Venkatraman On Feb. 1, 2016, 11:35 p.m., Jake Maes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43074/ > ----------------------------------------------------------- > > (Updated Feb. 1, 2016, 11:35 p.m.) > > > Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure). > > > Repository: samza > > > Description > ------- > > Bug: > ContainerAllocator and HostAwareContainerAllocator have different > try-catch logic in their run() methods. The HostAwareContainerAllocator > try-catch is outside the while loop, which would cause it to stop allocating > containers in the event of an exception. ContainerAllocator correctly has the > try-catch inside the loop > > Fix: > Refactor the loop and try-catch to a common run() method in > AbstractContainerAllocator > Change the log type to WARN for the general exception case > Refactor some duplicate code in ContainerRequestState > > > Diffs > ----- > > > samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java > 9ee2daccc3c44202308637207a084def81b49c09 > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java > 7c57a866114aabf76a36d3b7ca4c5810628e0c77 > > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java > ab3061eae2cfc2da6681ce2034492b165d0d8b96 > > samza-yarn/src/main/java/org/apache/samza/job/yarn/HostAwareContainerAllocator.java > ff22dbfe5537ba2a3c55c4e6063680ff4c9e55f4 > > Diff: https://reviews.apache.org/r/43074/diff/ > > > Testing > ------- > > Unit tests still pass. > > > Thanks, > > Jake Maes > >
