mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 66532: Added test for authorization actions for `RESIZE_VOLUME`.
Date Tue, 01 May 2018 04:05:19 GMT

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




src/tests/authorization_tests.cpp
Lines 1980 (patched)
<https://reviews.apache.org/r/66532/#comment283849>

    `resizing volumes`



src/tests/persistent_volume_tests.cpp
Lines 1027 (patched)
<https://reviews.apache.org/r/66532/#comment283853>

    Suggestion: I know this creates a little inconsistency within this test file, but it would
be more consistent if we use the full operation name (as in the code) in the backticks, i.e.,
"`GROW_VOLUME` and `SHRINK_VOLUME` operations." Or, just say "grow and shrink operations."



src/tests/persistent_volume_tests.cpp
Lines 1029-1030 (patched)
<https://reviews.apache.org/r/66532/#comment283884>

    We could combine the `CREATE` call with a `GROW_VOLUME` or `SHRINK_VOLUME` except for
the `GrowVolume` and `ShrinkVolume` tests, then this is TODO seems not that necessary.



src/tests/persistent_volume_tests.cpp
Lines 1037 (patched)
<https://reviews.apache.org/r/66532/#comment283854>

    Suggestion for the TODO: Make MOUNT a meaningful parameter value for this test, or create
a new fixture to avoid testing against it.



src/tests/persistent_volume_tests.cpp
Lines 1046-1051 (patched)
<https://reviews.apache.org/r/66532/#comment283858>

    This is not required. Also there's no need to create a nested block.



src/tests/persistent_volume_tests.cpp
Lines 1093 (patched)
<https://reviews.apache.org/r/66532/#comment283861>

    `Offer offer = offersBeforeCreate->at(0);`



src/tests/persistent_volume_tests.cpp
Lines 1095 (patched)
<https://reviews.apache.org/r/66532/#comment283862>

    `The disk spaces ... if the fixture parameter`



src/tests/persistent_volume_tests.cpp
Lines 1100 (patched)
<https://reviews.apache.org/r/66532/#comment283863>

    `which does not`



src/tests/persistent_volume_tests.cpp
Lines 1117-1120 (patched)
<https://reviews.apache.org/r/66532/#comment283896>

    Let's move this close to its first use.



src/tests/persistent_volume_tests.cpp
Lines 1124 (patched)
<https://reviews.apache.org/r/66532/#comment283864>

    `containing the original volume`



src/tests/persistent_volume_tests.cpp
Lines 1128 (patched)
<https://reviews.apache.org/r/66532/#comment283865>

    s/`and launch task `//



src/tests/persistent_volume_tests.cpp
Lines 1131 (patched)
<https://reviews.apache.org/r/66532/#comment283866>

    To make this test more succinct, how about doing
    `{CREATE(volume), GROW_VOLUME(volume, addition)}` so we can merge this `acceptOffers`
with the subsequent one?



src/tests/persistent_volume_tests.cpp
Lines 1140 (patched)
<https://reviews.apache.org/r/66532/#comment283885>

    `offer = offersBeforeGrow->at(0);`



src/tests/persistent_volume_tests.cpp
Lines 1152 (patched)
<https://reviews.apache.org/r/66532/#comment283877>

    `containing the grown volume`



src/tests/persistent_volume_tests.cpp
Lines 1156 (patched)
<https://reviews.apache.org/r/66532/#comment283887>

    `Grow the volume.` Or merge this call with the above `CREATE` one.



src/tests/persistent_volume_tests.cpp
Lines 1167 (patched)
<https://reviews.apache.org/r/66532/#comment283879>

    `offer = offersAfterGrow->at(0);`



src/tests/persistent_volume_tests.cpp
Lines 1177-1178 (patched)
<https://reviews.apache.org/r/66532/#comment283880>

    This is not needed since its the same as `addition`. If you prefer not to use the name
`addition` all the time, how about renaming it to `difference`?



src/tests/persistent_volume_tests.cpp
Lines 1182 (patched)
<https://reviews.apache.org/r/66532/#comment283883>

    `containing the original volume`



src/tests/persistent_volume_tests.cpp
Lines 1196 (patched)
<https://reviews.apache.org/r/66532/#comment283886>

    `offer = offersAfterShrink->at(0);`



