You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@apache.org> on 2018/04/12 03:35:12 UTC
Re: Review Request 65594: Checkpointed profiles in storage local
resource provider.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65594/
-----------------------------------------------------------
(Updated April 12, 2018, 3:35 a.m.)
Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
Changes
-------
Rebased and adopted CSI v0.2.
Bugs: MESOS-8492
https://issues.apache.org/jira/browse/MESOS-8492
Repository: mesos
Description
-------
SLRP now checkpoints profiles associated with storage pools, and does
not depend on the `DiskProfileAdaptor` module to return the set of
previously-known profiles during recovery.
Diffs (updated)
-----
src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b
src/resource_provider/storage/provider.cpp a07620d1c4bf618f669575b3e183bf598da905a6
Diff: https://reviews.apache.org/r/65594/diff/8/
Changes: https://reviews.apache.org/r/65594/diff/7-8/
Testing
-------
sudo make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 65594: Checkpointed profiles in storage local
resource provider.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On April 20, 2018, 12:57 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 1010 (original), 1011 (patched)
> > <https://reviews.apache.org/r/65594/diff/8/?file=1996911#file1996911line1015>
> >
> > Nit: _pending operations_ should be specific enough as this list is neither exhaustive today nor will remain in the future.
I'm specifically talking about `CREATE_VOLUME` and `CREATE_BLOCK` here. `DESTROY_VOLUME` and `DESTROY_BLOCK` don't need the profile info.
> On April 20, 2018, 12:57 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 1021 (original), 1014 (patched)
> > <https://reviews.apache.org/r/65594/diff/8/?file=1996911#file1996911line1026>
> >
> > Both `disk` and `profile` are `optional` fields. We should check explicitly whether they are set (otherwise we might at some point e.g., hit a failure where the profile `` (empty string) was not found.
The implicit invariant here is that each resource either has it's id or profile set. I'll add a check to make it more explicit.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65594/#review201627
-----------------------------------------------------------
On April 12, 2018, 3:35 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> -----------------------------------------------------------
>
> (Updated April 12, 2018, 3:35 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
>
>
> Repository: mesos
>
>
> Description
> -------
>
> SLRP now checkpoints profiles associated with storage pools, and does
> not depend on the `DiskProfileAdaptor` module to return the set of
> previously-known profiles during recovery.
>
>
> Diffs
> -----
>
> src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b
> src/resource_provider/storage/provider.cpp a07620d1c4bf618f669575b3e183bf598da905a6
>
>
> Diff: https://reviews.apache.org/r/65594/diff/8/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 65594: Checkpointed profiles in storage local
resource provider.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On April 20, 2018, 12:57 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3276 (patched)
> > <https://reviews.apache.org/r/65594/diff/8/?file=1996911#file1996911line3318>
> >
> > Should we capture this?
> >
> > ProfileInfo& profile_ = *(storage->mutable_profiles())[profile];
> >
> > No strong opinion, it seems hard to make interacting with proto maps nice.
>
> Chun-Hung Hsiao wrote:
> I'll do `ProfileInfo& profileInfo_ = ...`.
>
> Chun-Hung Hsiao wrote:
> BTW the deference operator (`*`) applies on `mutable_profiles()` not `[]`.
Typo in the above comment: `s/mutable_profiles/mutable_capability/`.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65594/#review201627
-----------------------------------------------------------
On April 12, 2018, 3:35 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> -----------------------------------------------------------
>
> (Updated April 12, 2018, 3:35 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
>
>
> Repository: mesos
>
>
> Description
> -------
>
> SLRP now checkpoints profiles associated with storage pools, and does
> not depend on the `DiskProfileAdaptor` module to return the set of
> previously-known profiles during recovery.
>
>
> Diffs
> -----
>
> src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b
> src/resource_provider/storage/provider.cpp a07620d1c4bf618f669575b3e183bf598da905a6
>
>
> Diff: https://reviews.apache.org/r/65594/diff/8/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 65594: Checkpointed profiles in storage local
resource provider.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On April 20, 2018, 12:57 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 1003 (patched)
> > <https://reviews.apache.org/r/65594/diff/8/?file=1996911#file1996911line1007>
> >
> > Even thought an `auto` might make sense in such a place _in general_, due to the interface of `MapInfo` it leads to pretty dense code and maybe repeating a type here might make this easier to read. How about
> >
> > // NOTE: this form requires a `using` decl above.
> > using Profile =
> > MapPair<string, ResourceProviderState::Storage::ProfileInfo>;
> >
> > foreach (const Profile& profile, storage.profiles()) {
> > profileInfos.put(
> > profile.first,
> > {profile.second.capability(), profile.second.parameters()});
> > }
Adopted your suggestion, but I use `ProfileEntry` instead of `Profile` and keep the variable name `entry`. I'd like to make it consistent across the code that `profile` indicates a name, `profileInfo` indicates the CSI data, and `entry` indicates a map pair.
> On April 20, 2018, 12:57 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 1010 (original), 1011 (patched)
> > <https://reviews.apache.org/r/65594/diff/8/?file=1996911#file1996911line1015>
> >
> > Nit: _pending operations_ should be specific enough as this list is neither exhaustive today nor will remain in the future.
>
> Chun-Hung Hsiao wrote:
> I'm specifically talking about `CREATE_VOLUME` and `CREATE_BLOCK` here. `DESTROY_VOLUME` and `DESTROY_BLOCK` don't need the profile info.
Removed them from the comments as suggested as the explicitness here doesn't bring much more clarity.
> On April 20, 2018, 12:57 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3276 (patched)
> > <https://reviews.apache.org/r/65594/diff/8/?file=1996911#file1996911line3318>
> >
> > Should we capture this?
> >
> > ProfileInfo& profile_ = *(storage->mutable_profiles())[profile];
> >
> > No strong opinion, it seems hard to make interacting with proto maps nice.
I'll do `ProfileInfo& profileInfo_ = ...`.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65594/#review201627
-----------------------------------------------------------
On April 12, 2018, 3:35 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> -----------------------------------------------------------
>
> (Updated April 12, 2018, 3:35 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
>
>
> Repository: mesos
>
>
> Description
> -------
>
> SLRP now checkpoints profiles associated with storage pools, and does
> not depend on the `DiskProfileAdaptor` module to return the set of
> previously-known profiles during recovery.
>
>
> Diffs
> -----
>
> src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b
> src/resource_provider/storage/provider.cpp a07620d1c4bf618f669575b3e183bf598da905a6
>
>
> Diff: https://reviews.apache.org/r/65594/diff/8/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 65594: Checkpointed profiles in storage local
resource provider.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On April 20, 2018, 12:57 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3276 (patched)
> > <https://reviews.apache.org/r/65594/diff/8/?file=1996911#file1996911line3318>
> >
> > Should we capture this?
> >
> > ProfileInfo& profile_ = *(storage->mutable_profiles())[profile];
> >
> > No strong opinion, it seems hard to make interacting with proto maps nice.
>
> Chun-Hung Hsiao wrote:
> I'll do `ProfileInfo& profileInfo_ = ...`.
BTW the deference operator (`*`) applies on `mutable_profiles()` not `[]`.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65594/#review201627
-----------------------------------------------------------
On April 12, 2018, 3:35 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> -----------------------------------------------------------
>
> (Updated April 12, 2018, 3:35 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
>
>
> Repository: mesos
>
>
> Description
> -------
>
> SLRP now checkpoints profiles associated with storage pools, and does
> not depend on the `DiskProfileAdaptor` module to return the set of
> previously-known profiles during recovery.
>
>
> Diffs
> -----
>
> src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b
> src/resource_provider/storage/provider.cpp a07620d1c4bf618f669575b3e183bf598da905a6
>
>
> Diff: https://reviews.apache.org/r/65594/diff/8/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 65594: Checkpointed profiles in storage local
resource provider.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65594/#review201627
-----------------------------------------------------------
src/resource_provider/storage/provider.cpp
Lines 1003 (patched)
<https://reviews.apache.org/r/65594/#comment283021>
Even thought an `auto` might make sense in such a place _in general_, due to the interface of `MapInfo` it leads to pretty dense code and maybe repeating a type here might make this easier to read. How about
// NOTE: this form requires a `using` decl above.
using Profile =
MapPair<string, ResourceProviderState::Storage::ProfileInfo>;
foreach (const Profile& profile, storage.profiles()) {
profileInfos.put(
profile.first,
{profile.second.capability(), profile.second.parameters()});
}
src/resource_provider/storage/provider.cpp
Line 1010 (original), 1011 (patched)
<https://reviews.apache.org/r/65594/#comment283022>
Nit: _pending operations_ should be specific enough as this list is neither exhaustive today nor will remain in the future.
src/resource_provider/storage/provider.cpp
Line 1020 (original), 1013 (patched)
<https://reviews.apache.org/r/65594/#comment283027>
Let's add a comment pointing out that this matches what is done and documented in some more detail in `checkpointResourceProviderState`.
src/resource_provider/storage/provider.cpp
Line 1021 (original), 1014 (patched)
<https://reviews.apache.org/r/65594/#comment283026>
Both `disk` and `profile` are `optional` fields. We should check explicitly whether they are set (otherwise we might at some point e.g., hit a failure where the profile `` (empty string) was not found.
src/resource_provider/storage/provider.cpp
Lines 3271 (patched)
<https://reviews.apache.org/r/65594/#comment283034>
This invariant seems to be pretty non-local :(
src/resource_provider/storage/provider.cpp
Lines 3276 (patched)
<https://reviews.apache.org/r/65594/#comment283029>
Should we capture this?
ProfileInfo& profile_ = *(storage->mutable_profiles())[profile];
No strong opinion, it seems hard to make interacting with proto maps nice.
- Benjamin Bannier
On April 12, 2018, 5:35 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> -----------------------------------------------------------
>
> (Updated April 12, 2018, 5:35 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
>
>
> Repository: mesos
>
>
> Description
> -------
>
> SLRP now checkpoints profiles associated with storage pools, and does
> not depend on the `DiskProfileAdaptor` module to return the set of
> previously-known profiles during recovery.
>
>
> Diffs
> -----
>
> src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b
> src/resource_provider/storage/provider.cpp a07620d1c4bf618f669575b3e183bf598da905a6
>
>
> Diff: https://reviews.apache.org/r/65594/diff/8/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 65594: Checkpointed profiles in storage local
resource provider.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65594/#review201143
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On April 12, 2018, 3:35 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> -----------------------------------------------------------
>
> (Updated April 12, 2018, 3:35 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
>
>
> Repository: mesos
>
>
> Description
> -------
>
> SLRP now checkpoints profiles associated with storage pools, and does
> not depend on the `DiskProfileAdaptor` module to return the set of
> previously-known profiles during recovery.
>
>
> Diffs
> -----
>
> src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b
> src/resource_provider/storage/provider.cpp a07620d1c4bf618f669575b3e183bf598da905a6
>
>
> Diff: https://reviews.apache.org/r/65594/diff/8/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 65594: Checkpointed profiles in storage local
resource provider.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65594/
-----------------------------------------------------------
(Updated April 24, 2018, 1:06 a.m.)
Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
Changes
-------
Addressed Benjamin's comments.
Bugs: MESOS-8492
https://issues.apache.org/jira/browse/MESOS-8492
Repository: mesos
Description
-------
SLRP now checkpoints profiles associated with storage pools, and does
not depend on the `DiskProfileAdaptor` module to return the set of
previously-known profiles during recovery.
Diffs (updated)
-----
src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b
src/resource_provider/storage/provider.cpp 8ca2d3a98858940e6e027becefb53c2f00b4ae43
Diff: https://reviews.apache.org/r/65594/diff/9/
Changes: https://reviews.apache.org/r/65594/diff/8-9/
Testing
-------
sudo make check
Thanks,
Chun-Hung Hsiao