> 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 > >
