You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Timothy Chen <tn...@apache.org> on 2014/09/12 06:31:13 UTC
Review Request 25569: Only perform docker validation once for tests
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Repository: mesos-git
Description
-------
Only perform docker validation once for tests
Diffs
-----
src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
Diff: https://reviews.apache.org/r/25569/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 25569: Only perform docker validation once for
tests
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review53262
-----------------------------------------------------------
Patch looks great!
Reviews applied: [25569]
All tests passed.
- Mesos ReviewBot
On Sept. 12, 2014, 4:31 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2014, 4:31 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Only perform docker validation once for tests
>
>
> Diffs
> -----
>
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 25569: Refactor test environment validations
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review53541
-----------------------------------------------------------
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93236>
vector<process::Owned<TestFilter>> testFilters;
we will eventually move process::Owned to unique_ptr so please start using it :)
- Dominic Hamon
On Sept. 16, 2014, 12:07 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2014, 12:07 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/25569
>
> Refactor how validation is done in the environment by defining a standard test filter interface, and only execute the validation once.
>
>
> Diffs
> -----
>
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 25569: Refactor test environment validations
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review53491
-----------------------------------------------------------
Bad patch!
Reviews applied: [25569]
Failed command: ./support/mesos-style.py
Error:
Checking 504 files using filter --filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/end_of_line,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/tab,+whitespace/todo
src/tests/environment.cpp:279: Lines should be <= 80 characters long [whitespace/line_length] [2]
Total errors found: 1
- Mesos ReviewBot
On Sept. 16, 2014, 7:07 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2014, 7:07 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/25569
>
> Refactor how validation is done in the environment by defining a standard test filter interface, and only execute the validation once.
>
>
> Diffs
> -----
>
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 25569: Refactor test environment validations
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review53893
-----------------------------------------------------------
Thanks Tim, this is a great step forward! Getting close!
Some slight suggestions to change the TestFilter design:
(1) I noticed that the `matches` method seems unncessary, since the caller only cares about filtering when it matches. So why not move the matching logic inside the test filter, rather than forcing the caller to call `matches` every time?
(2) The #ifdefs here are a bit confusing, sometimes the filters are always constructed but contain an #ifdef in the `filter` method, and sometimes the filters are both constructed in an #ifdef yet **still** contain an #ifdef in the `filter` method. How about the following to be consistent here: each filter is contained fully inside the relevant #ifdef, the construction of it is guarded by the same #ifdef.
(3) We tend to favor composition over inheritance for a number of reasons. Here is a suggestion to clean up the inheritance tree a bit. Rather than having TestFilter, TestNameFilter, and TestParamTypeFilter hierarchy. Could we get away with a single TestFilter parent type?
```
class TestFilter {
public:
virtual TestFilter() {}
virtual ~TestFilter() {}
virtual bool filter(const ::testing::TestInfo* test) const = 0;
// Returns whether the test name / parameterization matches the pattern.
static bool matches(const ::testing::TestInfo* test, const string pattern)
{
if (strings::contains(test->test_case_name(), pattern) ||
strings::contains(test->name(), pattern)) {
return true;
} else if (test->type_param() != NULL &&
strings::contains(test->type_param(), pattern)) {
return true;
}
return false;
}
};
```
Then all the filters inherit directly from `TestFilter` and can leverage the static `matches` to implement `filter`. This makes the filtering a bit more direct (no need to worry about the `passed` boolean. It also makes things a bit more flexible, because we no longer enforce only 1 pattern per filter (which meant that you needed 2 cgroup filters, one for test name, one for test parameterization).
(4) Given this, would love to see a single CgroupTestFilter and PortMappingTestFilter with this approach (instead of 1 per pattern)! :D
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93756>
Still need this?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93757>
Still need this?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93759>
Could we remove isRootUser? Looks like it won't be necessary with the other comments I've made (less calls needed).
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93760>
Could this be merged into the RootUserFilter within an #ifdef (per my design comments)?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93753>
Shouldn't the ':' be the responsibility of the caller, when joining the result of this call?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93755>
How about s/testFilter/filters/ ? :)
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93754>
Since this is a .cpp file, you can add a using clause for process::Owned so that you don't need the line wrapping here.
- Ben Mahler
On Sept. 16, 2014, 10:35 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2014, 10:35 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/25569
>
>
> Diffs
> -----
>
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 25569: Refactor test environment validations
Posted by Timothy Chen <tn...@apache.org>.
> On Sept. 22, 2014, 10:52 p.m., Ben Mahler wrote:
> > Did you look at the diff when you posted this review? Looks like you needed to rebase against master.
Sorry didn't really look at it and you're right it's not rebased!
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review54201
-----------------------------------------------------------
On Sept. 23, 2014, 5:28 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2014, 5:28 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/25569
>
>
> Diffs
> -----
>
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 25569: Refactor test environment validations
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review54201
-----------------------------------------------------------
Did you look at the diff when you posted this review? Looks like you needed to rebase against master.
- Ben Mahler
On Sept. 22, 2014, 9:17 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2014, 9:17 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/25569
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp 62df4b7645c6ab061a47634058d79ca849caa6b9
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 2187c296ea9b1a7de9ae3f09fdf1983f98a3d01b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 7164ecc0f068d4a72248521e3cbd345958efa880
> src/slave/containerizer/isolators/cgroups/mem.cpp b3d4a5daa90a842e501bc6be2f0cf20fe22906ac
> src/slave/containerizer/isolators/cgroups/perf_event.cpp 4ced508e600e13f3e5ae9d12ea199de743def652
> src/slave/containerizer/linux_launcher.cpp f7bc894830a7ca3f55465dacc7b653cdc2d7758b
> src/slave/slave.cpp 9a6646f0249fd43ae5d13bd9ee3aaaab5da08412
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 25569: Refactor test environment validations
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review54351
-----------------------------------------------------------
Ship it!
Thanks so much Tim, this is a great cleanup!
I'll fix the logical bugs and land this for you shortly.
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment94479>
Missing the include for <process/owned>
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment94487>
Looks like the double negatives were pretty tricky per the bug below, I'll rename this to 'disabled' to make it a bit clearer.
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment94488>
This is a bug! Should be ||, not &&
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment94489>
|| here as well
- Ben Mahler
On Sept. 23, 2014, 10:02 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2014, 10:02 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/25569
>
>
> Diffs
> -----
>
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 25569: Refactor test environment validations
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/
-----------------------------------------------------------
(Updated Sept. 23, 2014, 10:02 p.m.)
Review request for mesos and Ben Mahler.
Repository: mesos-git
Description
-------
Review: https://reviews.apache.org/r/25569
Diffs (updated)
-----
src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
Diff: https://reviews.apache.org/r/25569/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 25569: Refactor test environment validations
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/
-----------------------------------------------------------
(Updated Sept. 23, 2014, 9:24 p.m.)
Review request for mesos and Ben Mahler.
Repository: mesos-git
Description
-------
Review: https://reviews.apache.org/r/25569
Diffs (updated)
-----
src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
Diff: https://reviews.apache.org/r/25569/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 25569: Refactor test environment validations
Posted by Timothy Chen <tn...@apache.org>.
> On Sept. 23, 2014, 7:10 p.m., Ben Mahler wrote:
> > src/tests/environment.cpp, lines 57-59
> > <https://reviews.apache.org/r/25569/diff/7/?file=701944#file701944line57>
> >
> > Missing includes for these?
> >
> > #include <set>
> > #include <vector>
I think it's already included, but I'll add it in case.
> On Sept. 23, 2014, 7:10 p.m., Ben Mahler wrote:
> > src/tests/environment.cpp, lines 152-160
> > <https://reviews.apache.org/r/25569/diff/7/?file=701944#file701944line152>
> >
> > The other CgroupsParamTypeFilter checks for "root" user, but this one doesn't?
That's identical to what the original environment validation does.
> On Sept. 23, 2014, 7:10 p.m., Ben Mahler wrote:
> > src/tests/environment.cpp, line 210
> > <https://reviews.apache.org/r/25569/diff/7/?file=701944#file701944line210>
> >
> > Any reason to for the conversion to an Option<Error>?
> >
> > If possible, we can just keep it as a Try:
> > s/Option<Error>/Try<Docker>/
> >
> > That way, the check read a bit more directly:
> >
> > ```
> > return matches(test, "DOCKER_") && docker.isError();
> > ```
> > vs. existing patch:
> > ```
> > return matches(test, "DOCKER_") && dockerError.isSome();
> > ```
I tried that, but I can't compile since Try<Error> has no default constructor, and I don't think I have anything useful to initialize it with. You have suggestions?
> On Sept. 23, 2014, 7:10 p.m., Ben Mahler wrote:
> > src/tests/environment.cpp, lines 224-241
> > <https://reviews.apache.org/r/25569/diff/7/?file=701944#file701944line224>
> >
> > Can this one be collapsed into the CgropusFilter? Seems a bit confusing to split the cgroup filter logic into two separate filters.
For some reason the original validation was testing both isRoot and Cgroups Params the same type, therefore I split it into two filters.
> On Sept. 23, 2014, 7:10 p.m., Ben Mahler wrote:
> > src/tests/environment.cpp, lines 249-269
> > <https://reviews.apache.org/r/25569/diff/7/?file=701944#file701944line249>
> >
> > Does the following seem a bit easier to read? Trying to keep it as only one logical structure (only 1 "matches" expression, but what happens is controlled by the #ifdef):
> >
> >
> > ```
> > #ifdef WITH_NETWORK_ISOLATOR
> > if (matches(test, "MultipleSlaves")) {
> > return !routing::check().isError();
> > }
> > #endif
> >
> > if (matches(test, "PortMappingIsolatorTest") ||
> > matches(test, "PortMappingMesosTest")) {
> > #ifdef WITH_NETWORK_ISOLATOR
> > return routing::check().isError();
> > #else
> > return true;
> > #endif
> > }
> >
> > return false;
> > ```
> >
> > vs. current patch:
> >
> >
> > ```
> > #ifdef WITH_NETWORK_ISOLATOR
> > if (matches(test, "MultipleSlaves")) {
> > return !routing::check().isError();
> > }
> >
> > if (matches(test, "PortMappingIsolatorTest") ||
> > matches(test, "PortMappingMesosTest")) {
> > return routing::check().isError();
> > }
> >
> > return false;
> > #else
> > return matches(test, "PortMappingIsolatorTest") ||
> > matches(test, "PortMappingMesosTest");
> > #endif // WITH_NETWORK_ISOLATOR
> > }
> > ```
> >
> > FWIW, this is what you did for CgroupsFilter as well :)
:) thx, I'll be consistent in my style then!
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review54300
-----------------------------------------------------------
On Sept. 23, 2014, 5:28 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2014, 5:28 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/25569
>
>
> Diffs
> -----
>
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 25569: Refactor test environment validations
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review54300
-----------------------------------------------------------
This is coming together really nicely Tim! Just some minor cleanups left at this point, and then we can get this committed! :)
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment94351>
Missing includes for these?
#include <set>
#include <vector>
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment94356>
At this point I think these methods are self-evident, but I will leave it to you as to whether you think these comments are still beneficial. :)
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment94357>
const string& pattern
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment94398>
The other CgroupsParamTypeFilter checks for "root" user, but this one doesn't?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment94369>
Any reason to for the conversion to an Option<Error>?
If possible, we can just keep it as a Try:
s/Option<Error>/Try<Docker>/
That way, the check read a bit more directly:
```
return matches(test, "DOCKER_") && docker.isError();
```
vs. existing patch:
```
return matches(test, "DOCKER_") && dockerError.isSome();
```
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment94396>
Can this one be collapsed into the CgropusFilter? Seems a bit confusing to split the cgroup filter logic into two separate filters.
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment94392>
Does the following seem a bit easier to read? Trying to keep it as only one logical structure (only 1 "matches" expression, but what happens is controlled by the #ifdef):
```
#ifdef WITH_NETWORK_ISOLATOR
if (matches(test, "MultipleSlaves")) {
return !routing::check().isError();
}
#endif
if (matches(test, "PortMappingIsolatorTest") ||
matches(test, "PortMappingMesosTest")) {
#ifdef WITH_NETWORK_ISOLATOR
return routing::check().isError();
#else
return true;
#endif
}
return false;
```
vs. current patch:
```
#ifdef WITH_NETWORK_ISOLATOR
if (matches(test, "MultipleSlaves")) {
return !routing::check().isError();
}
if (matches(test, "PortMappingIsolatorTest") ||
matches(test, "PortMappingMesosTest")) {
return routing::check().isError();
}
return false;
#else
return matches(test, "PortMappingIsolatorTest") ||
matches(test, "PortMappingMesosTest");
#endif // WITH_NETWORK_ISOLATOR
}
```
FWIW, this is what you did for CgroupsFilter as well :)
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment94350>
How about s/disabledTests/disabled/ ?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment94349>
Looks like you're always adding two colons because of the outer strings::join(). You shouldn't need the ":" here anymore, right?
How about the following to be consistent with the rest of our code formatting:
```
disabled.push_back(
test->test_case_name() + string(".") + test->name());
```
- Ben Mahler
On Sept. 23, 2014, 5:28 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2014, 5:28 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/25569
>
>
> Diffs
> -----
>
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 25569: Refactor test environment validations
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/
-----------------------------------------------------------
(Updated Sept. 23, 2014, 5:28 a.m.)
Review request for mesos and Ben Mahler.
Repository: mesos-git
Description
-------
Review: https://reviews.apache.org/r/25569
Diffs (updated)
-----
src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
Diff: https://reviews.apache.org/r/25569/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 25569: Refactor test environment validations
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/
-----------------------------------------------------------
(Updated Sept. 22, 2014, 9:17 p.m.)
Review request for mesos and Ben Mahler.
Repository: mesos-git
Description
-------
Review: https://reviews.apache.org/r/25569
Diffs (updated)
-----
src/linux/cgroups.cpp 62df4b7645c6ab061a47634058d79ca849caa6b9
src/slave/containerizer/isolators/cgroups/cpushare.hpp 2187c296ea9b1a7de9ae3f09fdf1983f98a3d01b
src/slave/containerizer/isolators/cgroups/cpushare.cpp 7164ecc0f068d4a72248521e3cbd345958efa880
src/slave/containerizer/isolators/cgroups/mem.cpp b3d4a5daa90a842e501bc6be2f0cf20fe22906ac
src/slave/containerizer/isolators/cgroups/perf_event.cpp 4ced508e600e13f3e5ae9d12ea199de743def652
src/slave/containerizer/linux_launcher.cpp f7bc894830a7ca3f55465dacc7b653cdc2d7758b
src/slave/slave.cpp 9a6646f0249fd43ae5d13bd9ee3aaaab5da08412
src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
Diff: https://reviews.apache.org/r/25569/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 25569: Refactor test environment validations
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/
-----------------------------------------------------------
(Updated Sept. 20, 2014, 5:09 p.m.)
Review request for mesos and Ben Mahler.
Repository: mesos-git
Description
-------
Review: https://reviews.apache.org/r/25569
Diffs (updated)
-----
src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
Diff: https://reviews.apache.org/r/25569/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 25569: Refactor test environment validations
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review54045
-----------------------------------------------------------
Thanks Tim, this is looking really nice!
Appreciate the patience, just one more round of cleanup and we should be all set. :D
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93977>
Can you remove this? We only have two callers, and the code is really trivial in the callers. :)
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93976>
This might be a bit more succinct:
// Returns whether the test should be filtered.
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93965>
Curious why you want this vector version of matches.
It adds a method, and there is still more code in the call site than just calling matches twice:
Current:
```
vector<string> patterns;
patterns.push_back("PortMappingIsolatorTest");
patterns.push_back("PortMappingMesosTest");
return matches(test, patterns) && !isRoutingEnabled;
```
vs. removing vector matches:
```
return (matches(test, "PortMappingIsolatorTest")
matches(test, "PortMappingMesosTest") &&
!isRoutingEnabled;
```
Probably an if is a bit easier to read:
```
if (matches(test, "PortMappingIsolatorTest") ||
matches(test, "PortMappingMesosTest") {
return routingDisabled;
}
return false;
```
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93979>
Could we avoid the need for a constructor and a member variable by calling os::user() inside filter()? Let's avoid trying to optimize this since it's likely not noticeably expensive.
IMO doing it all in filter() actually makes the code more readable as well. :)
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93980>
Can we avoid the need for the constructor + member variable here as well, by doing it all in filter()?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93972>
We have 'enabled' in the name, so the 'is' seems redundant, the following seem clear without the 'is' prefix:
'cgroupsEnabled', 'dockerEnabled', etc.
What do you think?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93981>
How about including the message for non-linux systems as well:
```
#ifdef __linux__
docker = Docker::create(flags.docker);
#else
docker = Error("Docker tests not supported on non-Linux systems");
#endif
if (docker.isError()) {
// print message;
}
```
Could we store the Try<Docker> as a member instead of converting to the 'isDockerEnabled' boolean? Doesn't look like the boolean is giving us very much? It even lets us make the call to Docker::create vs Error("non linux") in the initializer list.
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93984>
Do you want to remove the std:: qualifiers here?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93982>
This is pretty implicit for non-Linux systems, in that we treat them as having hierarchies.
Could we have the #ifdef in the filter() method to make this a bit more clear?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93983>
Based on my other comment, how about just storing 'set<string> hierarchies' and checking .empty() in filter()? But inside a linux #ifdef! ;)
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93985>
We can avoid the need for the Constructor and the member variable if we just call these in filter().
The #ifdef inside filter will make things a bit clearer as well!
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93986>
Ditto, can you avoid the constructor + member variable, and instead push the #ifdef down into filter() to make things a bit clearer?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93960>
Leftover 'process::'?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93958>
Very clean, thanks Tim!
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93954>
What about s/disabledTest/filtered/ ?
If disabledTest() returned a vector of strings as before you can do the following here:
```
disabled += strings::join(":", filtered(unitTest, filters);
```
No clunky loop necessary. :)
I should have been a bit more clear in my previous comment, what I meant was that we can do the strings::join here if we're not tacking on ":" in our vector items in disabledTest(). Sound good?
- Ben Mahler
On Sept. 19, 2014, 10:27 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2014, 10:27 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/25569
>
>
> Diffs
> -----
>
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 25569: Refactor test environment validations
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review54063
-----------------------------------------------------------
Patch looks great!
Reviews applied: [25569]
All tests passed.
- Mesos ReviewBot
On Sept. 19, 2014, 10:27 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2014, 10:27 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/25569
>
>
> Diffs
> -----
>
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 25569: Refactor test environment validations
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/
-----------------------------------------------------------
(Updated Sept. 19, 2014, 10:27 p.m.)
Review request for mesos and Ben Mahler.
Repository: mesos-git
Description
-------
Review: https://reviews.apache.org/r/25569
Diffs (updated)
-----
src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
Diff: https://reviews.apache.org/r/25569/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 25569: Refactor test environment validations
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review53627
-----------------------------------------------------------
Patch looks great!
Reviews applied: [25569]
All tests passed.
- Mesos ReviewBot
On Sept. 16, 2014, 10:35 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2014, 10:35 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/25569
>
>
> Diffs
> -----
>
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 25569: Refactor test environment validations
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review53993
-----------------------------------------------------------
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93864>
Sure, I didn't see any other possibility needed so I just merged it here. I can just return the TestInfo instead.
- Timothy Chen
On Sept. 16, 2014, 10:35 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2014, 10:35 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/25569
>
>
> Diffs
> -----
>
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 25569: Refactor test environment validations
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/
-----------------------------------------------------------
(Updated Sept. 16, 2014, 10:35 p.m.)
Review request for mesos and Ben Mahler.
Repository: mesos-git
Description (updated)
-------
Review: https://reviews.apache.org/r/25569
Diffs (updated)
-----
src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
Diff: https://reviews.apache.org/r/25569/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 25569: Refactor test environment validations
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/
-----------------------------------------------------------
(Updated Sept. 16, 2014, 7:07 a.m.)
Review request for mesos and Ben Mahler.
Summary (updated)
-----------------
Refactor test environment validations
Repository: mesos-git
Description (updated)
-------
Review: https://reviews.apache.org/r/25569
Refactor how validation is done in the environment by defining a standard test filter interface, and only execute the validation once.
Diffs (updated)
-----
src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
Diff: https://reviews.apache.org/r/25569/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 25569: Refactor test environment validations
Posted by Timothy Chen <tn...@apache.org>.
> On Sept. 12, 2014, 11:39 p.m., Ben Mahler wrote:
> > Thanks for following up!
> >
> > Not your fault, but the current design of enable() seems a bit unfortunate, because we will print things excessively unless we use static variables as you've done here.
> > What about the following instead?
> >
> > (1) Add a method called 'disabled()' that returns a list of disabled test names.
> > (2) Inside 'disabled()', we can print the disablement messages once at the beginning, then loop over the tests to construct the list of disabled test names to return.
> > (3) Inside 'Environment::Environment()', we use disabled() to construct the full 'disabled' string.
> >
> > Does that seem cleaner?
> >
> > The static variables seem good for now, modulo my comment below. Consider adding a TODO to clean this up if you go this route!
Ok I've done a refactor based on your comments, let me know what you think.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review53246
-----------------------------------------------------------
On Sept. 16, 2014, 7:07 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2014, 7:07 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/25569
>
> Refactor how validation is done in the environment by defining a standard test filter interface, and only execute the validation once.
>
>
> Diffs
> -----
>
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 25569: Only perform docker validation once for
tests
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review53246
-----------------------------------------------------------
Thanks for following up!
Not your fault, but the current design of enable() seems a bit unfortunate, because we will print things excessively unless we use static variables as you've done here.
What about the following instead?
(1) Add a method called 'disabled()' that returns a list of disabled test names.
(2) Inside 'disabled()', we can print the disablement messages once at the beginning, then loop over the tests to construct the list of disabled test names to return.
(3) Inside 'Environment::Environment()', we use disabled() to construct the full 'disabled' string.
Does that seem cleaner?
The static variables seem good for now, modulo my comment below. Consider adding a TODO to clean this up if you go this route!
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment92803>
You've introduced a `return true` case here when docker is "verified", that looks like a regression.
How about encoding this tri-state situation (not initialized, not enabled, enabled) through a Result?
```
static Result<Docker> docker = None();
if (docker.isNone()) {
Try<Docker> docker_ = Docker::create(flags.docker);
if (docker_.isError()) {
std::cerr << ...
<< std::endl;
}
// Unfortunately there is no Result<T> constructor yet that takes a Try<T>.
docker = docker_.isSome() ? docker_.get() : Try<Docker>::error(docker_.error());
}
if (docker.isError()) {
return false;
}
```
- Ben Mahler
On Sept. 12, 2014, 4:31 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2014, 4:31 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Only perform docker validation once for tests
>
>
> Diffs
> -----
>
> src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747
>
> Diff: https://reviews.apache.org/r/25569/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>