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/06/20 05:32:36 UTC
Review Request 67663: Made `UriDiskProfileAdaptor` be able to handle
disappeared profiles.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67663/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
Bugs: MESOS-8825
https://issues.apache.org/jira/browse/MESOS-8825
Repository: mesos
Description
-------
Now the `UriDiskProfileAdaptor` module can handle disappeared profiles
when polling the profile matrix, and performs a best-effort immutability
check for reappeared profiles.
Diffs
-----
src/resource_provider/storage/uri_disk_profile_adaptor.hpp 0484933b42d0bd66c689b06cb48f492eef7bc606
src/resource_provider/storage/uri_disk_profile_adaptor.cpp 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7
src/tests/disk_profile_adaptor_tests.cpp 4485f1635f484ce6e1c7c532eedb277f5eee118b
Diff: https://reviews.apache.org/r/67663/diff/1/
Testing
-------
sudo make check
A end-to-end test will be added later in the chain.
Thanks,
Chun-Hung Hsiao
Re: Review Request 67663: Made `UriDiskProfileAdaptor` be able to
handle disappeared profiles.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67663/
-----------------------------------------------------------
(Updated July 10, 2018, 8:26 p.m.)
Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
Changes
-------
Addressed Benjamin's comments.
Bugs: MESOS-8825
https://issues.apache.org/jira/browse/MESOS-8825
Repository: mesos
Description
-------
Now the `UriDiskProfileAdaptor` module can handle disappeared profiles
when polling the profile matrix, and performs a best-effort immutability
check for reappeared profiles.
Diffs (updated)
-----
src/resource_provider/storage/uri_disk_profile_adaptor.hpp 0484933b42d0bd66c689b06cb48f492eef7bc606
src/resource_provider/storage/uri_disk_profile_adaptor.cpp 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7
src/tests/disk_profile_adaptor_tests.cpp 4485f1635f484ce6e1c7c532eedb277f5eee118b
Diff: https://reviews.apache.org/r/67663/diff/3/
Changes: https://reviews.apache.org/r/67663/diff/2-3/
Testing
-------
sudo make check
A end-to-end test will be added later in the chain.
Thanks,
Chun-Hung Hsiao
Re: Review Request 67663: Made `UriDiskProfileAdaptor` be able to
handle disappeared profiles.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On July 10, 2018, 4:02 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/uri_disk_profile_adaptor.cpp
> > Line 303 (original), 303 (patched)
> > <https://reviews.apache.org/r/67663/diff/2/?file=2043782#file2043782line322>
> >
> > We have already updated the profiles and can log `profileMatrix.size()` here.
`profileMatrix.size()` is the number of total (active + inactive) profiles, not the number of active profiles. Dropping.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67663/#review205914
-----------------------------------------------------------
On June 21, 2018, 4:27 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67663/
> -----------------------------------------------------------
>
> (Updated June 21, 2018, 4:27 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Now the `UriDiskProfileAdaptor` module can handle disappeared profiles
> when polling the profile matrix, and performs a best-effort immutability
> check for reappeared profiles.
>
>
> Diffs
> -----
>
> src/resource_provider/storage/uri_disk_profile_adaptor.hpp 0484933b42d0bd66c689b06cb48f492eef7bc606
> src/resource_provider/storage/uri_disk_profile_adaptor.cpp 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7
> src/tests/disk_profile_adaptor_tests.cpp 4485f1635f484ce6e1c7c532eedb277f5eee118b
>
>
> Diff: https://reviews.apache.org/r/67663/diff/2/
>
>
> Testing
> -------
>
> sudo make check
>
> A end-to-end test will be added later in the chain.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67663: Made `UriDiskProfileAdaptor` be able to
handle disappeared profiles.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On July 10, 2018, 4:02 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/uri_disk_profile_adaptor.cpp
> > Line 248 (original), 244 (patched)
> > <https://reviews.apache.org/r/67663/diff/2/?file=2043782#file2043782line256>
> >
> > `foreachpair` would give us a way to give better names to `first` and `second`.
`foreachpair` does not work for `google::protobuf::Map` hence `foreach`. Dropping.
> On July 10, 2018, 4:02 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/uri_disk_profile_adaptor.cpp
> > Line 292 (original), 281 (patched)
> > <https://reviews.apache.org/r/67663/diff/2/?file=2043782#file2043782line300>
> >
> > Use `contains` here.
`parsed.profile_matrix()` is a `google::protobuf::Map` hence no `contains`. Dropping.
> On July 10, 2018, 4:02 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/uri_disk_profile_adaptor.cpp
> > Lines 291 (patched)
> > <https://reviews.apache.org/r/67663/diff/2/?file=2043782#file2043782line310>
> >
> > `foreachpair` would give us a way to give better names to `first` and `second`.
`foreachpair` does not work for `google::protobuf::Map` hence `foreach`. Dropping.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67663/#review205914
-----------------------------------------------------------
On June 21, 2018, 4:27 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67663/
> -----------------------------------------------------------
>
> (Updated June 21, 2018, 4:27 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Now the `UriDiskProfileAdaptor` module can handle disappeared profiles
> when polling the profile matrix, and performs a best-effort immutability
> check for reappeared profiles.
>
>
> Diffs
> -----
>
> src/resource_provider/storage/uri_disk_profile_adaptor.hpp 0484933b42d0bd66c689b06cb48f492eef7bc606
> src/resource_provider/storage/uri_disk_profile_adaptor.cpp 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7
> src/tests/disk_profile_adaptor_tests.cpp 4485f1635f484ce6e1c7c532eedb277f5eee118b
>
>
> Diff: https://reviews.apache.org/r/67663/diff/2/
>
>
> Testing
> -------
>
> sudo make check
>
> A end-to-end test will be added later in the chain.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67663: Made `UriDiskProfileAdaptor` be able to
handle disappeared profiles.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67663/#review205914
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
src/resource_provider/storage/uri_disk_profile_adaptor.cpp
Line 248 (original), 244 (patched)
<https://reviews.apache.org/r/67663/#comment288843>
`foreachpair` would give us a way to give better names to `first` and `second`.
src/resource_provider/storage/uri_disk_profile_adaptor.cpp
Line 292 (original), 281 (patched)
<https://reviews.apache.org/r/67663/#comment288840>
Use `contains` here.
src/resource_provider/storage/uri_disk_profile_adaptor.cpp
Lines 284 (patched)
<https://reviews.apache.org/r/67663/#comment288844>
Is this something actionable to warn about or in reality just `INFO`?
src/resource_provider/storage/uri_disk_profile_adaptor.cpp
Lines 291 (patched)
<https://reviews.apache.org/r/67663/#comment288842>
`foreachpair` would give us a way to give better names to `first` and `second`.
src/resource_provider/storage/uri_disk_profile_adaptor.cpp
Line 303 (original), 303 (patched)
<https://reviews.apache.org/r/67663/#comment288841>
We have already updated the profiles and can log `profileMatrix.size()` here.
- Benjamin Bannier
On June 21, 2018, 6:27 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67663/
> -----------------------------------------------------------
>
> (Updated June 21, 2018, 6:27 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Now the `UriDiskProfileAdaptor` module can handle disappeared profiles
> when polling the profile matrix, and performs a best-effort immutability
> check for reappeared profiles.
>
>
> Diffs
> -----
>
> src/resource_provider/storage/uri_disk_profile_adaptor.hpp 0484933b42d0bd66c689b06cb48f492eef7bc606
> src/resource_provider/storage/uri_disk_profile_adaptor.cpp 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7
> src/tests/disk_profile_adaptor_tests.cpp 4485f1635f484ce6e1c7c532eedb277f5eee118b
>
>
> Diff: https://reviews.apache.org/r/67663/diff/2/
>
>
> Testing
> -------
>
> sudo make check
>
> A end-to-end test will be added later in the chain.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67663: Made `UriDiskProfileAdaptor` be able to
handle disappeared profiles.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67663/
-----------------------------------------------------------
(Updated June 21, 2018, 4:27 a.m.)
Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
Changes
-------
Addressed Benjamin's comments.
Bugs: MESOS-8825
https://issues.apache.org/jira/browse/MESOS-8825
Repository: mesos
Description
-------
Now the `UriDiskProfileAdaptor` module can handle disappeared profiles
when polling the profile matrix, and performs a best-effort immutability
check for reappeared profiles.
Diffs (updated)
-----
src/resource_provider/storage/uri_disk_profile_adaptor.hpp 0484933b42d0bd66c689b06cb48f492eef7bc606
src/resource_provider/storage/uri_disk_profile_adaptor.cpp 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7
src/tests/disk_profile_adaptor_tests.cpp 4485f1635f484ce6e1c7c532eedb277f5eee118b
Diff: https://reviews.apache.org/r/67663/diff/2/
Changes: https://reviews.apache.org/r/67663/diff/1-2/
Testing
-------
sudo make check
A end-to-end test will be added later in the chain.
Thanks,
Chun-Hung Hsiao
Re: Review Request 67663: Made `UriDiskProfileAdaptor` be able to
handle disappeared profiles.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On June 20, 2018, 1:54 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/uri_disk_profile_adaptor.hpp
> > Lines 242 (patched)
> > <https://reviews.apache.org/r/67663/diff/1/?file=2042676#file2042676line243>
> >
> > Is this field needed for a later change? It looks like it should be fine to just remove entries from `profileMatrix` and then drive logic with e.g., `contains` checks.
>
> Chun-Hung Hsiao wrote:
> The profile data is kept in `profileMatrix` even for inactive profiles because I'd like to have a best-effort check on the profile immutability, i.e., if the profile is removed and added back, its capability and parameters should remain the same.
>
> There's a related TODO to consider checkpointing `profileMatrix` to make the check more robust.
In fact, the following unit test is changed to test this: "profile" is missing in the second poll, but appeared in the third one (with a mutated capability) and the fourth one (with the same capability). The third poll is ignored but the fourth one is taken.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67663/#review205073
-----------------------------------------------------------
On June 20, 2018, 5:32 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67663/
> -----------------------------------------------------------
>
> (Updated June 20, 2018, 5:32 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Now the `UriDiskProfileAdaptor` module can handle disappeared profiles
> when polling the profile matrix, and performs a best-effort immutability
> check for reappeared profiles.
>
>
> Diffs
> -----
>
> src/resource_provider/storage/uri_disk_profile_adaptor.hpp 0484933b42d0bd66c689b06cb48f492eef7bc606
> src/resource_provider/storage/uri_disk_profile_adaptor.cpp 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7
> src/tests/disk_profile_adaptor_tests.cpp 4485f1635f484ce6e1c7c532eedb277f5eee118b
>
>
> Diff: https://reviews.apache.org/r/67663/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
> A end-to-end test will be added later in the chain.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67663: Made `UriDiskProfileAdaptor` be able to
handle disappeared profiles.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On June 20, 2018, 1:54 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/uri_disk_profile_adaptor.hpp
> > Lines 242 (patched)
> > <https://reviews.apache.org/r/67663/diff/1/?file=2042676#file2042676line243>
> >
> > Is this field needed for a later change? It looks like it should be fine to just remove entries from `profileMatrix` and then drive logic with e.g., `contains` checks.
The profile data is kept in `profileMatrix` even for inactive profiles because I'd like to have a best-effort check on the profile immutability, i.e., if the profile is removed and added back, its capability and parameters should remain the same.
There's a related TODO to consider checkpointing `profileMatrix` to make the check more robust.
> On June 20, 2018, 1:54 p.m., Benjamin Bannier wrote:
> > src/tests/disk_profile_adaptor_tests.cpp
> > Line 687 (original), 689 (patched)
> > <https://reviews.apache.org/r/67663/diff/1/?file=2042678#file2042678line703>
> >
> > Not added here, but this is redundant since we always `resume` the clock when tearing down a test.
Yeah I am aware of this, but some people seems to prefer explicitly resuming the clock still, and that's why I kept it is as. Personally I prefer shorter test body so I'll remove this.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67663/#review205073
-----------------------------------------------------------
On June 20, 2018, 5:32 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67663/
> -----------------------------------------------------------
>
> (Updated June 20, 2018, 5:32 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Now the `UriDiskProfileAdaptor` module can handle disappeared profiles
> when polling the profile matrix, and performs a best-effort immutability
> check for reappeared profiles.
>
>
> Diffs
> -----
>
> src/resource_provider/storage/uri_disk_profile_adaptor.hpp 0484933b42d0bd66c689b06cb48f492eef7bc606
> src/resource_provider/storage/uri_disk_profile_adaptor.cpp 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7
> src/tests/disk_profile_adaptor_tests.cpp 4485f1635f484ce6e1c7c532eedb277f5eee118b
>
>
> Diff: https://reviews.apache.org/r/67663/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
> A end-to-end test will be added later in the chain.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67663: Made `UriDiskProfileAdaptor` be able to
handle disappeared profiles.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On June 20, 2018, 1:54 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/uri_disk_profile_adaptor.hpp
> > Lines 242 (patched)
> > <https://reviews.apache.org/r/67663/diff/1/?file=2042676#file2042676line243>
> >
> > Is this field needed for a later change? It looks like it should be fine to just remove entries from `profileMatrix` and then drive logic with e.g., `contains` checks.
>
> Chun-Hung Hsiao wrote:
> The profile data is kept in `profileMatrix` even for inactive profiles because I'd like to have a best-effort check on the profile immutability, i.e., if the profile is removed and added back, its capability and parameters should remain the same.
>
> There's a related TODO to consider checkpointing `profileMatrix` to make the check more robust.
>
> Chun-Hung Hsiao wrote:
> In fact, the following unit test is changed to test this: "profile" is missing in the second poll, but appeared in the third one (with a mutated capability) and the fourth one (with the same capability). The third poll is ignored but the fourth one is taken.
Dropping this as this is required. Please reopen if you think otherwise.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67663/#review205073
-----------------------------------------------------------
On June 21, 2018, 4:27 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67663/
> -----------------------------------------------------------
>
> (Updated June 21, 2018, 4:27 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Now the `UriDiskProfileAdaptor` module can handle disappeared profiles
> when polling the profile matrix, and performs a best-effort immutability
> check for reappeared profiles.
>
>
> Diffs
> -----
>
> src/resource_provider/storage/uri_disk_profile_adaptor.hpp 0484933b42d0bd66c689b06cb48f492eef7bc606
> src/resource_provider/storage/uri_disk_profile_adaptor.cpp 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7
> src/tests/disk_profile_adaptor_tests.cpp 4485f1635f484ce6e1c7c532eedb277f5eee118b
>
>
> Diff: https://reviews.apache.org/r/67663/diff/2/
>
>
> Testing
> -------
>
> sudo make check
>
> A end-to-end test will be added later in the chain.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67663: Made `UriDiskProfileAdaptor` be able to
handle disappeared profiles.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67663/#review205073
-----------------------------------------------------------
src/resource_provider/storage/uri_disk_profile_adaptor.hpp
Lines 242 (patched)
<https://reviews.apache.org/r/67663/#comment288028>
Is this field needed for a later change? It looks like it should be fine to just remove entries from `profileMatrix` and then drive logic with e.g., `contains` checks.
src/tests/disk_profile_adaptor_tests.cpp
Line 671 (original), 674 (patched)
<https://reviews.apache.org/r/67663/#comment288029>
Not changed here, but this `settle` seems to belong to the `ASSERT` below.
src/tests/disk_profile_adaptor_tests.cpp
Line 687 (original), 689 (patched)
<https://reviews.apache.org/r/67663/#comment288030>
Not added here, but this is redundant since we always `resume` the clock when tearing down a test.
- Benjamin Bannier
On June 20, 2018, 7:32 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67663/
> -----------------------------------------------------------
>
> (Updated June 20, 2018, 7:32 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Now the `UriDiskProfileAdaptor` module can handle disappeared profiles
> when polling the profile matrix, and performs a best-effort immutability
> check for reappeared profiles.
>
>
> Diffs
> -----
>
> src/resource_provider/storage/uri_disk_profile_adaptor.hpp 0484933b42d0bd66c689b06cb48f492eef7bc606
> src/resource_provider/storage/uri_disk_profile_adaptor.cpp 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7
> src/tests/disk_profile_adaptor_tests.cpp 4485f1635f484ce6e1c7c532eedb277f5eee118b
>
>
> Diff: https://reviews.apache.org/r/67663/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
> A end-to-end test will be added later in the chain.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>