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 71150: Factored out storage provider method to update resources.
Date Fri, 09 Aug 2019 07:22:26 GMT

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


Fix it, then Ship it!





src/resource_provider/storage/provider.cpp
Line 718 (original), 728 (patched)
<https://reviews.apache.org/r/71150/#comment304403>

    Move `.then` to the next line.



src/resource_provider/storage/provider.cpp
Lines 746-750 (original), 739-749 (patched)
<https://reviews.apache.org/r/71150/#comment304401>

    The order of applying the resource conversions in the inner vector is important, so if
you prefer having a a flatterned vector, let's add a comment saying that the order is preserved
when flatterning.



src/resource_provider/storage/provider.cpp
Lines 949-950 (original), 931-932 (patched)
<https://reviews.apache.org/r/71150/#comment304399>

    It seems to me now that there's no need to hard-fail the SLRP here.
    
    Also, let's establish a convention that the error handling is always handled in the top-level
caller that doesn't propagate the failure, to avoid repeatitive log messages. So let's remove
the `onFailed` and `onDiscarded`.



src/resource_provider/storage/provider.cpp
Lines 976 (patched)
<https://reviews.apache.org/r/71150/#comment304405>

    The return type should be `Try<Nothing>` or simply `Nothing` since this is not an
asynchoronus function and never returns an error.



src/resource_provider/storage/provider.cpp
Lines 979-995 (patched)
<https://reviews.apache.org/r/71150/#comment304404>

    Sorry for not making my suggestion clear enough. I was actually thinking about removing
`reconcileStoragePools()` and always calling `reconcileResources()` even when we only want
to reconcile storage pools.
    
    This suggestion makes more sense if we don't reconcile storage pools after destroying
a MOUNT disk with a stale profile, as I suggested in the next patch. But if we want to keep
this behavior, then this approach would introduce an extra `ListVolumes` call.
    
    If you prefer to avoid having this extra grpc call, then adding this extra function seems
only give us a little bit of code reuse, and I'm not sure if it worths adding an extra function
name to the already-long list of functions in this class. I'd vote for keeping the code in
its original place and avoid introducing a function name that doesn't convey its purpose clearly.



src/resource_provider/storage/provider.cpp
Line 1874 (original), 1902 (patched)
<https://reviews.apache.org/r/71150/#comment304402>

    Let's print out log messages here instead:
    ```
    auto err = [](const Resource& resource, const string& message) {
      LOG(ERROR)
        << "Failed to reconcile storage pools after resource '" << resource
        << "' has been freed: " << message;
    };
    
    reconciled = sequence.add(...)
      .onFailed(std::bind(err, resource, lambda::_1))
      .onDiscarded(std::bind(err, resource, "future discarded"));
    ```


- Chun-Hung Hsiao


On July 23, 2019, 8:18 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71150/
> -----------------------------------------------------------
> 
> (Updated July 23, 2019, 8:18 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
>     https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Factored out storage provider method to update resources.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/storage/provider.cpp 6d632606f411d3ca99d3573a57c9f68b02ba8072

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


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