> On April 8, 2016, 8:42 p.m., Chris Pettitt wrote: > > samza-yarn/src/test/java/org/apache/samza/job/yarn/TestHostAwareContainerAllocator.java, > > line 59 > > <https://reviews.apache.org/r/45190/diff/2/?file=1330433#file1330433line59> > > > > 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.
It was bothering me too, so it's done. > On April 8, 2016, 8:42 p.m., Chris Pettitt wrote: > > samza-yarn/src/test/java/org/apache/samza/job/yarn/TestHostAwareContainerAllocator.java, > > line 362 > > <https://reviews.apache.org/r/45190/diff/2/?file=1330433#file1330433line362> > > > > 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. Done and boy, does it make the code more readable. Thanks for this suggestion. > On April 8, 2016, 8:42 p.m., Chris Pettitt wrote: > > samza-yarn/src/test/java/org/apache/samza/job/yarn/util/MockContainerListener.java, > > line 86 > > <https://reviews.apache.org/r/45190/diff/2/?file=1330434#file1330434line86> > > > > 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. It's like you made your comments in ascending order of importance. :-) Spurious wakeups! Those were causing one of the tests to fail on occasion. I implemented the latch and now I can't recreate the failure on either of my machines. I think this long-standing issue might finally be resolved! Unfortunately, order does matter, so I couldn't push all the assertions to the main thread, but the latch still makes it cleaner and more reliable. - Jake ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45190/#review127874 ----------------------------------------------------------- On April 8, 2016, 11:27 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, 11:27 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/TestContainerAllocatorCommon.java > PRE-CREATION > > 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 > >
