> On June 18, 2018, 2:08 p.m., Benjamin Bannier wrote: > > src/tests/storage_local_resource_provider_tests.cpp > > Lines 2561-2562 (patched) > > <https://reviews.apache.org/r/65995/diff/7/?file=2040455#file2040455line2562> > > > > If there is a reason to `settle` before we `advance`, we should add a > > comment, otherwise I would expect a sequence of first `advance`, then > > `settle`. > > > > We don't seem to be very disciplined to _always_ `settle` after and > > `advance` in this file, we could clean that up in a follow-up. > > Chun-Hung Hsiao wrote: > Yes there's a reason to do `settle` before `advance`: we need to ensure > that the allocater gets the RP resouces before doing any allocation. I'll add > a comment here. > > However, it seems to me that it needs not to always `settle` after > `advance`. IMO `settle` enforces many synchronizations, and sometimes > enforces too many for our unit tests to reveal certain schedules that might > expose data races. > Instead, I'd prefer more specific synchronizations after `advance`. For > example, > ``` > Clock::advance(masterFlags.allocation_interval); > > AWAIT_READY(offers); > ``` > The above `advance` is for triggering an offer, and thus waiting for the > offer should be sufficient. > > WDYT? > > Chun-Hung Hsiao wrote: > Hmm sorry I made a mistake on the previous `settle`. Actually there's > already a comment there to explain why we do the `settle` first.
Makes sense, dropping. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65995/#review204917 ----------------------------------------------------------- On June 14, 2018, 2:02 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65995/ > ----------------------------------------------------------- > > (Updated June 14, 2018, 2:02 a.m.) > > > Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, and > Jie Yu. > > > Bugs: MESOS-8825 > https://issues.apache.org/jira/browse/MESOS-8825 > > > Repository: mesos > > > Description > ------- > > The two SLRP tests assume that SLRP will send out a RAW resource in its > first `UPDATE_STATE` message, and expect that the test framework would > receive an offer containing the RAW resource in its first offer. However > this behavior is not guaranteed and should not be relied on. This patch > makes the tests decline unwanted offers by default so they no longer > rely on SLRP's internal behavior. > > > Diffs > ----- > > src/tests/storage_local_resource_provider_tests.cpp > 1afe8a8e0413ef225e952cd9cd6376e5d82774e5 > > > Diff: https://reviews.apache.org/r/65995/diff/7/ > > > Testing > ------- > > sudo make check > Ran the two tests in repitition. > > > Thanks, > > Chun-Hung Hsiao > >
