mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 66924: Made the master include the operation ID in OPERATION_DROPPED updates.
Date Fri, 04 May 2018 12:39:18 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66924/#review202436
-----------------------------------------------------------




src/master/master.hpp
Line 442 (original), 442 (patched)
<https://reviews.apache.org/r/66924/#comment284252>

    The original pattern here seems to have been to always consitently break after the opening
paren.
    
    Let's not change that.



src/master/master.hpp
Line 506 (original), 505 (patched)
<https://reviews.apache.org/r/66924/#comment284254>

    See above.



src/master/master.hpp
Line 513 (original), 511 (patched)
<https://reviews.apache.org/r/66924/#comment284255>

    Break after `(`.



src/tests/storage_local_resource_provider_tests.cpp
Lines 3500 (patched)
<https://reviews.apache.org/r/66924/#comment284261>

    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.


- Benjamin Bannier


On May 4, 2018, 2 a.m., Gaston Kleiman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66924/
> -----------------------------------------------------------
> 
> (Updated May 4, 2018, 2 a.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