> On Aug. 15, 2018, 3:45 p.m., Benjamin Mahler wrote: > > src/tests/authentication_tests.cpp > > Lines 411-425 (patched) > > <https://reviews.apache.org/r/68354/diff/2/?file=2073117#file2073117line411> > > > > Can we push down the start into the loop (only for the 1st iteration) > > and avoid the 1st special casing here? It also seems a little odd to be > > doing manual calling into the function, can we also use the default mock > > behavior of calling through to avoid the manual calling?
The first authentication is different from retires. Only advancing the clock by `authentication_backoff_factor` will trigger the authentication. While retry is guaranteed to not happen before `authentication_timeout_min`. There may be some way to push the logic to the loop, but I feel the current code is more readable. We are testing retry specifically, excluding the first non-retry authentication would make it more clear. Dropping, feel free to reopen if you think otherwise. As for manual calling, I added some verbose comments. Hopefully, it will make it more understandable. > On Aug. 15, 2018, 3:45 p.m., Benjamin Mahler wrote: > > src/tests/authentication_tests.cpp > > Lines 412-413 (patched) > > <https://reviews.apache.org/r/68354/diff/2/?file=2073117#file2073117line412> > > > > Why are the underscores needed? It's confusing since `_authenticate` is > > also the continuation of `authenticate` Done. > On Aug. 15, 2018, 3:45 p.m., Benjamin Mahler wrote: > > src/tests/authentication_tests.cpp > > Lines 419 (patched) > > <https://reviews.apache.org/r/68354/diff/2/?file=2073117#file2073117line419> > > > > Do you actually still need this retirement on all of these? Expectaions are sticky, if I do not retire it here. It will oversaturate (since `authenticate()` will be called more times later) and fail. > On Aug. 15, 2018, 3:45 p.m., Benjamin Mahler wrote: > > src/tests/authentication_tests.cpp > > Lines 427 (patched) > > <https://reviews.apache.org/r/68354/diff/2/?file=2073117#file2073117line427> > > > > Why 10 here? Does it take that many to reach the max? Should probably > > explain to the reader the math on why 10? > > > > Maybe it's easier if this is just a while loop until we hit max or see > > two maxes? Sounds good. > On Aug. 15, 2018, 3:45 p.m., Benjamin Mahler wrote: > > src/tests/authentication_tests.cpp > > Lines 443-444 (patched) > > <https://reviews.apache.org/r/68354/diff/2/?file=2073117#file2073117line443> > > > > Maybe use milliseconds here? Not sure if this level of timer precision > > is supported on windows, etc. Done. > On Aug. 15, 2018, 3:45 p.m., Benjamin Mahler wrote: > > src/tests/authentication_tests.cpp > > Lines 462-463 (patched) > > <https://reviews.apache.org/r/68354/diff/2/?file=2073117#file2073117line462> > > > > Hm.. can't we test that max follows `min+b*2^n`? Done. > On Aug. 15, 2018, 3:45 p.m., Benjamin Mahler wrote: > > src/tests/authentication_tests.cpp > > Lines 468-471 (patched) > > <https://reviews.apache.org/r/68354/diff/2/?file=2073117#file2073117line468> > > > > I guess there's no delay between authentication success and > > registration? > > > > Doing the advance before the FUTURE_PROTOBUF is racy, it might miss the > > message and cause a flaky test. Done. - Meng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68354/#review207367 ----------------------------------------------------------- On Aug. 15, 2018, 5:06 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68354/ > ----------------------------------------------------------- > > (Updated Aug. 15, 2018, 5:06 p.m.) > > > Review request for mesos, Benjamin Mahler and Gastón Kleiman. > > > Repository: mesos > > > Description > ------- > > This test verifies that the agent backs-off properly when > retrying authentication according to the configured parameters. > > Also mocked `Slave::authenticate()` for this test. > > > Diffs > ----- > > src/slave/slave.hpp 802d4eb9e9eba2f1175dd85e56dcc80a61e32f74 > src/tests/authentication_tests.cpp c9a8f85951a50e278ae509f4efa7105755015ce9 > src/tests/mock_slave.hpp 9a74bf35d2cab0a72ba6376392239d8080a49304 > src/tests/mock_slave.cpp 94a5b0d20475f49dde99108a009682b520175aa4 > > > Diff: https://reviews.apache.org/r/68354/diff/3/ > > > Testing > ------- > > make check > > Added test continuously running without failure. > > > Thanks, > > Meng Zhu > >
