Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-04-07 Thread James Peach 

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

(Updated April 7, 2016, 9:49 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac c693b825294f82ace8a14563cf2229820e159e3c 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-04-06 Thread James Peach 

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

(Updated April 6, 2016, 10:35 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac c693b825294f82ace8a14563cf2229820e159e3c 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-04-06 Thread James Peach 

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

(Updated April 6, 2016, 6:42 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  3rdparty/libprocess/include/process/limiter.hpp 
b4b6a7bb48aa19fa40c0ea9f8759b8f553f43ca6 
  configure.ac c693b825294f82ace8a14563cf2229820e159e3c 
  docs/docker-containerizer.md 6ec1cecd2e39bc4737688d8c4f6fc05e82c92023 
  src/CMakeLists.txt 9a4cffa952a452d5df70ea42949142e534067cd6 
  src/Makefile.am ba9cc8b683bba2ae8fe9d9c58642690f5b80afaf 
  src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
  src/launcher/executor.cpp bec9cba091df2d1b340e68e67966ec322558c315 
  src/master/allocator/mesos/hierarchical.hpp 
be4ccffa6042261f03690eb38b071e5f3b6c1105 
  src/master/allocator/mesos/hierarchical.cpp 
a8d80b4412b324717f5b5d3ff2711eba0f7a2ed5 
  src/slave/http.cpp a684ff504535e2b9b2064f8048f5e03a0efcb059 
  src/slave/monitor.hpp PRE-CREATION 
  src/slave/monitor.cpp PRE-CREATION 
  src/slave/slave.hpp 5b6076afc62cc3978276162aa4ea6e8c0670f1ab 
  src/slave/slave.cpp cd4264e7376edaba079115632a94e710375f2a55 
  src/tests/limiter.hpp 16d5682b0d76d2348a78abd568deb9fd41900ee4 
  src/tests/mesos.hpp 8c8cd1a55bb37bd9bda53b7dd7e9caa178f2a923 
  src/tests/mesos.cpp 803b925d6741e43342419b600756a7e1e906783c 
  src/tests/monitor_tests.cpp PRE-CREATION 
  src/tests/oversubscription_tests.cpp 84e1104ea14c976b4184c8570ee996891fe260a5 
  src/tests/slave_tests.cpp c4d80aa21a8640ef36226458d684bb3d31366c81 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-04-05 Thread James Peach 

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

(Updated April 5, 2016, 11:40 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac c693b825294f82ace8a14563cf2229820e159e3c 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-04-05 Thread James Peach 

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

(Updated April 5, 2016, 11:15 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac c693b825294f82ace8a14563cf2229820e159e3c 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-04-05 Thread James Peach 

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

(Updated April 5, 2016, 11 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  3rdparty/Makefile.am 0523151d924a1398334767bd401ae84e41bdceb7 
  3rdparty/cmake/Versions.cmake 86c51edb3aa2daf6451459aaf18278f09b91b000 
  3rdparty/libprocess/3rdparty/http-parser-2.6.1.tar.gz PRE-CREATION 
  3rdparty/libprocess/3rdparty/http-parser-2.6.2.patch  
  3rdparty/libprocess/3rdparty/http-parser-2.6.2.tar.gz 
f42342a03b351ab223615883583e79de62d81ee1 
  3rdparty/libprocess/3rdparty/stout/include/stout/errorbase.hpp 
96d395d37cdad706c2f23fdd9caeefc2a33e0541 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/parse.hpp 
ef365e4d714a2c25d358a702722c5f1216869382 
  3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/grp.hpp 
232588c46c8207c6a002f60d00aa675f9e505083 
  3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/pwd.hpp 
46e3ec25cbf379fb2b82297849216b0866baf333 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
edaa76a5322d0bf60b7172405aa754b5aca95458 
  3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 
89dedec0c818263f17f220a2e71ed470bf75a42f 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/error.hpp 
d634491b301632ce6468f86d04f21fa25afbbaaa 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/format.hpp 
872ddbc6e3cd81ac5976ee12df2db32905a9fe7b 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
c48106e5905e3be0faeba7177ef534766089faff 
  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
66e16abe914e2a1ee7599bab857ff478b7ec20dc 
  3rdparty/libprocess/3rdparty/versions.am 
5506eb3aeafb09226ba5b90dae194ece31285412 
  3rdparty/libprocess/include/process/network.hpp 
2bc940fb3ba2bc6b2fed281a3920d13fdb1277e9 
  3rdparty/libprocess/src/encoder.hpp 69163830eaa9f77132a16fc14351b309144827bf 
  3rdparty/libprocess/src/poll_socket.cpp 
cb2878565a112017b190b4ff83dc65a876ea45f9 
  3rdparty/libprocess/src/process.cpp d2c458ed93307f75358bb642aaf2ed8e17b2fe97 
  3rdparty/libprocess/src/time.cpp a6c3f3de69e056544406f4416c2d0aea06adc34d 
  3rdparty/versions.am a94723e213dbbbd1df7e1b784386023412f7b961 
  3rdparty/zookeeper-3.4.5.patch PRE-CREATION 
  3rdparty/zookeeper-3.4.8.patch 486df1ae96af3426835c9d47ff2e36dd47ccde3f 
  3rdparty/zookeeper-3.4.8.tar.gz a23d68becf596b0246371fc244f018e4c71ca5d9 
  CHANGELOG b5dd490a064a226d242448093a306b9485a88de3 
  LICENSE 19130d977986dc9e23872817251ffcaa5a3c2c7d 
  configure.ac c693b825294f82ace8a14563cf2229820e159e3c 
  docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
  docs/mesos-containerizer.md 2fde74362f35568bb944da80a1db17e6d3ba9d7b 
  docs/modules.md 28eb233a30b844b302fd95c03e4ff6647355cdfa 
  docs/versioning.md 800debe5ddb40f941a3b839aa79dddbd2dbf1462 
  include/mesos/docker/spec.hpp 2ebacc70d92a593c8dd006b34519c3a2a5225481 
  src/docker/docker.hpp da6e9a25b99079940958001128ee949cdb9b931b 
  src/docker/docker.cpp bb9ddde27464aa4d9215e3ca673fa87c6e00e326 
  src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
  src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 
  src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3 
  src/examples/java/test-log.in 4132617ccbe5067c22d959a58640be825cedfe67 
  src/examples/no_executor_framework.cpp 
f578edfd99f3b7adf19cf06eab20696532c7b67d 
  src/examples/persistent_volume_framework.cpp 
b4faa0ee25dc3a72c17ef2b0640a3695423ef79a 
  src/examples/test_hook_module.cpp 3ff9fd71c275fd1a705ab3aca7a8041f29289bb0 
  src/exec/exec.cpp a2e6d86fd0b1f3d688d17296151db74bcb9b3418 
  src/launcher/executor.cpp 4658b9e5bb48b09e403991c8bdfdbf4d4dae0416 
  src/linux/cgroups.hpp 5f4010734ed9e3295dcc3a4390123e4f4ce99c16 
  src/linux/fs.cpp 2087b4ac1503e0fd085319b1017389f1f947536f 
  src/local/local.cpp 0d980188f933a8d543af696d8addd7ca5855413e 
  src/master/allocator/mesos/hierarchical.cpp 
cd6b91edf3dde9bc4c978daefdfc83d7c3c69304 
  src/master/main.cpp 181bbcb1758c0e9b83ef46496e990ce3d8c2195c 
  src/master/master.hpp e5b16f94fa5650b9db44cd7c975adeb5871e16f6 
  src/master/master.cpp 2cfa5075991bbb5df8a7e33b14767de558bda4ac 
  src/master/validation.cpp 504cd9b8bd5d40bb591b7aa5a23bd74cc210c2fc 
  src/module/manager.cpp 7850fd3e1e574e1a92289118e2e27c62b68eaf05 
  src/python/native_common/ext_modules.py.in 
ad98a576ee3662ccb5b8900a339e530089f41355 
  src/sched/sched.cpp d740ef630683b61554db58e628333e8a19e9aa27 
  src/slave/containerizer/docker.cpp 0576eab65a1f3556644472d5c9baba4ea9c34346 
  src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.cpp

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-04-04 Thread James Peach 

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

(Updated April 4, 2016, 5:26 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac c693b825294f82ace8a14563cf2229820e159e3c 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-31 Thread James Peach 

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

(Updated April 1, 2016, midnight)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac c693b825294f82ace8a14563cf2229820e159e3c 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-30 Thread James Peach 

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

(Updated March 30, 2016, 10:18 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and updates with review comments.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac 812c92aa13c0cc01c4907669000d050221cb62fd 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-30 Thread Jiang Yan Xu 


> On March 25, 2016, 11:43 p.m., Jiang Yan Xu wrote:
> > configure.ac, lines 957-960
> > 
> >
> > Can we put AC_MSG_CHECKING before `AC_CHECK_HEADERS` and inside `AS_IF` 
> > above?
> > 
> > i.e., AC_MSG_CHECKING/AC_MSG_CHECKING/AC_MSG_ERROR inform user what 
> > they don't already know (whether the system dependencies are met), not what 
> > they already know (the fact that they provided the 
> > `--enable_xfs_disk_isolator` flag).
> > 
> > And this check is conditional: we only print `AC_MSG_CHECKING` and do 
> > these checks when `test "x$enable_xfs_disk_isolator" = "xyes"`; we print 
> > `AC_MSG_ERROR([...])` when such checks fail and `AC_MSG_RESULT([yes])` when 
> > all checks succeed.
> > 
> > Would this work?
> > 
> > If we do this, then the message could be `AC_MSG_CHECKING([whether we 
> > can enable the XFS disk isolator])`
> 
> James Peach wrote:
> No, this message should be outside the check so that the builder always 
> knows what happened, independent of what configure flags they passed. 
> Typically, people start by not passing any flags, and it is helpful to 
> explicitly log that a feature was not enabled. Even if you pass the --enable 
> flags, it is comforting to get an explicit confirmation that the feature you 
> asked for is enabled.
> 
> Jiang Yan Xu wrote:
> I agree that explicitly confirming that the user has enabled something is 
> better. 
> 
> I still don't think we should print it after we've done all the 
> dependency checks for it and have potentially aborted configure with error 
> messages but without telling the error is due to "whether to enable the XFS 
> disk isolator..."
> 
> i.e., the following is better.
> 
> ```
> checking whether to enable the XFS disk isolator... yes
> 
> OR 
> 
> checking whether to enable the XFS disk isolator... missing libblkid 
> headers
> ---
> Please install the libblkid development package.
> ---
> 
> OR
> 
> checking whether to enable the XFS disk isolator... no / checking whether 
> to enable the XFS disk isolator... not enabled by user
> 
> ```
> 
> Agreed?
> 
> James Peach wrote:
> If you do it this way, you get the headers, etc check output intermingled 
> in the middle of your nice clean feature check message.

I see. Seems like technology is really lacking in this! In the absence of a 
perfect solution, IMO this is better:

checking whether we are asked to enable the XFS disk isolator... yes
checking for xfs/xfs.h... yes
// Move on to other headers

OR

checking whether we are asked to enable the XFS disk isolator... yes
checking for xfs/xfs.h... missing XFS quota headers
---
Please install the Linux kernel headers and xfsprogs development
packages for XFS disk isolator support.
---

OR 
checking whether we are asked to enable the XFS disk isolator... no
// Move on to other feature checking


TBH I haven't seen this style of using AC_MSG_CHECKING to check a user supplied 
flag so I am not convinced it's idiomatic but I do agree with the comforting 
factor. So let's at least make the message honest about what it's doing.

The above style still doesn't print anything when it has finished checking all 
of the dependencies for XFS but it's not like autoconf has ever done a good job 
of this: this is lack of information.

At least we are not printing all these header checking messages and potentially 
fail before we print "checking whether to enable the XFS disk isolator", which 
implies could be misleading.


- Jiang Yan


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


On March 29, 2016, 1:55 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 29, 2016, 1:55 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 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-29 Thread James Peach 

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

(Updated March 29, 2016, 8:55 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and updated.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-28 Thread James Peach 


> On March 26, 2016, 6:43 a.m., Jiang Yan Xu wrote:
> > configure.ac, lines 957-960
> > 
> >
> > Can we put AC_MSG_CHECKING before `AC_CHECK_HEADERS` and inside `AS_IF` 
> > above?
> > 
> > i.e., AC_MSG_CHECKING/AC_MSG_CHECKING/AC_MSG_ERROR inform user what 
> > they don't already know (whether the system dependencies are met), not what 
> > they already know (the fact that they provided the 
> > `--enable_xfs_disk_isolator` flag).
> > 
> > And this check is conditional: we only print `AC_MSG_CHECKING` and do 
> > these checks when `test "x$enable_xfs_disk_isolator" = "xyes"`; we print 
> > `AC_MSG_ERROR([...])` when such checks fail and `AC_MSG_RESULT([yes])` when 
> > all checks succeed.
> > 
> > Would this work?
> > 
> > If we do this, then the message could be `AC_MSG_CHECKING([whether we 
> > can enable the XFS disk isolator])`
> 
> James Peach wrote:
> No, this message should be outside the check so that the builder always 
> knows what happened, independent of what configure flags they passed. 
> Typically, people start by not passing any flags, and it is helpful to 
> explicitly log that a feature was not enabled. Even if you pass the --enable 
> flags, it is comforting to get an explicit confirmation that the feature you 
> asked for is enabled.
> 
> Jiang Yan Xu wrote:
> I agree that explicitly confirming that the user has enabled something is 
> better. 
> 
> I still don't think we should print it after we've done all the 
> dependency checks for it and have potentially aborted configure with error 
> messages but without telling the error is due to "whether to enable the XFS 
> disk isolator..."
> 
> i.e., the following is better.
> 
> ```
> checking whether to enable the XFS disk isolator... yes
> 
> OR 
> 
> checking whether to enable the XFS disk isolator... missing libblkid 
> headers
> ---
> Please install the libblkid development package.
> ---
> 
> OR
> 
> checking whether to enable the XFS disk isolator... no / checking whether 
> to enable the XFS disk isolator... not enabled by user
> 
> ```
> 
> Agreed?

If you do it this way, you get the headers, etc check output intermingled in 
the middle of your nice clean feature check message.


- James


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


On March 26, 2016, 5:05 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 26, 2016, 5:05 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 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-28 Thread Jiang Yan Xu 


> On March 25, 2016, 11:43 p.m., Jiang Yan Xu wrote:
> > configure.ac, lines 957-960
> > 
> >
> > Can we put AC_MSG_CHECKING before `AC_CHECK_HEADERS` and inside `AS_IF` 
> > above?
> > 
> > i.e., AC_MSG_CHECKING/AC_MSG_CHECKING/AC_MSG_ERROR inform user what 
> > they don't already know (whether the system dependencies are met), not what 
> > they already know (the fact that they provided the 
> > `--enable_xfs_disk_isolator` flag).
> > 
> > And this check is conditional: we only print `AC_MSG_CHECKING` and do 
> > these checks when `test "x$enable_xfs_disk_isolator" = "xyes"`; we print 
> > `AC_MSG_ERROR([...])` when such checks fail and `AC_MSG_RESULT([yes])` when 
> > all checks succeed.
> > 
> > Would this work?
> > 
> > If we do this, then the message could be `AC_MSG_CHECKING([whether we 
> > can enable the XFS disk isolator])`
> 
> James Peach wrote:
> No, this message should be outside the check so that the builder always 
> knows what happened, independent of what configure flags they passed. 
> Typically, people start by not passing any flags, and it is helpful to 
> explicitly log that a feature was not enabled. Even if you pass the --enable 
> flags, it is comforting to get an explicit confirmation that the feature you 
> asked for is enabled.

I agree that explicitly confirming that the user has enabled something is 
better. 

I still don't think we should print it after we've done all the dependency 
checks for it and have potentially aborted configure with error messages but 
without telling the error is due to "whether to enable the XFS disk isolator..."

i.e., the following is better.

```
checking whether to enable the XFS disk isolator... yes

OR 

checking whether to enable the XFS disk isolator... missing libblkid headers
---
Please install the libblkid development package.
---

OR

checking whether to enable the XFS disk isolator... no / checking whether to 
enable the XFS disk isolator... not enabled by user

```

Agreed?


- Jiang Yan


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


On March 26, 2016, 10:05 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 26, 2016, 10:05 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 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-26 Thread James Peach 

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

(Updated March 26, 2016, 5:05 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and addressed review comments.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-26 Thread James Peach 


> On March 26, 2016, 6:43 a.m., Jiang Yan Xu wrote:
> > configure.ac, lines 957-960
> > 
> >
> > Can we put AC_MSG_CHECKING before `AC_CHECK_HEADERS` and inside `AS_IF` 
> > above?
> > 
> > i.e., AC_MSG_CHECKING/AC_MSG_CHECKING/AC_MSG_ERROR inform user what 
> > they don't already know (whether the system dependencies are met), not what 
> > they already know (the fact that they provided the 
> > `--enable_xfs_disk_isolator` flag).
> > 
> > And this check is conditional: we only print `AC_MSG_CHECKING` and do 
> > these checks when `test "x$enable_xfs_disk_isolator" = "xyes"`; we print 
> > `AC_MSG_ERROR([...])` when such checks fail and `AC_MSG_RESULT([yes])` when 
> > all checks succeed.
> > 
> > Would this work?
> > 
> > If we do this, then the message could be `AC_MSG_CHECKING([whether we 
> > can enable the XFS disk isolator])`

No, this message should be outside the check so that the builder always knows 
what happened, independent of what configure flags they passed. Typically, 
people start by not passing any flags, and it is helpful to explicitly log that 
a feature was not enabled. Even if you pass the --enable flags, it is 
comforting to get an explicit confirmation that the feature you asked for is 
enabled.


- James


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


On March 26, 2016, 5:05 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 26, 2016, 5:05 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 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-26 Thread Jiang Yan Xu 

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




configure.ac (line 258)


Two spaces to the right.

Also, append "default: no"



configure.ac (line 259)


We probably shouldn't take option arugments, i.e., 
`--enable-xfs-disk-isolator` is sufficient. Otherwise there is confusion with 
`--enable-xfs-disk-isolator=yes|true|1|absolutely`. :)

s/[enable_xfs_disk_isolator=no]/[] for consistency.



configure.ac (line 930)


Kill this blank line.

Also, I know the headers below imply Linux but I guess it won't hurt to add 

```
  AS_IF([test "$OS_NAME" = "linux"],
[],
[AC_MSG_ERROR([cannot build xfs disk isolator
---
XFS disk isolator is only supported on Linux.
---
  ])])
```

so the error message is clearer.



configure.ac (line 934)


Move one space to the right.



configure.ac (line 954)


This could be the place we insert 

```
AC_DEFINE([ENABLE_XFS_DISK_ISOLATOR])
```



configure.ac (lines 957 - 960)


Can we put AC_MSG_CHECKING before `AC_CHECK_HEADERS` and inside `AS_IF` 
above?

i.e., AC_MSG_CHECKING/AC_MSG_CHECKING/AC_MSG_ERROR inform user what they 
don't already know (whether the system dependencies are met), not what they 
already know (the fact that they provided the `--enable_xfs_disk_isolator` 
flag).

And this check is conditional: we only print `AC_MSG_CHECKING` and do these 
checks when `test "x$enable_xfs_disk_isolator" = "xyes"`; we print 
`AC_MSG_ERROR([...])` when such checks fail and `AC_MSG_RESULT([yes])` when all 
checks succeed.

Would this work?

If we do this, then the message could be `AC_MSG_CHECKING([whether we can 
enable the XFS disk isolator])`



configure.ac (lines 962 - 963)


If we pull this up into the aforementioned place we don't need to `test 
"x$enable_xfs_disk_isolator" = "xyes"` repeatedly and therefore be more concise.


- Jiang Yan Xu


On March 22, 2016, 4:21 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 22, 2016, 4:21 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 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-22 Thread James Peach 

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

(Updated March 22, 2016, 11:21 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-21 Thread James Peach 

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

(Updated March 22, 2016, 1:20 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and fixed review comments.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-21 Thread James Peach 

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

(Updated March 21, 2016, 9:46 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and fixed review comments.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-21 Thread Jie Yu 


> On March 18, 2016, 6:36 p.m., Jiang Yan Xu wrote:
> > configure.ac, line 923
> > 
> >
> > 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 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-21 Thread Jiang Yan Xu 


> On March 18, 2016, 11:36 a.m., Jiang Yan Xu wrote:
> > configure.ac, line 923
> > 
> >
> > 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 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-20 Thread James Peach 

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


Changes
---

Rebased and addressed review comments.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-20 Thread James Peach 


> On March 18, 2016, 6:36 p.m., Jiang Yan Xu wrote:
> > configure.ac, line 923
> > 
> >
> > 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.

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``).


- James


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


On March 18, 2016, 3:45 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 18, 2016, 3:45 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 8e4f03593df4a8ba13f00292963e351acc3f71c1 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-19 Thread Jiang Yan Xu 

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




configure.ac (line 923)


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


On March 17, 2016, 8:45 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 17, 2016, 8:45 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 8e4f03593df4a8ba13f00292963e351acc3f71c1 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-19 Thread Jie Yu 


> On March 18, 2016, 6:36 p.m., Jiang Yan Xu wrote:
> > configure.ac, line 923
> > 
> >
> > 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`.

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.


- Jie


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


On March 18, 2016, 3:45 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 18, 2016, 3:45 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 8e4f03593df4a8ba13f00292963e351acc3f71c1 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-19 Thread James Peach 

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

(Updated March 17, 2016, 10:43 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac 8e4f03593df4a8ba13f00292963e351acc3f71c1 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-19 Thread James Peach 

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

(Updated March 18, 2016, 3:45 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-4828
https://issues.apache.org/jira/browse/MESOS-4828


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac 8e4f03593df4a8ba13f00292963e351acc3f71c1 

Diff: https://reviews.apache.org/r/44945/diff/


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

2016-03-18 Thread Jiang Yan Xu 


> On March 18, 2016, 11:36 a.m., Jiang Yan Xu wrote:
> > configure.ac, line 923
> > 
> >
> > 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.

Sorry I meant `--enable-xfs-isolator`.


- Jiang Yan


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


On March 17, 2016, 8:45 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 17, 2016, 8:45 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 8e4f03593df4a8ba13f00292963e351acc3f71c1 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>