> On May 4, 2018, 5:39 a.m., Benjamin Bannier wrote: > > src/master/master.hpp > > Line 442 (original), 442 (patched) > > <https://reviews.apache.org/r/66924/diff/1/?file=2016644#file2016644line442> > > > > The original pattern here seems to have been to always consitently > > break after the opening paren. > > > > Let's not change that.
We seem to only break after the opening paren if the first parameter is a `const process::UPID&`, see these cases in which we don't insert a line break: https://github.com/apache/mesos/blob/351bade6c28682daf821e88a40140e1364d69cb0/src/master/master.hpp#L523 https://github.com/apache/mesos/blob/351bade6c28682daf821e88a40140e1364d69cb0/src/master/master.hpp#L546 https://github.com/apache/mesos/blob/351bade6c28682daf821e88a40140e1364d69cb0/src/master/master.hpp#L569 https://github.com/apache/mesos/blob/351bade6c28682daf821e88a40140e1364d69cb0/src/master/master.hpp#L573 > On May 4, 2018, 5:39 a.m., Benjamin Bannier wrote: > > src/tests/storage_local_resource_provider_tests.cpp > > Lines 3500 (patched) > > <https://reviews.apache.org/r/66924/diff/1/?file=2016646#file2016646line3500> > > > > The behavior described in the comment above seems rather simple, but we > > use a pretty complicated setup below. This not only leads to us writing too > > much code unrelated to the thing under test, but also introduces unneeded > > dependencies on behavior we don't care about (here) -- e.g., I don't think > > this test depends on a SLRP. I would also much prefer a non-`ROOT` test so > > we run this as often as possible (I suspect developers do not regularly run > > `make check` as root on their development machines). > > > > I would suggest to reimplement the test with a `MockResourceProvider` > > if we do really need a `CREATE_VOLUME` operation below -- but it seems we > > should be able to perform this test with even non-RP operations which would > > simplify things further. > > > > See e.g., the test added in r/66908 which needs 60% less test code. I had originally implemented test like yours in r/66908, but then remembered that Mesos 1.6.0 will not support operation feedback on non-RP resources; I am going to add a validation rejecting operations that don't affect RP resources but have an operation ID, so we need a test that uses a resource provider. I agree that there is too much boiler plate in the test, I also think we should make it easier to write tests using a SLRP. We currently have no test that checks that the master correctly forwards the `OPERATION_DROPPED` updates generated by a SLRP when an operation ID is specified. This test checks that, but if I reimplement it to use a `MockResourceProvider`, then we should check in `StorageLocalResourceProviderTest.ROOT_ReconcileDroppedOperation` whether the update is correctly forwarded to the scheduler. - Gaston ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66924/#review202436 ----------------------------------------------------------- On May 3, 2018, 5 p.m., Gaston Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66924/ > ----------------------------------------------------------- > > (Updated May 3, 2018, 5 p.m.) > > > Review request for mesos, Benjamin Bannier and Greg Mann. > > > Bugs: MESOS-8784 > https://issues.apache.org/jira/browse/MESOS-8784 > > > Repository: mesos > > > Description > ------- > > Made the master include the operation ID in OPERATION_DROPPED updates. > > > Diffs > ----- > > src/master/master.hpp c30cf082d9ddf2e5dd97e0d887d6d07d8e03cbd4 > src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47 > src/tests/storage_local_resource_provider_tests.cpp > ccb114aacc904998dfeef4128f6d38dfb3c865c7 > > > Diff: https://reviews.apache.org/r/66924/diff/1/ > > > Testing > ------- > > `make check` on GNU/Linux. > > > The test in this patch fails without the corresponding master change, but > succeeds with it applied. > > > Thanks, > > Gaston Kleiman > >
