mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 66050: Implemented grow and shrink of persistent volumes.
Date Fri, 13 Apr 2018 00:33:12 GMT

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




src/master/master.cpp
Lines 4949 (patched)
<https://reviews.apache.org/r/66050/#comment282002>

    Nit:
    s/in operator API/in the operator API/
    
    Here and elsewhere.



src/master/master.cpp
Lines 4986-4994 (patched)
<https://reviews.apache.org/r/66050/#comment282048>

    What will happen if these operations are sent to a 1.5 agent which has the RESOURCE_PROVIDER
capability, but does not support the operation?



src/master/master.cpp
Lines 5029 (patched)
<https://reviews.apache.org/r/66050/#comment282015>

    Suggestion:
    s/with subtract/subtracting/



src/master/master.cpp
Lines 5331-5334 (original), 5467-5472 (patched)
<https://reviews.apache.org/r/66050/#comment282018>

    This change doesn't really belong in this patch. Also, if we're going to update this location
for consistency, then we should update other similar occurrences in this function as well
(like DESTROY_VOLUME below).
    
    I'd recommend either following up with a separate cleanup patch, or leaving this as-is.



src/master/validation.cpp
Lines 2344 (patched)
<https://reviews.apache.org/r/66050/#comment282039>

    Are you sure this will definitely result in a scalar value of zero? I think protobuf will
default-initialize the double in the `value` field, which leaves it in an indeterminate state?
    
    I think we should do `zero.set_value(0);` before the comparison is performed.
    
    Ditto for the shrink_volume validator below.



src/master/validation.cpp
Lines 2346 (patched)
<https://reviews.apache.org/r/66050/#comment282046>

    Suggestion:
    "The size of the 'addition' volume in 'grow_volume' must be greater than zero"
    
    If you end up enclosing 'grow_volume' in quotes here, then I would do it in other error
messages in these functions as well.



src/master/validation.cpp
Lines 2355 (patched)
<https://reviews.apache.org/r/66050/#comment282040>

    s/Not a/Invalid/
    
    Here and below.



src/master/validation.cpp
Lines 2359 (patched)
<https://reviews.apache.org/r/66050/#comment282041>

    Nit:
    s/Growing/Growing a/



src/master/validation.cpp
Lines 2392 (patched)
<https://reviews.apache.org/r/66050/#comment282042>

    s/cannot be zero/must be greater than zero/



src/master/validation.cpp
Lines 2397 (patched)
<https://reviews.apache.org/r/66050/#comment282047>

    Suggestion:
    "The value of 'subtract' in 'shrink_volume' must be smaller than the persistent volume's
size"
    
    If you end up enclosing 'shrink_volume' in quotes here, then I would do it in other error
messages as well.



src/master/validation.cpp
Lines 2412 (patched)
<https://reviews.apache.org/r/66050/#comment282044>

    Nit:
    s/Shrinking/Shrinking a/


- Greg Mann


On April 10, 2018, 4:18 a.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> -----------------------------------------------------------
> 
> (Updated April 10, 2018, 4:18 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
>     https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The new offer operations are implemented as speculative operations, but
> we will use validation to make them non-speculative on API level so that
> we can transition later without a breaking change.
> 
> 
> Diffs
> -----
> 
>   src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 
>   src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
>   src/tests/persistent_volume_tests.cpp 4edf781711d9efdb994114aeb6289b6af750b87a 
>   src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 
> 
> 
> Diff: https://reviews.apache.org/r/66050/diff/8/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


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