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 71151: Performed periodic storage local provider reconciliations.
Date Fri, 09 Aug 2019 07:41:19 GMT

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


Fix it, then Ship it!





src/resource_provider/storage/provider.cpp
Line 735 (original), 796 (patched)
<https://reviews.apache.org/r/71151/#comment304408>

    Let's check that the `reconciled` future is pending at the very beginning of this function,
similar to what we do in `reconcileStoragePools`, because `getExistingVolumes` and `getStoragePools`
must be called after a reconciliation starts.



src/resource_provider/storage/provider.cpp
Line 752 (original), 811 (patched)
<https://reviews.apache.org/r/71151/#comment304409>

    As commented in the previous patch, since we're having two functions, `reconcileStoragePools`
and `reconcileResources`, it's alright to have a little bit of repeated code and don't introduce
an extra function without a very focused purpose.
    
    In `reconcileResources`, we could have the logic that checks for `alwaysUpdate`. In `reconcileStoragePools`,
since it's only called when profiles are updated or volumes of a stale profile are deleted,
there's no need for the extra `alwaysUpdate` logic there.



src/tests/storage_local_resource_provider_tests.cpp
Lines 6743 (patched)
<https://reviews.apache.org/r/71151/#comment304411>

    These are kinda ugly. Probably should find some time to refactor them in the future :)



src/tests/storage_local_resource_provider_tests.cpp
Lines 6871 (patched)
<https://reviews.apache.org/r/71151/#comment304412>

    Let's do a `Clock::settle()` before `Clock::advance()` to ensure that the timer has been
set up.



src/tests/storage_local_resource_provider_tests.cpp
Lines 6881 (patched)
<https://reviews.apache.org/r/71151/#comment304413>

    Ditto.


- Chun-Hung Hsiao


On Aug. 6, 2019, 2:27 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2019, 2:27 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
>     https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/storage/provider.cpp 6d632606f411d3ca99d3573a57c9f68b02ba8072

>   src/tests/storage_local_resource_provider_tests.cpp 69b59d48ceefebbb7accefe411c54ac5cecff1c3

> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/4/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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