src/tests/persistent_volume_tests.cpp
Lines 1213 (patched)
<https://reviews.apache.org/r/66532/#comment283888>

    Suggestion: This test verifies that `GROW_VOLUME` and `SHRINK_VOLUME` operations (or "grow
and shrink operations" if you prefer) get dropped if authorization fails and no principal
is supplied.



src/tests/persistent_volume_tests.cpp
Lines 1215 (patched)
<https://reviews.apache.org/r/66532/#comment283891>

    Suggestion: `BadACLDropGrowAndShrink` for consistency.



src/tests/persistent_volume_tests.cpp
Lines 1221 (patched)
<https://reviews.apache.org/r/66532/#comment283889>

    Suggestion for the TODO: Make MOUNT a meaningful parameter value for this test, or create
a new fixture to avoid testing against it.



src/tests/persistent_volume_tests.cpp
Lines 1230-1235 (patched)
<https://reviews.apache.org/r/66532/#comment283890>

    This is not required. Also there's no need to create a nested block.



src/tests/persistent_volume_tests.cpp
Lines 1256 (patched)
<https://reviews.apache.org/r/66532/#comment283898>

    Suggestion: Start the first framework with `DEFAULT_CREDENTIAL`, to be consistent with
your comment for the second framework.



src/tests/persistent_volume_tests.cpp
Lines 1277 (patched)
<https://reviews.apache.org/r/66532/#comment283892>

    `Offer offer = offersBeforeCreate->at(0);`



src/tests/persistent_volume_tests.cpp
Lines 1279 (patched)
<https://reviews.apache.org/r/66532/#comment283893>

    `The disk spaces ... if the fixture parameter`



src/tests/persistent_volume_tests.cpp
Lines 1284 (patched)
<https://reviews.apache.org/r/66532/#comment283894>

    `which does not`



src/tests/persistent_volume_tests.cpp
Lines 1292-1293 (patched)
<https://reviews.apache.org/r/66532/#comment283895>

    How about renaming `addition` to `difference` and removing `subtract`?



src/tests/persistent_volume_tests.cpp
Lines 1302-1305 (patched)
<https://reviews.apache.org/r/66532/#comment283897>

    Let's move this close to its first use.



src/tests/persistent_volume_tests.cpp
Lines 1309 (patched)
<https://reviews.apache.org/r/66532/#comment283900>

    `containing the original volume`



src/tests/persistent_volume_tests.cpp
Lines 1316 (patched)
<https://reviews.apache.org/r/66532/#comment283899>

    To make this test more succinct, how about doing
    `{CREATE(volume), GROW_VOLUME(volume, addition)}` so we can merge this `acceptOffers`
with the subsequent one, and check that the next offer contains the original volume but not
the grown one?



src/tests/persistent_volume_tests.cpp
Lines 1325 (patched)
<https://reviews.apache.org/r/66532/#comment283860>

    `offer = offersBeforeGrow1->at(0);`



src/tests/persistent_volume_tests.cpp
Lines 1336 (patched)
<https://reviews.apache.org/r/66532/#comment283901>

    This is not necessary if we examine the offers.



src/tests/persistent_volume_tests.cpp
Lines 1343 (patched)
<https://reviews.apache.org/r/66532/#comment283905>

    `Grow the volume.` Or merge with the above `acceptOffers`.



src/tests/persistent_volume_tests.cpp
Lines 1348 (patched)
<https://reviews.apache.org/r/66532/#comment283914>

    `Clock::settle();`



src/tests/persistent_volume_tests.cpp
Lines 1351 (patched)
<https://reviews.apache.org/r/66532/#comment283857>

    This test fails here:
    ```
    ../src/tests/persistent_volume_tests.cpp:1351: Failure
    Failed to wait 15secs for offersAfterGrow1
    ```



src/tests/persistent_volume_tests.cpp
Lines 1353 (patched)
<https://reviews.apache.org/r/66532/#comment283859>

    `offer = offersAfterGrow1->at(0);`



src/tests/persistent_volume_tests.cpp
Lines 1355 (patched)
<https://reviews.apache.org/r/66532/#comment283906>

    `Check that the offer still contains the original volume.`



src/tests/persistent_volume_tests.cpp
Lines 1362 (patched)
<https://reviews.apache.org/r/66532/#comment283907>

    `containing the original volume`



src/tests/persistent_volume_tests.cpp
Lines 1370 (patched)
<https://reviews.apache.org/r/66532/#comment283915>

    `Clock::settle();`



src/tests/persistent_volume_tests.cpp
Lines 1375 (patched)
<https://reviews.apache.org/r/66532/#comment283904>

    `offer = offersAfterShrink1->at(0);`



src/tests/persistent_volume_tests.cpp
Lines 1381 (patched)
<https://reviews.apache.org/r/66532/#comment283902>

    The subsequent `Clock::advance()` calls have no effect once the clock is resumed.
    
    I'm not sure if you can stop and join a driver with a paused clock. If not, please pause
the clock again afterward.



src/tests/persistent_volume_tests.cpp
Lines 1386 (patched)
<https://reviews.apache.org/r/66532/#comment283908>

    Suggestion: s/`which has`/`with`/



src/tests/persistent_volume_tests.cpp
Lines 1408 (patched)
<https://reviews.apache.org/r/66532/#comment283909>

    `offer = offersBeforeGrow2->at(0);`



src/tests/persistent_volume_tests.cpp
Lines 1423 (patched)
<https://reviews.apache.org/r/66532/#comment283910>

    `Grow the volume`



src/tests/persistent_volume_tests.cpp
Lines 1428 (patched)
<https://reviews.apache.org/r/66532/#comment283916>

    `Clock::settle();`



src/tests/persistent_volume_tests.cpp
Lines 1433 (patched)
<https://reviews.apache.org/r/66532/#comment283918>

    `offer = offersAfterGrow2->at(0);`



src/tests/persistent_volume_tests.cpp
Lines 1435 (patched)
<https://reviews.apache.org/r/66532/#comment283911>

    `Check that the offer still contains the original volume.`



src/tests/persistent_volume_tests.cpp
Lines 1442 (patched)
<https://reviews.apache.org/r/66532/#comment283912>

    `containing the original volume`



src/tests/persistent_volume_tests.cpp
Lines 1450 (patched)
<https://reviews.apache.org/r/66532/#comment283917>

    `Clock::settle();`



src/tests/persistent_volume_tests.cpp
Lines 1464 (patched)
<https://reviews.apache.org/r/66532/#comment283913>

    No need to resume the clock here, as the fixture teardown will resume it. However, I'm
not sure if you need to resume the clock before stopping and joining `driver2`.


- Chun-Hung Hsiao


On April 27, 2018, 8:08 p.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> -----------------------------------------------------------
> 
> (Updated April 27, 2018, 8:08 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
>     https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added test for authorization actions for `RESIZE_VOLUME`.
> 
> 
> Diffs
> -----
> 
>   src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
>   src/tests/persistent_volume_tests.cpp 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66532/diff/6/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


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