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 <be...@mesosphere.io> on 2017/01/06 15:16:14 UTC
Review Request 55269: Avoided use of CHECK macros.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55269/
-----------------------------------------------------------
Review request for mesos and Kapil Arya.
Bugs: MESOS-6860
https://issues.apache.org/jira/browse/MESOS-6860
Repository: mesos
Description
-------
Avoided use of CHECK macros.
Diffs
-----
src/tests/command_executor_tests.cpp f4e7044b82e8e81d6430551dc0b2a288db10bc3c
src/tests/container_logger_tests.cpp a8bc83449816ec8ba3f2b7e02594e4abe39c265d
src/tests/containerizer/cgroups_tests.cpp 0afaec6ae948cabf1472bf01103210d8f9809cb1
src/tests/containerizer/port_mapping_tests.cpp 207742f165e2c91a917ec293689c9030931f29db
src/tests/cram_md5_authentication_tests.cpp 1f414282b8d2aba7403f8617a30fa15c4a694d02
src/tests/credentials_tests.cpp ed50609cc5ed1937d2bcd782451245e0b5889ed5
src/tests/disk_quota_tests.cpp 90c42b301d749e7a9c6d6d42a51e929ec66d761c
src/tests/fetcher_cache_tests.cpp 03e817d46194d451eceb70f4cebb54dfdcb4c2e7
src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b
src/tests/partition_tests.cpp 3afdbba13a9753edce17b24ff6d1898d3cc6de62
src/tests/paths_tests.cpp a8aa5c26db7bd21f54eabaddf13a86582abaa2c3
src/tests/persistent_volume_endpoints_tests.cpp 1d40380dedbd6f0e1402b9a37d8a231cf73881f7
src/tests/persistent_volume_tests.cpp 8198b6b5ad323d17835ba067c7ff3d34ef948125
src/tests/reservation_endpoints_tests.cpp d94fe29e5972e6ed0011ca00cc56fe1c20cda495
src/tests/slave_tests.cpp d633a74d6b342239fbca0b44eec281eb315df5ff
Diff: https://reviews.apache.org/r/55269/diff/
Testing
-------
Tested on various Linux configurations in internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 55269: Avoided use of CHECK macros.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Jan. 7, 2017, 12:28 a.m., Kapil Arya wrote:
> > LGTM. Have you run any tests where the existing `CHECK_SOME` would fail and the new code will behave properly? It might be worth running it at least for one test. Maybe, in one of the containerizer value, just set it to `Error()` to validate the idea.
>
> Benjamin Bannier wrote:
> I tested this in internal CI on an incompletly configured machine; after this fix I was able to see failures in e.g., from `PortMappingIsolatorTest` where previously the code would have just aborted. As a test I also redefined to be just `return Error("")` which did not lead to aborts of the test run via `HTTPCommandExecutorTest.TerminateWithACK` or `ContainerLoggerTest.DefaultToSandbox` anymore.
Duh,
> I tested this in internal CI on an incompletly configured machine; after this fix I was able to see failures from e.g., PortMappingIsolatorTest where previously the code would have just aborted. As a test I also redefined `MesosContainerizer::create` to be just return Error("") which did not lead to aborts of the test run via HTTPCommandExecutorTest.TerminateWithACK or ContainerLoggerTest.DefaultToSandbox anymore.
FTFY, sorry.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55269/#review160788
-----------------------------------------------------------
On Jan. 10, 2017, 2:19 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55269/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2017, 2:19 p.m.)
>
>
> Review request for mesos and Kapil Arya.
>
>
> Bugs: MESOS-6860
> https://issues.apache.org/jira/browse/MESOS-6860
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Avoided use of CHECK macros.
>
>
> Diffs
> -----
>
> src/tests/command_executor_tests.cpp f4e7044b82e8e81d6430551dc0b2a288db10bc3c
> src/tests/container_logger_tests.cpp 7ac83338b5944967d0cbe768bf622c654fee99e1
> src/tests/containerizer/cgroups_tests.cpp 0afaec6ae948cabf1472bf01103210d8f9809cb1
> src/tests/containerizer/port_mapping_tests.cpp 207742f165e2c91a917ec293689c9030931f29db
> src/tests/cram_md5_authentication_tests.cpp 1f414282b8d2aba7403f8617a30fa15c4a694d02
> src/tests/credentials_tests.cpp ed50609cc5ed1937d2bcd782451245e0b5889ed5
> src/tests/disk_quota_tests.cpp 90c42b301d749e7a9c6d6d42a51e929ec66d761c
> src/tests/fetcher_cache_tests.cpp 03e817d46194d451eceb70f4cebb54dfdcb4c2e7
> src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b
> src/tests/partition_tests.cpp 72013d1bfee275c6f3cb90173f0c408d55e0bc5d
> src/tests/paths_tests.cpp a8aa5c26db7bd21f54eabaddf13a86582abaa2c3
> src/tests/persistent_volume_endpoints_tests.cpp 1d40380dedbd6f0e1402b9a37d8a231cf73881f7
> src/tests/persistent_volume_tests.cpp 8198b6b5ad323d17835ba067c7ff3d34ef948125
> src/tests/reservation_endpoints_tests.cpp d94fe29e5972e6ed0011ca00cc56fe1c20cda495
> src/tests/slave_tests.cpp 152c53ff102a081ce5c3b74984720fda8b791811
>
> Diff: https://reviews.apache.org/r/55269/diff/
>
>
> Testing
> -------
>
> Tested on various Linux configurations in internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55269: Avoided use of CHECK macros.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Jan. 7, 2017, 12:28 a.m., Kapil Arya wrote:
> > LGTM. Have you run any tests where the existing `CHECK_SOME` would fail and the new code will behave properly? It might be worth running it at least for one test. Maybe, in one of the containerizer value, just set it to `Error()` to validate the idea.
I tested this in internal CI on an incompletly configured machine; after this fix I was able to see failures in e.g., from `PortMappingIsolatorTest` where previously the code would have just aborted. As a test I also redefined to be just `return Error("")` which did not lead to aborts of the test run via `HTTPCommandExecutorTest.TerminateWithACK` or `ContainerLoggerTest.DefaultToSandbox` anymore.
> On Jan. 7, 2017, 12:28 a.m., Kapil Arya wrote:
> > src/tests/master_quota_tests.cpp, lines 92-94
> > <https://reviews.apache.org/r/55269/diff/1/?file=1598747#file1598747line92>
> >
> > Should we put it in a separate RR?
I felt this was just a refactoring similar to the instances of `return Error(...)` I added elsewhere in this patch, so I'd say no. Would you feel this would warrant its own patch?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55269/#review160788
-----------------------------------------------------------
On Jan. 6, 2017, 4:16 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55269/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2017, 4:16 p.m.)
>
>
> Review request for mesos and Kapil Arya.
>
>
> Bugs: MESOS-6860
> https://issues.apache.org/jira/browse/MESOS-6860
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Avoided use of CHECK macros.
>
>
> Diffs
> -----
>
> src/tests/command_executor_tests.cpp f4e7044b82e8e81d6430551dc0b2a288db10bc3c
> src/tests/container_logger_tests.cpp a8bc83449816ec8ba3f2b7e02594e4abe39c265d
> src/tests/containerizer/cgroups_tests.cpp 0afaec6ae948cabf1472bf01103210d8f9809cb1
> src/tests/containerizer/port_mapping_tests.cpp 207742f165e2c91a917ec293689c9030931f29db
> src/tests/cram_md5_authentication_tests.cpp 1f414282b8d2aba7403f8617a30fa15c4a694d02
> src/tests/credentials_tests.cpp ed50609cc5ed1937d2bcd782451245e0b5889ed5
> src/tests/disk_quota_tests.cpp 90c42b301d749e7a9c6d6d42a51e929ec66d761c
> src/tests/fetcher_cache_tests.cpp 03e817d46194d451eceb70f4cebb54dfdcb4c2e7
> src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b
> src/tests/partition_tests.cpp 3afdbba13a9753edce17b24ff6d1898d3cc6de62
> src/tests/paths_tests.cpp a8aa5c26db7bd21f54eabaddf13a86582abaa2c3
> src/tests/persistent_volume_endpoints_tests.cpp 1d40380dedbd6f0e1402b9a37d8a231cf73881f7
> src/tests/persistent_volume_tests.cpp 8198b6b5ad323d17835ba067c7ff3d34ef948125
> src/tests/reservation_endpoints_tests.cpp d94fe29e5972e6ed0011ca00cc56fe1c20cda495
> src/tests/slave_tests.cpp d633a74d6b342239fbca0b44eec281eb315df5ff
>
> Diff: https://reviews.apache.org/r/55269/diff/
>
>
> Testing
> -------
>
> Tested on various Linux configurations in internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55269: Avoided use of CHECK macros.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55269/#review160788
-----------------------------------------------------------
LGTM. Have you run any tests where the existing `CHECK_SOME` would fail and the new code will behave properly? It might be worth running it at least for one test. Maybe, in one of the containerizer value, just set it to `Error()` to validate the idea.
src/tests/master_quota_tests.cpp (lines 92 - 94)
<https://reviews.apache.org/r/55269/#comment232003>
Should we put it in a separate RR?
- Kapil Arya
On Jan. 6, 2017, 10:16 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55269/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2017, 10:16 a.m.)
>
>
> Review request for mesos and Kapil Arya.
>
>
> Bugs: MESOS-6860
> https://issues.apache.org/jira/browse/MESOS-6860
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Avoided use of CHECK macros.
>
>
> Diffs
> -----
>
> src/tests/command_executor_tests.cpp f4e7044b82e8e81d6430551dc0b2a288db10bc3c
> src/tests/container_logger_tests.cpp a8bc83449816ec8ba3f2b7e02594e4abe39c265d
> src/tests/containerizer/cgroups_tests.cpp 0afaec6ae948cabf1472bf01103210d8f9809cb1
> src/tests/containerizer/port_mapping_tests.cpp 207742f165e2c91a917ec293689c9030931f29db
> src/tests/cram_md5_authentication_tests.cpp 1f414282b8d2aba7403f8617a30fa15c4a694d02
> src/tests/credentials_tests.cpp ed50609cc5ed1937d2bcd782451245e0b5889ed5
> src/tests/disk_quota_tests.cpp 90c42b301d749e7a9c6d6d42a51e929ec66d761c
> src/tests/fetcher_cache_tests.cpp 03e817d46194d451eceb70f4cebb54dfdcb4c2e7
> src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b
> src/tests/partition_tests.cpp 3afdbba13a9753edce17b24ff6d1898d3cc6de62
> src/tests/paths_tests.cpp a8aa5c26db7bd21f54eabaddf13a86582abaa2c3
> src/tests/persistent_volume_endpoints_tests.cpp 1d40380dedbd6f0e1402b9a37d8a231cf73881f7
> src/tests/persistent_volume_tests.cpp 8198b6b5ad323d17835ba067c7ff3d34ef948125
> src/tests/reservation_endpoints_tests.cpp d94fe29e5972e6ed0011ca00cc56fe1c20cda495
> src/tests/slave_tests.cpp d633a74d6b342239fbca0b44eec281eb315df5ff
>
> Diff: https://reviews.apache.org/r/55269/diff/
>
>
> Testing
> -------
>
> Tested on various Linux configurations in internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55269: Avoided use of CHECK macros.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55269/
-----------------------------------------------------------
(Updated Jan. 10, 2017, 2:19 p.m.)
Review request for mesos and Kapil Arya.
Changes
-------
Rebased.
Bugs: MESOS-6860
https://issues.apache.org/jira/browse/MESOS-6860
Repository: mesos
Description
-------
Avoided use of CHECK macros.
Diffs (updated)
-----
src/tests/command_executor_tests.cpp f4e7044b82e8e81d6430551dc0b2a288db10bc3c
src/tests/container_logger_tests.cpp 7ac83338b5944967d0cbe768bf622c654fee99e1
src/tests/containerizer/cgroups_tests.cpp 0afaec6ae948cabf1472bf01103210d8f9809cb1
src/tests/containerizer/port_mapping_tests.cpp 207742f165e2c91a917ec293689c9030931f29db
src/tests/cram_md5_authentication_tests.cpp 1f414282b8d2aba7403f8617a30fa15c4a694d02
src/tests/credentials_tests.cpp ed50609cc5ed1937d2bcd782451245e0b5889ed5
src/tests/disk_quota_tests.cpp 90c42b301d749e7a9c6d6d42a51e929ec66d761c
src/tests/fetcher_cache_tests.cpp 03e817d46194d451eceb70f4cebb54dfdcb4c2e7
src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b
src/tests/partition_tests.cpp 72013d1bfee275c6f3cb90173f0c408d55e0bc5d
src/tests/paths_tests.cpp a8aa5c26db7bd21f54eabaddf13a86582abaa2c3
src/tests/persistent_volume_endpoints_tests.cpp 1d40380dedbd6f0e1402b9a37d8a231cf73881f7
src/tests/persistent_volume_tests.cpp 8198b6b5ad323d17835ba067c7ff3d34ef948125
src/tests/reservation_endpoints_tests.cpp d94fe29e5972e6ed0011ca00cc56fe1c20cda495
src/tests/slave_tests.cpp 152c53ff102a081ce5c3b74984720fda8b791811
Diff: https://reviews.apache.org/r/55269/diff/
Testing
-------
Tested on various Linux configurations in internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 55269: Avoided use of CHECK macros.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55269/#review160728
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55268, 55269]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 6, 2017, 3:16 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55269/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2017, 3:16 p.m.)
>
>
> Review request for mesos and Kapil Arya.
>
>
> Bugs: MESOS-6860
> https://issues.apache.org/jira/browse/MESOS-6860
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Avoided use of CHECK macros.
>
>
> Diffs
> -----
>
> src/tests/command_executor_tests.cpp f4e7044b82e8e81d6430551dc0b2a288db10bc3c
> src/tests/container_logger_tests.cpp a8bc83449816ec8ba3f2b7e02594e4abe39c265d
> src/tests/containerizer/cgroups_tests.cpp 0afaec6ae948cabf1472bf01103210d8f9809cb1
> src/tests/containerizer/port_mapping_tests.cpp 207742f165e2c91a917ec293689c9030931f29db
> src/tests/cram_md5_authentication_tests.cpp 1f414282b8d2aba7403f8617a30fa15c4a694d02
> src/tests/credentials_tests.cpp ed50609cc5ed1937d2bcd782451245e0b5889ed5
> src/tests/disk_quota_tests.cpp 90c42b301d749e7a9c6d6d42a51e929ec66d761c
> src/tests/fetcher_cache_tests.cpp 03e817d46194d451eceb70f4cebb54dfdcb4c2e7
> src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b
> src/tests/partition_tests.cpp 3afdbba13a9753edce17b24ff6d1898d3cc6de62
> src/tests/paths_tests.cpp a8aa5c26db7bd21f54eabaddf13a86582abaa2c3
> src/tests/persistent_volume_endpoints_tests.cpp 1d40380dedbd6f0e1402b9a37d8a231cf73881f7
> src/tests/persistent_volume_tests.cpp 8198b6b5ad323d17835ba067c7ff3d34ef948125
> src/tests/reservation_endpoints_tests.cpp d94fe29e5972e6ed0011ca00cc56fe1c20cda495
> src/tests/slave_tests.cpp d633a74d6b342239fbca0b44eec281eb315df5ff
>
> Diff: https://reviews.apache.org/r/55269/diff/
>
>
> Testing
> -------
>
> Tested on various Linux configurations in internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>