mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 44945: Add autoconf tests for XFS project quotas.
Date Mon, 21 Mar 2016 07:52:41 GMT


> On March 18, 2016, 11:36 a.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``).

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?


- Jiang Yan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44945/#review124239
-----------------------------------------------------------


On March 20, 2016, 6:59 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> -----------------------------------------------------------
> 
> (Updated March 20, 2016, 6:59 p.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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message