----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49175/#review139658 -----------------------------------------------------------
Fix it, then Ship it! 3rdparty/libprocess/src/tests/process_tests.cpp (line 741) <https://reviews.apache.org/r/49175/#comment204928> brace on next line here and elsewhere 3rdparty/libprocess/src/tests/process_tests.cpp (line 743) <https://reviews.apache.org/r/49175/#comment204943> Looks like we need a TearDown which ensures that the subprocess is cleaned up if the test did not kill it. This will let a few of the tests avoid needing to call the more comprehensive kill_linkee (which should be used when the test needs the status, to avoid the reap delays). 3rdparty/libprocess/src/tests/process_tests.cpp (line 745) <https://reviews.apache.org/r/49175/#comment204932> Let's not bother with this anymore because it seems to have proliferated via being copied around and we should be checking this at the start of the libprocess test main. However, I'm not sure we'd even compile if this was false given it seems to correspond to whether we have pthread support. 3rdparty/libprocess/src/tests/process_tests.cpp (line 748) <https://reviews.apache.org/r/49175/#comment204933> It may not be clear to the reader that this is accomplished via receiving a message from the child. Do we need this? 3rdparty/libprocess/src/tests/process_tests.cpp (line 757) <https://reviews.apache.org/r/49175/#comment204934> Do you need to hardcode the port? 3rdparty/libprocess/src/tests/process_tests.cpp (lines 763 - 770) <https://reviews.apache.org/r/49175/#comment204937> Can you take this out now that we figured out the bug here? 3rdparty/libprocess/src/tests/process_tests.cpp (lines 779 - 790) <https://reviews.apache.org/r/49175/#comment204939> Hm.. perhaps document that this is in place to perform the reaping speed up? is virtual needed? 3rdparty/libprocess/src/tests/process_tests.cpp (line 781) <https://reviews.apache.org/r/49175/#comment204940> s/.get()./->/ here and elsewhere 3rdparty/libprocess/src/tests/process_tests.cpp (lines 783 - 789) <https://reviews.apache.org/r/49175/#comment204941> Hm.. this is a bit unfortunate in that the caller's clock will be resumed implicitly? Can we only resume if we need to pause here in the callee? E.g. https://github.com/apache/mesos/blob/8c7b227c859f199bdbfbf63ab47a30688e762737/src/tests/cluster.cpp#L318-L328 3rdparty/libprocess/src/tests/process_tests.cpp (lines 805 - 815) <https://reviews.apache.org/r/49175/#comment204942> The status being satisfied does not necessarily entail that the exited message is delivered since settling the clock does not ensure that socket events are processed. Can you use a Future instead of the atomic_bool? 3rdparty/libprocess/src/tests/process_tests.cpp (lines 833 - 834) <https://reviews.apache.org/r/49175/#comment204945> In a specific situation, right? i.e. when there is no persistent connection 3rdparty/libprocess/src/tests/process_tests.cpp (lines 839 - 849) <https://reviews.apache.org/r/49175/#comment204946> Ditto using a Future for exited, here and below. 3rdparty/libprocess/src/tests/process_tests.cpp (lines 860 - 861) <https://reviews.apache.org/r/49175/#comment204948> Can you put these inside initialize, here and below? 3rdparty/libprocess/src/tests/process_tests.cpp (line 927) <https://reviews.apache.org/r/49175/#comment204951> Do this within initialize. 3rdparty/libprocess/src/tests/process_tests.cpp (lines 934 - 936) <https://reviews.apache.org/r/49175/#comment204954> maybe just `ping_linkee`? 3rdparty/libprocess/src/tests/process_tests.cpp (line 935) <https://reviews.apache.org/r/49175/#comment204950> Can you avoid the 22 here? 3rdparty/libprocess/src/tests/process_tests.cpp (lines 973 - 979) <https://reviews.apache.org/r/49175/#comment204961> Can you check for the specific error codes that indicates that the connection isn't established yet, to avoid looping if something more erroenous is happening? Also consider bounding this loop via a timeout and avoiding the tight loop via a small sleep (you can find other examples in the tests). Also, do you need to log to stdout here? Seems nice to only log upon failing after the timeout. Ditto in the test below. 3rdparty/libprocess/src/tests/process_tests.cpp (lines 973 - 976) <https://reviews.apache.org/r/49175/#comment204966> Can you point out that this simulates the "stale" socket scenario in which we don't receive a RST but a later send will trigger a socket error? 3rdparty/libprocess/src/tests/process_tests.cpp (lines 986 - 991) <https://reviews.apache.org/r/49175/#comment204964> Can you avoid the settling once you change to using a Future? It seems pretty brittle to assume that the link breakage will propagate definitively after settling. 3rdparty/libprocess/src/tests/process_tests.cpp (lines 1001 - 1003) <https://reviews.apache.org/r/49175/#comment204972> The first sentence seems a bit out of context for the reader that approaches this test without having read the previous test. 3rdparty/libprocess/src/tests/test_linkee.cpp (line 24) <https://reviews.apache.org/r/49175/#comment204927> Maybe call this LinkeeTestProcess? Ideally we could call this StaleLinkProcess but it looks like we're using to test more than this particular case. 3rdparty/libprocess/src/tests/test_linkee.cpp (lines 27 - 33) <https://reviews.apache.org/r/49175/#comment204922> Can you fix the braces here and in the test? 3rdparty/libprocess/src/tests/test_linkee.cpp (line 28) <https://reviews.apache.org/r/49175/#comment204923> Can you avoid using 10 here in favor of s.size() or strlen? Ideally we have send overloads that can take a string (by reference or move) but we don't have these at the current time. If you'd like to fix that I'd be happy to review. Or just: ``` send(parent, "ALIVE", "", 0); ``` 3rdparty/libprocess/src/tests/test_linkee.cpp (lines 31 - 33) <https://reviews.apache.org/r/49175/#comment204925> Can you describe why you need this here? It's a bit hard for someone coming across this to figure out why this was added. The description at the top could describe more of the "why" than the "what": ``` // Used for testing the remote link semantics, this will // send a message to the "parent" Process in order to allow // the parent to discover the UPID of the linkee. // // In order to simulate "stale" links (see MESOS-XXXX), this // will exit upon receiving a message. class LinkeeProcess ``` 3rdparty/libprocess/src/tests/test_linkee.cpp (lines 44 - 48) <https://reviews.apache.org/r/49175/#comment204924> How about using flags or returning a help string if no arguments are provided or we can't parse it as a pid. - Benjamin Mahler On June 24, 2016, 2:43 a.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49175/ > ----------------------------------------------------------- > > (Updated June 24, 2016, 2:43 a.m.) > > > Review request for mesos, Benjamin Hindman, Benjamin Mahler, Gilbert Song, > Artem Harutyunyan, and Jie Yu. > > > Bugs: MESOS-5576 > https://issues.apache.org/jira/browse/MESOS-5576 > > > Repository: mesos > > > Description > ------- > > Adds tests which exercise "link" semantics against remote processes. > This includes detection of `ExitedEvents` when the process exits > as well as mixing "link" semantics. > > Includes a test case that emulates the failure observed in MESOS-5576. > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am 3c3249fe9799ba919ac7bd13e2ddb07a306737f0 > 3rdparty/libprocess/src/tests/process_tests.cpp > 3fce6fc41faaa7b6e8a2a957e85de3de973a51ba > 3rdparty/libprocess/src/tests/test_linkee.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/49175/diff/ > > > Testing > ------- > > See end of chain. > > > Thanks, > > Joseph Wu > >
