You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <bb...@apache.org> on 2019/07/23 20:18:15 UTC

Review Request 71150: Factored out storage provider method to update resources.

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

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/1/


Testing
-------

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71150: Factored out storage provider method to update resources.

Posted by Benjamin Bannier <bb...@apache.org>.

> On Aug. 9, 2019, 9:22 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 979-995 (patched)
> > <https://reviews.apache.org/r/71150/diff/3/?file=2159962#file2159962line1016>
> >
> >     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.

Updated the code, PTAL.


- Benjamin


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


On Aug. 19, 2019, 1:58 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71150/
> -----------------------------------------------------------
> 
> (Updated Aug. 19, 2019, 1:58 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 
>   src/tests/storage_local_resource_provider_tests.cpp 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71150/diff/5/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71150: Factored out storage provider method to update resources.

Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
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
> 
>

Re: Review Request 71150: Factored out storage provider method to update resources.

Posted by Benjamin Bannier <bb...@apache.org>.

> On Aug. 2, 2019, 9:24 a.m., Chun-Hung Hsiao wrote:
> > I'm not sure if this is

Could you rephrase that?


> On Aug. 2, 2019, 9:24 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 730 (patched)
> > <https://reviews.apache.org/r/71150/diff/1/?file=2157525#file2157525line730>
> >
> >     Remove this debug log.

Moved the removal in https://reviews.apache.org/r/71151/ here.


> On Aug. 2, 2019, 9:24 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 735 (patched)
> > <https://reviews.apache.org/r/71150/diff/1/?file=2157525#file2157525line735>
> >
> >     Remove this debug log.

Moved the removal in https://reviews.apache.org/r/71151/ here.


- Benjamin


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


On July 23, 2019, 10: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, 10: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/2/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71150: Factored out storage provider method to update resources.

Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71150/#review216947
-----------------------------------------------------------



I'm not sure if this is


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

    Did you accidentally add a new line?



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

    How about merging this function and `reconcileStoragePools` since the logic is very similar?



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

    Remove this debug log.



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

    Remove this debug log.


- 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/1/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71150: Factored out storage provider method to update resources.

Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71150/#review217314
-----------------------------------------------------------


Ship it!




Ship It!

- Chun-Hung Hsiao


On Aug. 19, 2019, 11:58 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71150/
> -----------------------------------------------------------
> 
> (Updated Aug. 19, 2019, 11:58 a.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 
>   src/tests/storage_local_resource_provider_tests.cpp 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71150/diff/5/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71150: Factored out storage provider method to update resources.

Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71150/#review217316
-----------------------------------------------------------




src/resource_provider/storage/provider.cpp
Lines 722-723 (original), 729-739 (patched)
<https://reviews.apache.org/r/71150/#comment304617>

    Do you think this extra flattern code makes the next continuation more readable? If not maybe let's just keep the original `foreach` loop to save an extra copy and an extra continuation?
    
    Otherwise, please feel free to drop this. I already gave you another ship-it :)


- Chun-Hung Hsiao


On Aug. 19, 2019, 11:58 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71150/
> -----------------------------------------------------------
> 
> (Updated Aug. 19, 2019, 11:58 a.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 
>   src/tests/storage_local_resource_provider_tests.cpp 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71150/diff/5/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71150: Factored out storage provider method to update resources.

Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71150/
-----------------------------------------------------------

(Updated Aug. 20, 2019, 11:25 a.m.)


Review request for mesos and Chun-Hung Hsiao.


Changes
-------

Address comments from Chun


Bugs: MESOS-9254
    https://issues.apache.org/jira/browse/MESOS-9254


Repository: mesos


Description
-------

Factored out storage provider method to update resources.


Diffs (updated)
-----

  src/resource_provider/storage/provider.cpp 6d632606f411d3ca99d3573a57c9f68b02ba8072 
  src/tests/storage_local_resource_provider_tests.cpp 69b59d48ceefebbb7accefe411c54ac5cecff1c3 


Diff: https://reviews.apache.org/r/71150/diff/6/

Changes: https://reviews.apache.org/r/71150/diff/5-6/


Testing
-------

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71150: Factored out storage provider method to update resources.

Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71150/
-----------------------------------------------------------

(Updated Aug. 19, 2019, 1:58 p.m.)


Review request for mesos and Chun-Hung Hsiao.


Changes
-------

Address issue raised by Chun


Bugs: MESOS-9254
    https://issues.apache.org/jira/browse/MESOS-9254


Repository: mesos


Description
-------

Factored out storage provider method to update resources.


Diffs (updated)
-----

  src/resource_provider/storage/provider.cpp 6d632606f411d3ca99d3573a57c9f68b02ba8072 
  src/tests/storage_local_resource_provider_tests.cpp 69b59d48ceefebbb7accefe411c54ac5cecff1c3 


Diff: https://reviews.apache.org/r/71150/diff/5/

Changes: https://reviews.apache.org/r/71150/diff/4-5/


Testing
-------

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71150: Factored out storage provider method to update resources.

Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71150/
-----------------------------------------------------------

(Updated Aug. 15, 2019, 3:31 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 (updated)
-----

  src/resource_provider/storage/provider.cpp 6d632606f411d3ca99d3573a57c9f68b02ba8072 


Diff: https://reviews.apache.org/r/71150/diff/4/

Changes: https://reviews.apache.org/r/71150/diff/3-4/


Testing
-------

`make check`


Thanks,

Benjamin Bannier