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 2019/02/05 08:06:40 UTC
Review Request 69898: Updated `CreateDestroyDisk*` SLRP tests to test
pipelined operations.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69898/
-----------------------------------------------------------
Review request for mesos and Benjamin Bannier.
Bugs: MESOS-9544
https://issues.apache.org/jira/browse/MESOS-9544
Repository: mesos
Description
-------
This patch extends the code coverage of the `CreateDestroyDisk` and
`CreateDestroyDiskRecovery` tests by testing pipelined `RESERVE`,
`CREATE`, `DESTROY` and `UNRESERVE` operations along with `CREATE_DISK`
and `DESTROY_DISK`. It also renames `CreateDestroyDiskRecovery` to
`CreateDestroyDiskWithRecovery` for consistency.
Diffs
-----
src/tests/storage_local_resource_provider_tests.cpp e8ed20f818ed7f1a3ce15758ea3c366520443377
Diff: https://reviews.apache.org/r/69898/diff/1/
Testing
-------
`make check`
Run in repetition
Thanks,
Chun-Hung Hsiao
Re: Review Request 69898: Updated `CreateDestroyDisk*` SLRP tests to
test pipelined operations.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Feb. 12, 2019, 3:10 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Line 818 (original), 817 (patched)
> > <https://reviews.apache.org/r/69898/diff/2/?file=2124911#file2124911line838>
> >
> > Is this needed?
I believe so as there's no clock manipulation in this test and we use a very small allocation interval. Am I making a mistake?
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69898/#review212743
-----------------------------------------------------------
On Feb. 12, 2019, 5:26 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69898/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2019, 5:26 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch extends the code coverage of the `CreateDestroyDisk` and
> `CreateDestroyDiskRecovery` tests by testing pipelined `RESERVE`,
> `CREATE`, `DESTROY` and `UNRESERVE` operations along with `CREATE_DISK`
> and `DESTROY_DISK`. It also renames `CreateDestroyDiskRecovery` to
> `CreateDestroyDiskWithRecovery` for consistency.
>
>
> Diffs
> -----
>
> src/tests/storage_local_resource_provider_tests.cpp fb001aa8d32d1a0a03014a35772fe10b65ce8d9a
>
>
> Diff: https://reviews.apache.org/r/69898/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
> Run in repetition
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 69898: Updated `CreateDestroyDisk*` SLRP tests to
test pipelined operations.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 12, 2019, 4:10 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Line 818 (original), 817 (patched)
> > <https://reviews.apache.org/r/69898/diff/2/?file=2124911#file2124911line838>
> >
> > Is this needed?
>
> Chun-Hung Hsiao wrote:
> I believe so as there's no clock manipulation in this test and we use a very small allocation interval. Am I making a mistake?
The way the test is written the framework should never be offered the same resources twice, so whatever you set here has no effect on the test.
While it doesn't actively hurt, I feel it distracts a little from the meat of the test.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69898/#review212743
-----------------------------------------------------------
On Feb. 13, 2019, 10:24 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69898/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2019, 10:24 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-9565
> https://issues.apache.org/jira/browse/MESOS-9565
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch extends the code coverage of the `CreateDestroyDisk` and
> `CreateDestroyDiskRecovery` tests by testing pipelined `RESERVE`,
> `CREATE`, `DESTROY` and `UNRESERVE` operations along with `CREATE_DISK`
> and `DESTROY_DISK`. It also renames `CreateDestroyDiskRecovery` to
> `CreateDestroyDiskWithRecovery` for consistency.
>
>
> Diffs
> -----
>
> src/tests/storage_local_resource_provider_tests.cpp fb001aa8d32d1a0a03014a35772fe10b65ce8d9a
>
>
> Diff: https://reviews.apache.org/r/69898/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
> Run in repetition
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 69898: Updated `CreateDestroyDisk*` SLRP tests to
test pipelined operations.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Feb. 12, 2019, 3:10 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Line 818 (original), 817 (patched)
> > <https://reviews.apache.org/r/69898/diff/2/?file=2124911#file2124911line838>
> >
> > Is this needed?
>
> Chun-Hung Hsiao wrote:
> I believe so as there's no clock manipulation in this test and we use a very small allocation interval. Am I making a mistake?
>
> Benjamin Bannier wrote:
> The way the test is written the framework should never be offered the same resources twice, so whatever you set here has no effect on the test.
>
> While it doesn't actively hurt, I feel it distracts a little from the meat of the test.
Good point. Let me follow up with a patch to clean up these filters.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69898/#review212743
-----------------------------------------------------------
On Feb. 13, 2019, 9:24 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69898/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2019, 9:24 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-9565
> https://issues.apache.org/jira/browse/MESOS-9565
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch extends the code coverage of the `CreateDestroyDisk` and
> `CreateDestroyDiskRecovery` tests by testing pipelined `RESERVE`,
> `CREATE`, `DESTROY` and `UNRESERVE` operations along with `CREATE_DISK`
> and `DESTROY_DISK`. It also renames `CreateDestroyDiskRecovery` to
> `CreateDestroyDiskWithRecovery` for consistency.
>
>
> Diffs
> -----
>
> src/tests/storage_local_resource_provider_tests.cpp fb001aa8d32d1a0a03014a35772fe10b65ce8d9a
>
>
> Diff: https://reviews.apache.org/r/69898/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
> Run in repetition
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 69898: Updated `CreateDestroyDisk*` SLRP tests to
test pipelined operations.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69898/#review212743
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/storage_local_resource_provider_tests.cpp
Line 818 (original), 817 (patched)
<https://reviews.apache.org/r/69898/#comment298638>
Is this needed?
src/tests/storage_local_resource_provider_tests.cpp
Lines 879-880 (original), 873-874 (patched)
<https://reviews.apache.org/r/69898/#comment298639>
This is not symmetric with how this disk was created (`reserve`, then `create_disk`). If we decide to deliberately test a switched teardown order here we should probably add a comment.
src/tests/storage_local_resource_provider_tests.cpp
Lines 1024 (patched)
<https://reviews.apache.org/r/69898/#comment298640>
Does the first sentence document that _when we see the extra pool in offers, we have observed that the RP has seen a profile_? We should make the reason we need this a little crisper.
Also `s/setup/set up/`.
src/tests/storage_local_resource_provider_tests.cpp
Lines 1094-1095 (patched)
<https://reviews.apache.org/r/69898/#comment298641>
Could document ordering of `unreserve` and `destroy_disk` here.
- Benjamin Bannier
On Feb. 12, 2019, 6:26 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69898/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2019, 6:26 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch extends the code coverage of the `CreateDestroyDisk` and
> `CreateDestroyDiskRecovery` tests by testing pipelined `RESERVE`,
> `CREATE`, `DESTROY` and `UNRESERVE` operations along with `CREATE_DISK`
> and `DESTROY_DISK`. It also renames `CreateDestroyDiskRecovery` to
> `CreateDestroyDiskWithRecovery` for consistency.
>
>
> Diffs
> -----
>
> src/tests/storage_local_resource_provider_tests.cpp fb001aa8d32d1a0a03014a35772fe10b65ce8d9a
>
>
> Diff: https://reviews.apache.org/r/69898/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
> Run in repetition
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 69898: Updated `CreateDestroyDisk*` SLRP tests to
test pipelined operations.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69898/
-----------------------------------------------------------
(Updated Feb. 28, 2019, 12:20 a.m.)
Review request for mesos and Benjamin Bannier.
Changes
-------
Removed unnecessary filters.
Bugs: MESOS-9565
https://issues.apache.org/jira/browse/MESOS-9565
Repository: mesos
Description
-------
This patch extends the code coverage of the `CreateDestroyDisk` and
`CreateDestroyDiskRecovery` tests by testing pipelined `RESERVE`,
`CREATE`, `DESTROY` and `UNRESERVE` operations along with `CREATE_DISK`
and `DESTROY_DISK`. It also renames `CreateDestroyDiskRecovery` to
`CreateDestroyDiskWithRecovery` for consistency.
Diffs (updated)
-----
src/tests/storage_local_resource_provider_tests.cpp a661951a0a326cc342aa0c45dd0967692ae70941
Diff: https://reviews.apache.org/r/69898/diff/4/
Changes: https://reviews.apache.org/r/69898/diff/3-4/
Testing
-------
`make check`
Run in repetition
Thanks,
Chun-Hung Hsiao
Re: Review Request 69898: Updated `CreateDestroyDisk*` SLRP tests to
test pipelined operations.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69898/
-----------------------------------------------------------
(Updated Feb. 13, 2019, 9:24 p.m.)
Review request for mesos and Benjamin Bannier.
Changes
-------
Addressed Benjamin's comments and fixed a compilation error.
Bugs: MESOS-9565
https://issues.apache.org/jira/browse/MESOS-9565
Repository: mesos
Description
-------
This patch extends the code coverage of the `CreateDestroyDisk` and
`CreateDestroyDiskRecovery` tests by testing pipelined `RESERVE`,
`CREATE`, `DESTROY` and `UNRESERVE` operations along with `CREATE_DISK`
and `DESTROY_DISK`. It also renames `CreateDestroyDiskRecovery` to
`CreateDestroyDiskWithRecovery` for consistency.
Diffs (updated)
-----
src/tests/storage_local_resource_provider_tests.cpp fb001aa8d32d1a0a03014a35772fe10b65ce8d9a
Diff: https://reviews.apache.org/r/69898/diff/3/
Changes: https://reviews.apache.org/r/69898/diff/2-3/
Testing
-------
`make check`
Run in repetition
Thanks,
Chun-Hung Hsiao
Re: Review Request 69898: Updated `CreateDestroyDisk*` SLRP tests to
test pipelined operations.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69898/
-----------------------------------------------------------
(Updated Feb. 12, 2019, 5:26 a.m.)
Review request for mesos and Benjamin Bannier.
Changes
-------
Resolved a test flake more cleanly (IMO).
Bugs: MESOS-9544
https://issues.apache.org/jira/browse/MESOS-9544
Repository: mesos
Description
-------
This patch extends the code coverage of the `CreateDestroyDisk` and
`CreateDestroyDiskRecovery` tests by testing pipelined `RESERVE`,
`CREATE`, `DESTROY` and `UNRESERVE` operations along with `CREATE_DISK`
and `DESTROY_DISK`. It also renames `CreateDestroyDiskRecovery` to
`CreateDestroyDiskWithRecovery` for consistency.
Diffs (updated)
-----
src/tests/storage_local_resource_provider_tests.cpp fb001aa8d32d1a0a03014a35772fe10b65ce8d9a
Diff: https://reviews.apache.org/r/69898/diff/2/
Changes: https://reviews.apache.org/r/69898/diff/1-2/
Testing
-------
`make check`
Run in repetition
Thanks,
Chun-Hung Hsiao
Re: Review Request 69898: Updated `CreateDestroyDisk*` SLRP tests to
test pipelined operations.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69898/#review212543
-----------------------------------------------------------
PASS: Mesos patch 69898 was successfully built and tested.
Reviews applied: `['69858', '69866', '69892', '69893', '69894', '69895', '69896', '69897', '69898']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2851/mesos-review-69898
- Mesos Reviewbot Windows
On Feb. 5, 2019, 8:06 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69898/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2019, 8:06 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch extends the code coverage of the `CreateDestroyDisk` and
> `CreateDestroyDiskRecovery` tests by testing pipelined `RESERVE`,
> `CREATE`, `DESTROY` and `UNRESERVE` operations along with `CREATE_DISK`
> and `DESTROY_DISK`. It also renames `CreateDestroyDiskRecovery` to
> `CreateDestroyDiskWithRecovery` for consistency.
>
>
> Diffs
> -----
>
> src/tests/storage_local_resource_provider_tests.cpp e8ed20f818ed7f1a3ce15758ea3c366520443377
>
>
> Diff: https://reviews.apache.org/r/69898/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
> Run in repetition
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 69898: Updated `CreateDestroyDisk*` SLRP tests to
test pipelined operations.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69898/#review212587
-----------------------------------------------------------
Patch looks great!
Reviews applied: [69858, 69866, 69892, 69893, 69894, 69895, 69896, 69897, 69898]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 5, 2019, 8:06 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69898/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2019, 8:06 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch extends the code coverage of the `CreateDestroyDisk` and
> `CreateDestroyDiskRecovery` tests by testing pipelined `RESERVE`,
> `CREATE`, `DESTROY` and `UNRESERVE` operations along with `CREATE_DISK`
> and `DESTROY_DISK`. It also renames `CreateDestroyDiskRecovery` to
> `CreateDestroyDiskWithRecovery` for consistency.
>
>
> Diffs
> -----
>
> src/tests/storage_local_resource_provider_tests.cpp e8ed20f818ed7f1a3ce15758ea3c366520443377
>
>
> Diff: https://reviews.apache.org/r/69898/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
> Run in repetition
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>