----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43074/#review118073 -----------------------------------------------------------
Ship it! samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java (line 54) <https://reviews.apache.org/r/43074/#comment179332> Minor nit: I think the max character line limit is 120 character. Ideally, it should be : ```java while (!containerRequestState.getRequestsQueue() && allocatedContainers != null && allocatedContainers.size() > 0) { ... } ``` - Navina Ramesh On Feb. 3, 2016, 8:04 p.m., Jake Maes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43074/ > ----------------------------------------------------------- > > (Updated Feb. 3, 2016, 8:04 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 > >
