----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45190/#review127874 -----------------------------------------------------------
Fix it, then Ship it! samza-yarn/src/test/java/org/apache/samza/job/yarn/TestHostAwareContainerAllocator.java (line 59) <https://reviews.apache.org/r/45190/#comment191234> It looks like this and TestContainerAllocator have some overlap. I wonder if it would be easy to have a base class to cover the common functionality? Not necessary for this RB as it is not related to your change, but worth considering. samza-yarn/src/test/java/org/apache/samza/job/yarn/TestHostAwareContainerAllocator.java (line 359) <https://reviews.apache.org/r/45190/#comment191233> Not necessarily specific to your RB, but maybe a low hanging fruit: I would suggest extracting out all of the Runnables into local variables. That way you can document what they're doing via the name and the construction of listener is easier to follow. samza-yarn/src/test/java/org/apache/samza/job/yarn/util/MockContainerListener.java (line 76) <https://reviews.apache.org/r/45190/#comment191236> This doesn't handle spurious wakeups: https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#wait() --- Assuming that the order in which post condition assertions are applied does not matter (maybe a bad assumption?) I think this could be simplified massively by using a CountDownLatch to join once everything is done. You could make the numXXX fields volatile and after join check all of the post conditions on the main thread (e.g. in the test code that calls this). One other nice thing about this approach (other than killing off several LoC) is that you get a timeout for the whole process versus timeouts on individual steps. Looks very good! I especially like the more intuitive host names. - Chris Pettitt On April 8, 2016, 3:02 p.m., Jake Maes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45190/ > ----------------------------------------------------------- > > (Updated April 8, 2016, 3:02 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > SAMZA-910 Fix expired request test in HostAwareContainerAllocator > > Summary of changes: > 1. Remove the last sleep() from HostAwareContainerAllocator > 2. Fix a silent failure in testRerequestOnAnyHostIfContainerStartFails by > setting the neededContainers to 1 before running the test. > 3. Update MockContainerListener so assertion failures in other threads are > thrown in the main thread to fail the test. (no silent failures) This should > help troubleshoot the tests if there are any remaining issues. > 4. Rename obscure hostnames to make it easier to reason about the tests. > > > Diffs > ----- > > > 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 > > samza-yarn/src/test/java/org/apache/samza/job/yarn/util/MockContainerUtil.java > 2f9669f8b7e77abb65b244ccd067ae7ab1f245c3 > > Diff: https://reviews.apache.org/r/45190/diff/ > > > Testing > ------- > > > Thanks, > > Jake Maes > >
