mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gaston Kleiman <gas...@mesosphere.io>
Subject Re: Review Request 66924: Made the master include the operation ID in OPERATION_DROPPED updates.
Date Fri, 04 May 2018 17:06:54 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message