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

[Benjamin Bannier]

---
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.

[Chun-Hung Hsiao]

---
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)


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.

[Chun-Hung Hsiao]

---
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.

[Benjamin Bannier]

> On Aug. 9, 2019, 9:22 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 979-995 (patched)
> > 
> >
> > 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.

[Benjamin Bannier]

---
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.

[Benjamin Bannier]

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

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

[Chun-Hung Hsiao]

---
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)


Move `.then` to the next line.



src/resource_provider/storage/provider.cpp
Lines 746-750 (original), 739-749 (patched)


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)


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)


The return type should be `Try` 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)


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)


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.

[Benjamin Bannier]

> 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)
> > 
> >
> > 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)
> > 
> >
> > 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.

[Chun-Hung Hsiao]

---
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)


Did you accidentally add a new line?



src/resource_provider/storage/provider.cpp
Lines 728 (patched)


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



src/resource_provider/storage/provider.cpp
Lines 730 (patched)


Remove this debug log.



src/resource_provider/storage/provider.cpp
Lines 735 (patched)


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

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

[Benjamin Bannier]

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