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 Tue, 27 Mar 2018 19:14:29 GMT

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




src/common/resources_utils.cpp
Lines 203-205 (patched)
<https://reviews.apache.org/r/66050/#comment280665>

    This is already done in the validation code - do we need to do it again here? Ditto below.



src/common/resources_utils.cpp
Lines 207-208 (patched)
<https://reviews.apache.org/r/66050/#comment280667>

    Suggestion:
    
    "To grow a persistent volume, we consume the original volume and the additional resource
and convert into a single volume with the new size."



src/common/resources_utils.cpp
Lines 223 (patched)
<https://reviews.apache.org/r/66050/#comment280671>

    s/consume/consume the/



src/common/resources_utils.cpp
Lines 232-237 (patched)
<https://reviews.apache.org/r/66050/#comment280672>

    Could you provide a comment explaining the two cases here?



src/common/resources_utils.cpp
Lines 243 (patched)
<https://reviews.apache.org/r/66050/#comment280674>

    s/shrinked/shrunk/



src/common/resources_utils.cpp
Lines 696-698 (patched)
<https://reviews.apache.org/r/66050/#comment280675>

    No period at the end of error messages. Here and below.



src/master/master.cpp
Lines 4925-4928 (patched)
<https://reviews.apache.org/r/66050/#comment280694>

    Not indented far enough.



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

    s/have/have the/
    
    Here and below.



src/master/master.cpp
Lines 4945-4946 (patched)
<https://reviews.apache.org/r/66050/#comment280699>

    Indented too far.



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

    Could you update the indentation on these `drop()` calls to be consistent? I'm fine with
either
    ```
    drop(
        framework,
        etc...
    ```
    or
    ```
    drop(framework,
         etc...
    ```



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

    s/Operation/operation/
    
    Here and below.



src/master/master.cpp
Lines 4973-4974 (patched)
<https://reviews.apache.org/r/66050/#comment280700>

    Fits on one line.



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

    s/GROW_VOLUME/SHRINK_VOLUME/



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

    s/with subtract/subtracting/



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

    "Addition in grow_volume must be greater than zero"



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

    Fits on one line.



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

    Fits on one line.



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

    Is this error message accurate? Looks like it should be something like "Not a valid resource:
"



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

    "The 'subtract' field in 'shrink_volume' must be greater than zero"



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

    "The 'subtract' field in 'shrink_volume' must be smaller than the persistent volume's
size"



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

    Is this test necessary, given the above check for `shrink.volume().scalar() <= shrink.subtract()`?



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

    Fits on one line.


- Greg Mann


On March 27, 2018, 6:30 a.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> -----------------------------------------------------------
> 
> (Updated March 27, 2018, 6:30 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 non-speculative operations,
> which means they cannot be chained with previous offer operations which
> depend on results of each other.
> 
> 
> Diffs
> -----
> 
>   src/common/protobuf_utils.cpp 2acdc798200e82d14651b1edcc5f83e174477d53 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 20692c36c021fdae91de0b156f26fc56cf7c4f45 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp 9254cd22e67d9aca53135701575fb0857ff6f7d6 
>   src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
>   src/tests/mesos.hpp 01fd1c2bf21ef96dbab0769a1c1c92d7e3b60032 
>   src/tests/persistent_volume_tests.cpp 924d8458e54e34a49c99593482b5908c5f7c7a48 
>   src/tests/reservation_tests.cpp 8d8e9c8390e65187269bd194bb322bbdff88e0bd 
> 
> 
> Diff: https://reviews.apache.org/r/66050/diff/6/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


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