mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zhitao Li <zhitaoli...@gmail.com>
Subject Re: Review Request 66220: Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
Date Mon, 16 Apr 2018 17:57:42 GMT

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



Please consider unreplied comments as "will do".


src/tests/persistent_volume_tests.cpp
Lines 453 (patched)
<https://reviews.apache.org/r/66220/#comment282370>

    Is the goal of splitting the test to have more focused tests? Otherwise, just taking out
the task launching part from the code seems resulting in minimum amount of code.
    
    The file check actually verifies the fix for r/66218, which I think is something pretty
important (not losing data during a grow/shrink). Only checking path exists for the volume
would not capture that behavior.



src/tests/persistent_volume_tests.cpp
Lines 455-459 (patched)
<https://reviews.apache.org/r/66220/#comment282372>

    I think we discussed about not allowing this but I forgot add this to validation. I'll
update parent patch to include that validation.
    
    If we will drop any `GROW`/`SHRINK` operation, then we can verify that `GROW` does not
trigger any `ApplyOperationMessage` and framework will be offered original volume as is and
keep this well tested.



src/tests/persistent_volume_tests.cpp
Lines 492 (patched)
<https://reviews.apache.org/r/66220/#comment282373>

    I do not see a possibility. Let me try to drop it and see what happens.



src/tests/persistent_volume_tests.cpp
Lines 541-542 (patched)
<https://reviews.apache.org/r/66220/#comment282374>

    Let's capture this message and test its content to make sure proper operation is forwarded.



src/tests/persistent_volume_tests.cpp
Lines 597-603 (patched)
<https://reviews.apache.org/r/66220/#comment282376>

    This was necessary if the operation is implemented as non-speculative, because master/allocator
only sends out `converted` resources after receiving the feedback from agent. If we don't
ensure this is processed, the offer could be subject to race condition (I think).
    
    Now that we changed the course, this may not be necessary in 1.6, but we'd need them again
once we get to MESOS-8791.
    
    Please let me know if you think keeping it is still a bad idea.



src/tests/persistent_volume_tests.cpp
Lines 677-678 (patched)
<https://reviews.apache.org/r/66220/#comment282378>

    Resources::contains only matches a volume if their size perfectly matches, so I think
it is sufficient.


- Zhitao Li


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> -----------------------------------------------------------
> 
> (Updated April 11, 2018, 2:19 p.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
> -------
> 
> Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -----
> 
>   src/tests/persistent_volume_tests.cpp 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


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