> On March 18, 2016, 6:36 p.m., Jiang Yan Xu wrote: > > configure.ac, line 923 > > <https://reviews.apache.org/r/44945/diff/3/?file=1304828#file1304828line923> > > > > Sorry this should have been a continued discussion on > > https://reviews.apache.org/r/44342/#comment184051 but anyways: > > > > Your response to my initial comments: > > > There's no AC_ARG_WITH. If the dependencies area available we will > > build the isolator and the operator can configure it at runtime. I don't > > think there's any need to disable this at build time since it is not > > enabled by default anyway. > > > > I am still favoring building XFS isolator based on > > `--with-xfs-isolator` (AC_ARG_ENABLE is probably better than AC_ARG_WITH) > > for the following reasons: > > > > > > 1. Explicitness: In general, if we build things solely based on the > > availability of headers, users can inadvertently build more things into the > > deployed binary which could result in larger binaries and perhaps other > > side effects (through bugs, etc.). > > 2. Preventing user mistake: I am thinking about how we would advise > > users on enabling this feature: > > 1. "The XFS isolator feature is disabled by default, to enable it in > > build, install these packages whose name could be different for different > > distros and if you haven't made mistakes in finding them, the feature will > > be built." vs. > > 2. "The XFS isolator feature is disabled by default, to enable it in > > build, install these packages (which could be named differently depending > > on your distro), if the dependencies are not satisfied, the build aborts > > with an message stating such error." The latter feels better to me. > > 3. Consistency: Other optional features of Mesos follow this pattern. > > Jiang Yan Xu wrote: > Sorry I meant `--enable-xfs-isolator`. > > Jie Yu wrote: > For consistency: I think the network isolator is using > `--with-network-isolator` (`AC_ARG_WITH([network-isolator],`), so it'e better > to be consistent and use 'AC_ARG_WITH' here. > > I agree with Yan that we should be more explicit, instead of relying on > headers. > > James Peach wrote: > My view is that unless there is a tradeof (performance, security, etc), > then every optional feature should be available at runtime. The alternative > imposes an undue burden on operators because they will have to repackage if > they want to enable a feature. > > However, I'm not going to argue this any furthere here. I'll just guard > this with ``AC_ARG_WITH`` (@xujyan is right this should have been > ``AC_ARG_ENABLE``). > > Jiang Yan Xu wrote: > We are already inconsistent in the use of enable vs. with: > `--enable-ssl`, `--enable-libevent`, `--enable-debug`, etc. > > I think the convention (not in Mesos) is: > > - "Build this problem with some **internal** feature xyz of this program > enabled", i.e., `--enable-xyz` > - "Build this program with some (potetally also specify its location) > **external** dependency abc". i.e, `--with-abc`. > > I'd prefer `--enable` here. What do you guys think?
yeah, sounds good. Could you please add a TODO to rename --with-network-isolator to --enable-port-mapping-isolator? - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44945/#review124239 ----------------------------------------------------------- On March 21, 2016, 1:59 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44945/ > ----------------------------------------------------------- > > (Updated March 21, 2016, 1:59 a.m.) > > > Review request for mesos, Jie Yu and Jiang Yan Xu. > > > Bugs: MESOS-4828 > https://issues.apache.org/jira/browse/MESOS-4828 > > > Repository: mesos > > > Description > ------- > > Add autoconf tests for XFS project quotas. > > > Diffs > ----- > > configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1fbbbb2d19ffb8 > > Diff: https://reviews.apache.org/r/44945/diff/ > > > Testing > ------- > > Make check. Manual verification. > > > Thanks, > > James Peach > >