Re: Review Request 41854: Added module initialization to Master main().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41854/ --- (Updated Feb. 13, 2016, 5:32 p.m.) Review request for mesos, Anand Mazumdar and Kapil Arya. Changes --- Added missing period to commit message. Summary (updated) - Added module initialization to Master main(). Bugs: MESOS-4253 https://issues.apache.org/jira/browse/MESOS-4253 Repository: mesos Description (updated) --- Added module initialization to Master main(). Diffs (updated) - src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 src/slave/main.cpp 222198ca89f672332cb80773a3f36fe1f0438f64 Diff: https://reviews.apache.org/r/41854/diff/ Testing --- make && make check [==] 932 tests from 119 test cases ran. (210502 ms total) [ PASSED ] 932 tests. Thanks, Marco Massenzio
Re: Review Request 41760: Add initialization method to Anonymous module
> On Feb. 8, 2016, 4:53 p.m., Kapil Arya wrote:
> > I am little concerned about `Flags` being passed to the module. Since there
> > is no visibility about the allowed master/agent flags from the module's
> > perspective, how does it cope with the changes to master/slave flags? Would
> > we want to expose available master/slave flags as well in include/mesos?
> > It's not quite clear what the correct representation would be.
>
> Marco Massenzio wrote:
> Hey Kapil,
>
> thanks for review!
> I'm in the middle of something, but I'll get round to it this weekend -
> it all seems very agreeable at a quick glance.
>
> I'll reply with more detail, but in the meantime, please note that teh
> idea was *not* expose the master/agent flags as types - the module simply
> iterates over the map (and getting the string values, not the actual typed
> objects); but I'm assuming that the flags' names will be from *fairly* to
> *very* stable, so the risk is rather low?
>
> I think that exposing the flags in an include module would actually make
> matters worse, as it would cause backward compatibility issues and would
> impact the ability of Mesos to move forward wrt existing modules?
>
> In any event, the way to cope with change would be as usual: for the
> *required* ones the module would have to fail (great suggestion to have a
> `Try` returned by the initialization) and for the *optional* ones, it will
> just have to make do without (possibly emitting warnings).
So, as a practical example, this is how it would be used in a module:
```
virtual void initialize(const flags::FlagsBase& flags)
{
string workDir;
string sandboxDir;
LOG(INFO) << "Executing init() for module; parsing runtime flags";
for(auto flag : flags) {
string name = flag.first;
Option value = flag.second.stringify(flags);
if (name == "work_dir" && value.isSome()) {
workDir = value.get();
} else if (name == "sandbox_directory" && value.isSome()) {
sandboxDir = value.get();
}
}
LOG(INFO) << "Configured work dir to [" << workDir
<< "] and Sandbox dir to [" << sandboxDir << "]";
process = new RemoteExecutionProcess(workDir, sandboxDir);
spawn(process);
}
```
This would only "break" if the names of the flags were to change - this would
be, I believe, extremely unlikely?
> On Feb. 8, 2016, 4:53 p.m., Kapil Arya wrote:
> > include/mesos/module/anonymous.hpp, line 59
> > <https://reviews.apache.org/r/41760/diff/2/?file=1180449#file1180449line59>
> >
> > I am wondering if we should allow the module to indicate an error
> > during initialization. Should the return type be `Try` instead?
Absolutely!
done.
> On Feb. 8, 2016, 4:53 p.m., Kapil Arya wrote:
> > src/examples/test_anonymous_module.hpp, lines 25-48
> > <https://reviews.apache.org/r/41760/diff/2/?file=1180450#file1180450line25>
> >
> > Let's move this to the cpp file as with the other modules.
Done - please note that we still need to leave the class definition in the
header file, as the test needs to cast the module object to verify it was
correctly created.
moved ``initialize()`` to the .cpp file, though.
> On Feb. 8, 2016, 4:53 p.m., Kapil Arya wrote:
> > src/examples/test_anonymous_module.cpp, lines 56-60
> > <https://reviews.apache.org/r/41760/diff/2/?file=1180451#file1180451line56>
> >
> > Do we also need a destructor just like in TestAnonymous?
Done; however TestAnonymous' destructor just logs, not a very useful destructor?
We don't allocate memory/resources that need freeing.
Also, in the current code, anon modules' destructors are never called (I was
planning to implement that - there is a TODO from benh - once we got this patch
reviewed).
> On Feb. 8, 2016, 4:53 p.m., Kapil Arya wrote:
> > src/slave/main.cpp, lines 275-278
> > <https://reviews.apache.org/r/41760/diff/2/?file=1180452#file1180452line275>
> >
> > Just add a CHECK here and remove the if-else. The module manager
> > returns `Error` for NULL.
Done.
I've also added a check on the Try returned by initialize() - I am also
following the same pattern as `create()`:
```
if (create.isError()) {
EXIT(EXIT_FAILURE)
<< "Failed to create anonymous module named '" << name << "'";
}
```
however, I'm not entirely sure we should crash the Agent (or Master, for that
matter) if an anon module cannot be initialized?
Are you ok with that?
> On Feb. 8, 2016, 4:53 p.m., Kapil Arya wrote:
> > src/tests/anonymous_tests.cpp, line 10
Re: Review Request 41854: Added module initialization to Master main() method
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41854/ --- (Updated Feb. 13, 2016, 6:32 a.m.) Review request for mesos, Anand Mazumdar and Kapil Arya. Changes --- Updated post-review (41760) changes. Note: for some reason the changes to src/slave/main.cpp are not picked up by this diff: this patch does NOT touch this file, it is exactly as it was in 41760. Bugs: MESOS-4253 https://issues.apache.org/jira/browse/MESOS-4253 Repository: mesos Description --- Added module initialization to Master main() method Diffs (updated) - src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 src/slave/main.cpp 222198ca89f672332cb80773a3f36fe1f0438f64 Diff: https://reviews.apache.org/r/41854/diff/ Testing --- All unit tests pass. Thanks, Marco Massenzio
Re: Review Request 41854: Added module initialization to Master main() method
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41854/ --- (Updated Feb. 13, 2016, 7:02 a.m.) Review request for mesos, Anand Mazumdar and Kapil Arya. Bugs: MESOS-4253 https://issues.apache.org/jira/browse/MESOS-4253 Repository: mesos Description --- Added module initialization to Master main() method Diffs (updated) - src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 src/slave/main.cpp 222198ca89f672332cb80773a3f36fe1f0438f64 Diff: https://reviews.apache.org/r/41854/diff/ Testing --- make && make check [==] 932 tests from 119 test cases ran. (210502 ms total) [ PASSED ] 932 tests. Thanks, Marco Massenzio
Re: Review Request 41760: Add initialization method to Anonymous class.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41760/ --- (Updated Feb. 13, 2016, 6:24 a.m.) Review request for mesos, Anand Mazumdar and Kapil Arya. Changes --- Addressed review's comment Summary (updated) - Add initialization method to Anonymous class. Bugs: MESOS-4253 https://issues.apache.org/jira/browse/MESOS-4253 Repository: mesos Description (updated) --- Jira: MESOS-4253 To provide a basic "context" to anonymous modules, we provide an `initialize(const FlagsBase&)` method that will be invoked shortly after creation of the module class. Diffs (updated) - include/mesos/module/anonymous.hpp 629eb123b9d5fa9e07ddb1c704ee72e96e8fec5b src/examples/test_anonymous_module.hpp 3da33a42f04b7421cee8fbb85e29b66e352f5894 src/examples/test_anonymous_module.cpp dd291cff3b5d47337e371cd2c1082fd6716af3fc src/slave/main.cpp 222198ca89f672332cb80773a3f36fe1f0438f64 src/tests/anonymous_tests.cpp bc29ff8be94af82dd97f43db4f9594036705e835 src/tests/module.hpp 4b32f29f2ce76100433621a5cb6b8cc87c9b38f8 src/tests/module.cpp 8cc305c0ef606b07eea39d548d3165a2bb2b042a Diff: https://reviews.apache.org/r/41760/diff/ Testing --- Unit tests pass. Also implemented in the [execute-module](http://github.com/massenz/execute-module) - and it works there too: ``` I0102 17:38:26.180788 19971 main.cpp:272] Initializing anonymous module 'com_alertavert_mesos_RemoteExecution' I0102 17:38:26.180800 19971 main.cpp:278] Sending flags to module 'com_alertavert_mesos_RemoteExecution' I0102 17:38:26.180810 19971 execute_module.hpp:129] Executing initialize() for module; parsing runtime flags I0102 17:38:26.181658 19971 execute_module.hpp:139] Configured work dir to [/tmp/agent] and Sandbox dir to [/mnt/mesos/sandbox] ``` Thanks, Marco Massenzio
Re: Review Request 41854: Added module initialization to Master main() method
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41854/ --- (Updated Feb. 13, 2016, 6:59 a.m.) Review request for mesos, Anand Mazumdar and Kapil Arya. Changes --- fixed typo Bugs: MESOS-4253 https://issues.apache.org/jira/browse/MESOS-4253 Repository: mesos Description --- Added module initialization to Master main() method Diffs (updated) - src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 src/slave/main.cpp 222198ca89f672332cb80773a3f36fe1f0438f64 Diff: https://reviews.apache.org/r/41854/diff/ Testing (updated) --- make && make check [==] 932 tests from 119 test cases ran. (210502 ms total) [ PASSED ] 932 tests. Thanks, Marco Massenzio
Re: Review Request 41760: Add initialization method to Anonymous module
> On Feb. 8, 2016, 4:53 p.m., Kapil Arya wrote: > > I am little concerned about `Flags` being passed to the module. Since there > > is no visibility about the allowed master/agent flags from the module's > > perspective, how does it cope with the changes to master/slave flags? Would > > we want to expose available master/slave flags as well in include/mesos? > > It's not quite clear what the correct representation would be. Hey Kapil, thanks for review! I'm in the middle of something, but I'll get round to it this weekend - it all seems very agreeable at a quick glance. I'll reply with more detail, but in the meantime, please note that teh idea was *not* expose the master/agent flags as types - the module simply iterates over the map (and getting the string values, not the actual typed objects); but I'm assuming that the flags' names will be from *fairly* to *very* stable, so the risk is rather low? I think that exposing the flags in an include module would actually make matters worse, as it would cause backward compatibility issues and would impact the ability of Mesos to move forward wrt existing modules? In any event, the way to cope with change would be as usual: for the *required* ones the module would have to fail (great suggestion to have a `Try` returned by the initialization) and for the *optional* ones, it will just have to make do without (possibly emitting warnings). - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41760/#review118242 --- On Jan. 4, 2016, 8:16 p.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41760/ > --- > > (Updated Jan. 4, 2016, 8:16 p.m.) > > > Review request for mesos, Anand Mazumdar and Kapil Arya. > > > Bugs: MESOS-4253 > https://issues.apache.org/jira/browse/MESOS-4253 > > > Repository: mesos > > > Description > --- > > Jira: [MESOS-4253](https://issues.apache.org/jira/browse/MESOS-4253) > > To provide a basic "context" to Anonymous modules, we pass in the `BaseFlags`. > > This tries to be backward compatible with existing modules, so a no-op > (`virtual`) implementation of the method is provided in the base class. > Currently the code is only implemented in the Agent's `main()` method, but if > deemed correct, adding it to Master is trivial. > > > Diffs > - > > include/mesos/module/anonymous.hpp 629eb123b9d5fa9e07ddb1c704ee72e96e8fec5b > src/examples/test_anonymous_module.hpp > 3da33a42f04b7421cee8fbb85e29b66e352f5894 > src/examples/test_anonymous_module.cpp > dd291cff3b5d47337e371cd2c1082fd6716af3fc > src/slave/main.cpp 9d48a0823189ea6505073a2803f02d90dc382ab4 > src/tests/anonymous_tests.cpp bc29ff8be94af82dd97f43db4f9594036705e835 > src/tests/module.hpp 8e92774ddd51bc8a1368fb1cf6546300696b2d22 > src/tests/module.cpp 7968519996ca9f9d8895e73d5f173d26a7e794e0 > > Diff: https://reviews.apache.org/r/41760/diff/ > > > Testing > --- > > Unit tests pass. > > Also implemented in the > [execute-module](http://github.com/massenz/execute-module) - and it works > there too: > ``` > I0102 17:38:26.180788 19971 main.cpp:272] Initializing anonymous module > 'com_alertavert_mesos_RemoteExecution' > I0102 17:38:26.180800 19971 main.cpp:278] Sending flags to module > 'com_alertavert_mesos_RemoteExecution' > I0102 17:38:26.180810 19971 execute_module.hpp:129] Executing initialize() > for module; parsing runtime flags > I0102 17:38:26.181658 19971 execute_module.hpp:139] Configured work dir to > [/tmp/agent] and Sandbox dir to [/mnt/mesos/sandbox] > ``` > > > Thanks, > > Marco Massenzio > >
Re: Review Request 41760: Add initialization method to Anonymous module
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41760/ --- (Updated Jan. 4, 2016, 8:16 p.m.) Review request for mesos, Anand Mazumdar and Kapil Arya. Summary (updated) - Add initialization method to Anonymous module Bugs: MESOS-4253 https://issues.apache.org/jira/browse/MESOS-4253 Repository: mesos Description --- Jira: [MESOS-4253](https://issues.apache.org/jira/browse/MESOS-4253) To provide a basic "context" to Anonymous modules, we pass in the `BaseFlags`. This tries to be backward compatible with existing modules, so a no-op (`virtual`) implementation of the method is provided in the base class. Currently the code is only implemented in the Agent's `main()` method, but if deemed correct, adding it to Master is trivial. Diffs - include/mesos/module/anonymous.hpp 629eb123b9d5fa9e07ddb1c704ee72e96e8fec5b src/examples/test_anonymous_module.hpp 3da33a42f04b7421cee8fbb85e29b66e352f5894 src/examples/test_anonymous_module.cpp dd291cff3b5d47337e371cd2c1082fd6716af3fc src/slave/main.cpp 9d48a0823189ea6505073a2803f02d90dc382ab4 src/tests/anonymous_tests.cpp bc29ff8be94af82dd97f43db4f9594036705e835 src/tests/module.hpp 8e92774ddd51bc8a1368fb1cf6546300696b2d22 src/tests/module.cpp 7968519996ca9f9d8895e73d5f173d26a7e794e0 Diff: https://reviews.apache.org/r/41760/diff/ Testing --- Unit tests pass. Also implemented in the [execute-module](http://github.com/massenz/execute-module) - and it works there too: ``` I0102 17:38:26.180788 19971 main.cpp:272] Initializing anonymous module 'com_alertavert_mesos_RemoteExecution' I0102 17:38:26.180800 19971 main.cpp:278] Sending flags to module 'com_alertavert_mesos_RemoteExecution' I0102 17:38:26.180810 19971 execute_module.hpp:129] Executing initialize() for module; parsing runtime flags I0102 17:38:26.181658 19971 execute_module.hpp:139] Configured work dir to [/tmp/agent] and Sandbox dir to [/mnt/mesos/sandbox] ``` Thanks, Marco Massenzio
Re: Review Request 41760: [WIP] Add init(const FlagsBase&) to Anonymous module
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41760/ --- (Updated Jan. 3, 2016, 1:40 a.m.) Review request for mesos, Anand Mazumdar and Kapil Arya. Bugs: MESOS-4253 https://issues.apache.org/jira/browse/MESOS-4253 Repository: mesos Description --- Jira: [MESOS-4253](https://issues.apache.org/jira/browse/MESOS-4253) To provide a basic "context" to Anonymous modules, we pass in the `BaseFlags`. This tries to be backward compatible with existing modules, so a no-op (`virtual`) implementation of the method is provided in the base class. Currently the code is only implemented in the Agent's `main()` method, but if deemed correct, adding it to Master is trivial. Diffs - include/mesos/module/anonymous.hpp 629eb123b9d5fa9e07ddb1c704ee72e96e8fec5b src/examples/test_anonymous_module.hpp 3da33a42f04b7421cee8fbb85e29b66e352f5894 src/examples/test_anonymous_module.cpp dd291cff3b5d47337e371cd2c1082fd6716af3fc src/slave/main.cpp 9d48a0823189ea6505073a2803f02d90dc382ab4 src/tests/anonymous_tests.cpp bc29ff8be94af82dd97f43db4f9594036705e835 src/tests/module.hpp 8e92774ddd51bc8a1368fb1cf6546300696b2d22 src/tests/module.cpp 7968519996ca9f9d8895e73d5f173d26a7e794e0 Diff: https://reviews.apache.org/r/41760/diff/ Testing (updated) --- Unit tests pass. Also implemented in the [execute-module](http://github.com/massenz/execute-module) - and it works there too: ``` I0102 17:38:26.180788 19971 main.cpp:272] Initializing anonymous module 'com_alertavert_mesos_RemoteExecution' I0102 17:38:26.180800 19971 main.cpp:278] Sending flags to module 'com_alertavert_mesos_RemoteExecution' I0102 17:38:26.180810 19971 execute_module.hpp:129] Executing initialize() for module; parsing runtime flags I0102 17:38:26.181658 19971 execute_module.hpp:139] Configured work dir to [/tmp/agent] and Sandbox dir to [/mnt/mesos/sandbox] ``` Thanks, Marco Massenzio
Re: Review Request 41760: [WIP] Add init(const FlagsBase&) to Anonymous module
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41760/ --- (Updated Jan. 3, 2016, 1:29 a.m.) Review request for mesos, Anand Mazumdar and Kapil Arya. Changes --- Changed method name to `initialize()` to be consistent with other initialization methods. Repository: mesos Description --- Jira: [MESOS-4253](https://issues.apache.org/jira/browse/MESOS-4253) To provide a basic "context" to Anonymous modules, we pass in the `BaseFlags`. This tries to be backward compatible with existing modules, so a no-op (`virtual`) implementation of the method is provided in the base class. Currently the code is only implemented in the Agent's `main()` method, but if deemed correct, adding it to Master is trivial. Diffs (updated) - include/mesos/module/anonymous.hpp 629eb123b9d5fa9e07ddb1c704ee72e96e8fec5b src/examples/test_anonymous_module.hpp 3da33a42f04b7421cee8fbb85e29b66e352f5894 src/examples/test_anonymous_module.cpp dd291cff3b5d47337e371cd2c1082fd6716af3fc src/slave/main.cpp 9d48a0823189ea6505073a2803f02d90dc382ab4 src/tests/anonymous_tests.cpp bc29ff8be94af82dd97f43db4f9594036705e835 src/tests/module.hpp 8e92774ddd51bc8a1368fb1cf6546300696b2d22 src/tests/module.cpp 7968519996ca9f9d8895e73d5f173d26a7e794e0 Diff: https://reviews.apache.org/r/41760/diff/ Testing --- Unit tests pass. Also implemented in the [execute-module](http://github.com/massenz/execute-module) - and it works there too: ``` I1228 17:40:37.769500 26127 main.cpp:273] Initializing anonymous module 'com_alertavert_mesos_RemoteExecution' I1228 17:40:37.769516 26127 main.cpp:279] Sending flags to module 'com_alertavert_mesos_RemoteExecution' I1228 17:40:37.769532 26127 execute_module.cpp:259] >>>>> init() I1228 17:40:37.769552 26127 execute_module.cpp:262] advertise_ip: None I1228 17:40:37.769567 26127 execute_module.cpp:262] advertise_port: None I1228 17:40:37.769585 26127 execute_module.cpp:262] appc_store_dir: /tmp/mesos/store/appc I1228 17:40:37.769603 26127 execute_module.cpp:262] attributes: None I1228 17:40:37.769632 26127 execute_module.cpp:262] authenticatee: crammd5 ... I1228 17:40:37.771070 26127 execute_module.cpp:262] version: false I1228 17:40:37.771086 26127 execute_module.cpp:262] work_dir: /tmp/agent I1228 17:40:37.771389 26127 main.cpp:307] Starting Mesos slave ... ``` Thanks, Marco Massenzio
Review Request 41854: Added module initialization to Master main() method
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41854/ --- Review request for mesos, Anand Mazumdar and Kapil Arya. Bugs: MESOS-4253 https://issues.apache.org/jira/browse/MESOS-4253 Repository: mesos Description --- Added module initialization to Master main() method Diffs - src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c src/slave/main.cpp 9d48a0823189ea6505073a2803f02d90dc382ab4 Diff: https://reviews.apache.org/r/41854/diff/ Testing --- All unit tests pass. Thanks, Marco Massenzio
Re: Review Request 41760: [WIP] Add init(const FlagsBase&) to Anonymous module
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41760/ --- (Updated Jan. 3, 2016, 1:33 a.m.) Review request for mesos, Anand Mazumdar and Kapil Arya. Bugs: MESOS-4253 https://issues.apache.org/jira/browse/MESOS-4253 Repository: mesos Description --- Jira: [MESOS-4253](https://issues.apache.org/jira/browse/MESOS-4253) To provide a basic "context" to Anonymous modules, we pass in the `BaseFlags`. This tries to be backward compatible with existing modules, so a no-op (`virtual`) implementation of the method is provided in the base class. Currently the code is only implemented in the Agent's `main()` method, but if deemed correct, adding it to Master is trivial. Diffs - include/mesos/module/anonymous.hpp 629eb123b9d5fa9e07ddb1c704ee72e96e8fec5b src/examples/test_anonymous_module.hpp 3da33a42f04b7421cee8fbb85e29b66e352f5894 src/examples/test_anonymous_module.cpp dd291cff3b5d47337e371cd2c1082fd6716af3fc src/slave/main.cpp 9d48a0823189ea6505073a2803f02d90dc382ab4 src/tests/anonymous_tests.cpp bc29ff8be94af82dd97f43db4f9594036705e835 src/tests/module.hpp 8e92774ddd51bc8a1368fb1cf6546300696b2d22 src/tests/module.cpp 7968519996ca9f9d8895e73d5f173d26a7e794e0 Diff: https://reviews.apache.org/r/41760/diff/ Testing --- Unit tests pass. Also implemented in the [execute-module](http://github.com/massenz/execute-module) - and it works there too: ``` I1228 17:40:37.769500 26127 main.cpp:273] Initializing anonymous module 'com_alertavert_mesos_RemoteExecution' I1228 17:40:37.769516 26127 main.cpp:279] Sending flags to module 'com_alertavert_mesos_RemoteExecution' I1228 17:40:37.769532 26127 execute_module.cpp:259] >>>>> init() I1228 17:40:37.769552 26127 execute_module.cpp:262] advertise_ip: None I1228 17:40:37.769567 26127 execute_module.cpp:262] advertise_port: None I1228 17:40:37.769585 26127 execute_module.cpp:262] appc_store_dir: /tmp/mesos/store/appc I1228 17:40:37.769603 26127 execute_module.cpp:262] attributes: None I1228 17:40:37.769632 26127 execute_module.cpp:262] authenticatee: crammd5 ... I1228 17:40:37.771070 26127 execute_module.cpp:262] version: false I1228 17:40:37.771086 26127 execute_module.cpp:262] work_dir: /tmp/agent I1228 17:40:37.771389 26127 main.cpp:307] Starting Mesos slave ... ``` Thanks, Marco Massenzio
Re: Review Request 40445: Added linter for license headers in some file types.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40445/#review108054 --- support/mesos-license.py (line 29) <https://reviews.apache.org/r/40445/#comment167428> best not to have spaces in the logger's name support/mesos-license.py (line 30) <https://reviews.apache.org/r/40445/#comment167429> is this dead code? please remove However, I'd like to see the log-level to be set to INFO by defaul and driven by customer option (eg, a --debug or -v,--verbose flag) to be DEBUG on-demand. support/mesos-license.py (line 36) <https://reviews.apache.org/r/40445/#comment167430> blank line between method definition IMO both methods are redundant, but please derive it from `Exception` (instead of `BaseEx`) unless you have a good reason for it. support/mesos-license.py (line 39) <https://reviews.apache.org/r/40445/#comment167431> please add epydoc to the method and explain what the @param is supposed to be (also @type) support/mesos-license.py (lines 43 - 47) <https://reviews.apache.org/r/40445/#comment167537> this is a bit on the ugly side of things :) and not very readable either (not to mention, not very extensible). Ideally, on would use a combination of RegEx and a comprehension list. - Marco Massenzio On Nov. 25, 2015, 9:59 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40445/ > --- > > (Updated Nov. 25, 2015, 9:59 a.m.) > > > Review request for mesos, Benjamin Hindman, Marco Massenzio, and Michael Park. > > > Bugs: MESOS-3581 > https://issues.apache.org/jira/browse/MESOS-3581 > > > Repository: mesos > > > Description > --- > > Added linter for license headers in some file types. > > > Diffs > - > > support/mesos-license.py PRE-CREATION > support/mesos-style.py 66b45692c3c04f68358b63d52e4d87934f241bd7 > > Diff: https://reviews.apache.org/r/40445/diff/ > > > Testing > --- > > Ran the a whole clean checkout through the linter with only one expected > failure (`3rdparty/libprocess/stout/tests/protobuf_tests.proto` which lacks a > license). > > > NOTE TO THE COMMITTER > - > > Before committing this, it is probably a good idea to check the whole code > base again and fix any new files which do not follow the current license > style. The commits which originally fixed this were > > * fa36917 (mesos), > * dc23756 (stout), and > * 3539b7a (libprocess). > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 37336: Added `execute()` method to process::Subprocess
20, 2015, 10:12 p.m., Jie Yu wrote: > > 3rdparty/libprocess/src/subprocess.cpp, lines 185-190 > > <https://reviews.apache.org/r/37336/diff/11/?file=1122403#file1122403line185> > > > > What if outData is called more than once? Great question! I would guess they'll get an empty string (as I assume that `io::read()` is sitting on an empty buffer) but worth looking into (and testing!) Thanks. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/#review107421 --- On Nov. 10, 2015, 8:51 p.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37336/ > --- > > (Updated Nov. 10, 2015, 8:51 p.m.) > > > Review request for mesos, Joris Van Remoortere and Michael Park. > > > Bugs: MESOS-3035 > https://issues.apache.org/jira/browse/MESOS-3035 > > > Repository: mesos > > > Description > --- > > The original API for `process::Subprocess` still left a lot of legwork > to do for the caller; we have now added an `execute()` method > that returns a `Future`. > > `Subprocess::Result`, also introduced with this patch, contains useful > information > about the command invocation (an `Invocation` struct); the exit code; > `stdout`; > and, optionally, `stderr` too. > > Once the Future completes, if successful, the caller will be able to retrieve > stdout/stderr; whether the command was successful; and whether it received a > signal > > > Diffs > - > > 3rdparty/libprocess/include/process/subprocess.hpp > f17816e813d5efce1d3bb1ff1e850eeda3ba > 3rdparty/libprocess/src/subprocess.cpp > efe0018d0414c4137fd833c153eb262232e712bc > 3rdparty/libprocess/src/tests/subprocess_tests.cpp > ac600a551fb1a7782ff33cce204b7819497ef54a > > Diff: https://reviews.apache.org/r/37336/diff/ > > > Testing > --- > > make check > > (also tested functionality with an anonymous module that exposes an > `/execute` endpoint and runs arbitrary commands, asynchronously, > on an Agent) > > > Thanks, > > Marco Massenzio > >
Re: Review Request 37336: Added `execute()` method to process::Subprocess
> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 53-75
> > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line53>
> >
> > What's the motivation of storing this? Should the caller already have
> > those information?
>
> Marco Massenzio wrote:
> not necessarily - please note that this is executed asynchronously, so
> the caller may have several of them running concurrently and having the
> invocation returned together with the invocation will greatly simplifly the
> caller's code.
>
> The whole point of this patch is to make the caller's API less
> "low-level" and simplify the life of those using this facility.
>
> I have, for example, a module that executes commands upon an HTTP request
> - having the `invocation` returned to me with the result, greatly simplifies
> my code and enables me to return a Response without waiting for execution.
>
> Jie Yu wrote:
> From what I can tell from the code you pasted below, doesn't seem to
> simplify the caller's code *a lot*.
Point well taken, but I'm confused here, Jie - on the one hand, you here seem
to be unhappy it does not simplify enough, then below you suggest it simplifies
too much.
The point here is - BenH and I went over the current usages of `subprocess()`
and saw a common pattern, and agreed on a way that would enable us to (a) cover
99% of usage patterns and (b) avoid the same boilerplate code to be repeated
over and over again.
Three months on, and countless reviews after, this is what we arrived at: it
would be good, at some point in time, to accept that this is **one** way of
doing things, among countless others - and I'd like this code committed.
Unless we all agree that it's either buggy/flawed/wrong, in which case, I'm
happy to discard this patch and re-start from scratch.
> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 119-130
> > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line119>
> >
> > Why not just use a single `int status` field here. The users can use
> > WEXITSTATUS ... to derive those information themself. This is also
> > consistent with other places in the code place.
>
> Marco Massenzio wrote:
> `Subprocess` does exactly that (actually, it uses an `Option`) -
> this leaves the caller with the burned to do all the legwork: again and again
> and again - it's all over the code base.
>
> The point of this patch is to encapsulate that work, having it done
> (hopefully, properly) in **one** place and avoid to have the same code
> sprinkled all over the codebase (and you can tell, most places, by copy &
> paste).
>
> Jie Yu wrote:
> What if there's no returnCode (due to signal)? Should you use Option
> returnCode here? Similarly, should you use Option for signal as well.
> What will the client code be look like? Do you still need to check if
> (returnCode.isSome()) ... if (signal.isSome()) ...
>
> Also
> 1) what if I want to know if WCOREDUMP is true or not, do you need to add
> another boolean?
> 2) what if the reaper cannot reap the subprocess (i.e., we cannot get the
> exit status)?
Please see the usage in the tests (this same patch).
`returnCode` is EXIT_FAILURE and `signal` is non-zero - in case one cares
(which most folks don't seems to, but whatever).
if we can't get the exit status (`Subprocess::status` is `None()`) we return a
`Failure` on the returned future and an error message (which is what ultimately
is done practically everywhere `subprocess()` is used - while I guess I
could've dreamt up tens of different scenarios, usage is pretty consistent in
Mesos codebase and is what we're trying to encapsulate here).
Note that `Subprocess::status` is still available to callers.
> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 328
> > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line328>
> >
> > I don't like the name 'execute'. When you create the Subprocess
> > instance, the subprocss is already launched and exec'ed. This is rather
> > waiting for the subprocess to terminate.
>
> Marco Massenzio wrote:
> This method most definitely does **not** wait.
>
> This is how one can use it as a caller (code simplified):
> ```
> auto s = process::subprocess(commandInfo.command(), args);
>
> if (s.isError()) {
> LOG(ERROR) << "Could not spawn subprocess: " << s.error();
> return http::ServiceUnavailable(s.error());
>
Re: Review Request 40340: [WIP] Windows: Added Windows support to `support/post-reviews.py`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40340/#review107112 --- Good stuff! Sorry for the delay in reviewing this: only got halfway through - will finish it off on the train. support/cpplint.py (line 4734) <https://reviews.apache.org/r/40340/#comment166050> function names should be `lower_case` according to [PEP8](https://www.python.org/dev/peps/pep-0008/#function-names) support/cpplint.py (lines 4741 - 4745) <https://reviews.apache.org/r/40340/#comment166051> can we use a format that Python automated doc tools will understand? `@param` / `@type` / `@return` or the `:param` equivalent. support/cpplint.py (line 4763) <https://reviews.apache.org/r/40340/#comment166052> not your code, admittedly, but it would be awesome to drag our Python scripts into the 21st century and use `argparse` instead :) support/cpplint.py (line 4766) <https://reviews.apache.org/r/40340/#comment166053> this does not make any sense at all, IMO (do we really have a wrapper script that checks `$?` and echoes the number of errors?) support/mesos-style.py (line 11) <https://reviews.apache.org/r/40340/#comment166056> I am not familiar with `cStringIO` - the name would seem to indicate (a) it's not a standard library and (b) that it requires some amount of C compilation. This is usually a pain "on some OS" (eg, OSX) - can you please confirm whether it's in the standard libraries and, if not, what are the steps involved in getting it installed on various OSes? This may break building/committing everywhere, right? support/mesos-style.py (line 63) <https://reviews.apache.org/r/40340/#comment166054> I have a suspicion it's just *one* OS that does not support this, but lets' not be picky here :) support/mesos-style.py (lines 71 - 76) <https://reviews.apache.org/r/40340/#comment166055> same comment as before support/mesos-style.py (line 97) <https://reviews.apache.org/r/40340/#comment166057> unnecessary parentheses - Marco Massenzio On Nov. 16, 2015, 9:25 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40340/ > --- > > (Updated Nov. 16, 2015, 9:25 a.m.) > > > Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van > Remoortere, Joseph Wu, and Marco Massenzio. > > > Repository: mesos > > > Description > --- > > Windows: Added Windows support to `support/post-reviews.py`. > > Early draft of the update to the Windows-compatible `post-reviews.py`. I'd > like to get early feedback from people in case this is entirely the wrong > direction. > > At the outset, I will say that the "right way" to do this would be to > refactor `cppylint.py` and `post-reviews.py` entirely so that they are more > modular (for example, make `post-reviews.py` function-oriented, instead of a > pile of statements and expressions, and `cpplint.py` should be a module > proper). I didn't do this the "right way" because it is blocking for our > Windows friends. If it's really important to do this the "right way", we may > need to break it into a couple reviews. > > Note that the most questionable part of this is the point at which we > redirect `stderr` in `cpplint.py` to a string capture it for use in > `post-reviews.py`. This is certainly the quickest way to do it, but it is not > the best -- the best way would be to refactor `cpplint.py` to be a module, at > least, and to handle error logging in a more pluggable way. We aimed for the > shortest diff because there are no tests for any of these scripts, and to be > honest, I was afraid of changing them without tests. > > > Diffs > - > > .gitignore-template 90b6697d19a5e0a68805b23b587b362731a1df25 > support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1 > support/mesos-style.py 66b45692c3c04f68358b63d52e4d87934f241bd7 > support/post-reviews.py 170be83aa6dca6e8175292169d78e8f7915f7e6e > > Diff: https://reviews.apache.org/r/40340/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 40340: [WIP] Windows: Added Windows support to `support/post-reviews.py`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40340/#review107116 --- support/mesos-style.py (line 104) <https://reviews.apache.org/r/40340/#comment166064> unnecessary parentheses support/mesos-style.py (lines 105 - 106) <https://reviews.apache.org/r/40340/#comment166065> Mesos-style indenting doesn't apply (I think) here: ``` errors_found, captured_stderr = lint_and_capture_stderr( rules_filter, source_paths) ``` support/mesos-style.py (line 112) <https://reviews.apache.org/r/40340/#comment166066> not your code, but could you please remove the unnecessary `is not None` support/mesos-style.py (line 113) <https://reviews.apache.org/r/40340/#comment166067> what was wrong with: `print(line)` (and please be a good man, if it's not already there, can you please add -at the very top of this file-): ``` from __future__ import print_function ``` so this works with both 2.7 and 3.x? support/mesos-style.py (line 134) <https://reviews.apache.org/r/40340/#comment166069> +10 thanks! support/post-reviews.py (line 71) <https://reviews.apache.org/r/40340/#comment166070> missing "user" also note that epydoc support markdown; I would enumerate the various options as a bulleted list. ``` can respond with: - foo - bar - ... ``` support/post-reviews.py (lines 71 - 73) <https://reviews.apache.org/r/40340/#comment166071> is the user input case-insensitive? if not, worth mentioning support/post-reviews.py (lines 81 - 91) <https://reviews.apache.org/r/40340/#comment166072> +1 this is awesome - I wish all our methods were documented in this depth! support/post-reviews.py (line 106) <https://reviews.apache.org/r/40340/#comment166073> ``` while not choice in ['y', 'n', ctrl_d]: ``` - Marco Massenzio On Nov. 16, 2015, 9:25 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40340/ > --- > > (Updated Nov. 16, 2015, 9:25 a.m.) > > > Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van > Remoortere, Joseph Wu, and Marco Massenzio. > > > Repository: mesos > > > Description > --- > > Windows: Added Windows support to `support/post-reviews.py`. > > Early draft of the update to the Windows-compatible `post-reviews.py`. I'd > like to get early feedback from people in case this is entirely the wrong > direction. > > At the outset, I will say that the "right way" to do this would be to > refactor `cppylint.py` and `post-reviews.py` entirely so that they are more > modular (for example, make `post-reviews.py` function-oriented, instead of a > pile of statements and expressions, and `cpplint.py` should be a module > proper). I didn't do this the "right way" because it is blocking for our > Windows friends. If it's really important to do this the "right way", we may > need to break it into a couple reviews. > > Note that the most questionable part of this is the point at which we > redirect `stderr` in `cpplint.py` to a string capture it for use in > `post-reviews.py`. This is certainly the quickest way to do it, but it is not > the best -- the best way would be to refactor `cpplint.py` to be a module, at > least, and to handle error logging in a more pluggable way. We aimed for the > shortest diff because there are no tests for any of these scripts, and to be > honest, I was afraid of changing them without tests. > > > Diffs > - > > .gitignore-template 90b6697d19a5e0a68805b23b587b362731a1df25 > support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1 > support/mesos-style.py 66b45692c3c04f68358b63d52e4d87934f241bd7 > support/post-reviews.py 170be83aa6dca6e8175292169d78e8f7915f7e6e > > Diff: https://reviews.apache.org/r/40340/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 39410: Added support for github to apply-reviews.py.
> On Nov. 10, 2015, 4:11 a.m., Kapil Arya wrote:
> > support/apply-reviews.py, lines 196-197
> > <https://reviews.apache.org/r/39410/diff/13/?file=1119828#file1119828line196>
> >
> > Can we enhance it as following:
> > ```
> > amend=options['no_amend'] ? '' : '-e'
> > cmd = 'git commit --author \'{author}\' {amend} -am \'{message}\''\
> > .format(author=quote(data['author']),
> > amend=amend,
> > message=quote(data['message']))
> > ```
> >
> > With this, we can now get rid of `amend_patch()`. This will speed up
> > the process since we don't have to call `git commit --amend`. As we know, a
> > call to `git commit --amend` without any staged file forces a full run of
> > mesos-style.py, which can take several seconds.
>
> Artem Harutyunyan wrote:
> TIL Python does not have a ternary operator, but point taken :).
Well, it has something that gets close enough :)
```
amend = '-e' if not options.get('no_amend') else ''
```
which I actually find more readable than the `? :` operator (and, apparently,
Guido does too ;) )
- Marco
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39410/#review105786
---
On Nov. 11, 2015, 10:10 a.m., Artem Harutyunyan wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39410/
> -------
>
> (Updated Nov. 11, 2015, 10:10 a.m.)
>
>
> Review request for mesos, Adam B, Joris Van Remoortere, Joseph Wu, Marco
> Massenzio, and Vinod Kone.
>
>
> Bugs: MESOS-3859
> https://issues.apache.org/jira/browse/MESOS-3859
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Added support for github to apply-reviews.py.
>
>
> Diffs
> -
>
> support/apply-reviews.py d39ee9bb0ee782bd756b7a5fc0dec70d056c9589
>
> Diff: https://reviews.apache.org/r/39410/diff/
>
>
> Testing
> ---
>
> Tested with python 2.7
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 37336: Simplified the caller interface to process::Subprocess
> On Nov. 7, 2015, 1:06 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 91
> > <https://reviews.apache.org/r/37336/diff/10/?file=1117802#file1117802line91>
> >
> > How come `Invocation` is within `Subprocess::Result`? Wouldn't it make
> > more sense for it to be at the `Subprocess` level to be
> > `Subprocess::Invocation`?
I probably misunderstood your earlier comment.
Moved one level up, into `Subprocess`.
> On Nov. 7, 2015, 1:06 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 278
> > <https://reviews.apache.org/r/37336/diff/10/?file=1117802#file1117802line278>
> >
> > We tend to not use `getX()` style naming for accessors. How about
> > `outData()`?
great point (wasn't sure, as it's not spelled out in our style guide - and it's
not truly a "getter" in the Google sytle sense: `foo_` -> `foo()`)
also renamed `stderr_` to `errData` and `getStderr()` to `errData()` - please
let me know if that was "overreach"!
> On Nov. 7, 2015, 1:06 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 340
> > <https://reviews.apache.org/r/37336/diff/10/?file=1117802#file1117802line340>
> >
> > I think this violates our use of `auto` rule. Could you please spell
> > out the type?
I would disagree here... the type is `Try<std::list>` and
adding that, I don't think makes the code any more readable (on the contrary).
In fact, we don't really make use of any of that - all we care is that no error
is returned: it could very well be a `Try` for all we care :)
This seems to me a poster child of the use-case for `auto`... anyways, no
biggie, I've added the type.
> On Nov. 7, 2015, 1:06 a.m., Michael Park wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 202-203
> > <https://reviews.apache.org/r/37336/diff/10/?file=1117803#file1117803line202>
> >
> > Why do we need the `promise` stuff here?
> >
> > Does something like the following not work?
> >
> > ```cpp
> > Future<tuple<Future<Option>, Future, Future>>
> > result =
> > await(status(), getStdout(), getStderr());
> >
> > return result.then([=](...) { ... })
> > .onFailed([=](...) { ... })
> > .onDiscard([this]() { cleanup(); });
> > ```
I don't quite remember what it was that I came across originally when I tried
the above (sans Promise) then Joris suggested the pattern that you see here
now, and this works. IIRC I could never get the `onFailed` to get invoked for
failed commands, so the pending process never got cleaned up.
If you feel strongly this should not be here (and/or are confident the pattern
above works) I'm happy to have another go at it.
- Marco
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37336/#review105544
---
On Nov. 6, 2015, 6:24 a.m., Marco Massenzio wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> ---
>
> (Updated Nov. 6, 2015, 6:24 a.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Michael Park.
>
>
> Bugs: MESOS-3035
> https://issues.apache.org/jira/browse/MESOS-3035
>
>
> Repository: mesos
>
>
> Description
> ---
>
> The original API for `process::Subprocess` still left a lot of legwork
> to do for the caller; we have now added an `execute()` method
> that returns a `Future`.
>
> `Subprocess::Result`, also introduced with this patch, contains useful
> information
> about the command invocation (an `Invocation` struct); the exit code;
> `stdout`;
> and, optionally, `stderr` too.
>
> Once the Future completes, if successful, the caller will be able to retrieve
> stdout/stderr; whether the command was successful; and whether it received a
> signal
>
>
> Diffs
> -
>
> 3rdparty/libprocess/include/process/subprocess.hpp
> f17816e813d5efce1d3bb1ff1e850eeda3ba
> 3rdparty/libprocess/src/subprocess.cpp
> efe0018d0414c4137fd833c153eb262232e712bc
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp
> ac600a551fb1a7782ff33cce204b7819497ef54a
>
> Diff: https://reviews.apache.org/r/37336/diff/
>
>
> Testing
> ---
>
> make check
>
> (also tested functionality with an anonymous module that exposes an
> `/execute` endpoint and runs arbitrary commands, asynchronously,
> on an Agent)
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 37336: Added `execute()` method to process::Subprocess
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/ --- (Updated Nov. 10, 2015, 8:51 p.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Addressed mpark's comments Summary (updated) - Added `execute()` method to process::Subprocess Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description --- The original API for `process::Subprocess` still left a lot of legwork to do for the caller; we have now added an `execute()` method that returns a `Future`. `Subprocess::Result`, also introduced with this patch, contains useful information about the command invocation (an `Invocation` struct); the exit code; `stdout`; and, optionally, `stderr` too. Once the Future completes, if successful, the caller will be able to retrieve stdout/stderr; whether the command was successful; and whether it received a signal Diffs (updated) - 3rdparty/libprocess/include/process/subprocess.hpp f17816e813d5efce1d3bb1ff1e850eeda3ba 3rdparty/libprocess/src/subprocess.cpp efe0018d0414c4137fd833c153eb262232e712bc 3rdparty/libprocess/src/tests/subprocess_tests.cpp ac600a551fb1a7782ff33cce204b7819497ef54a Diff: https://reviews.apache.org/r/37336/diff/ Testing --- make check (also tested functionality with an anonymous module that exposes an `/execute` endpoint and runs arbitrary commands, asynchronously, on an Agent) Thanks, Marco Massenzio
Re: Review Request 39452: MESOS-3566 Description of RecordIO format
> On Nov. 7, 2015, 1:15 a.m., Mesos ReviewBot wrote: > > Bad patch! > > > > Reviews applied: [39452] > > > > Failed command: ./support/apply-review.sh -n -r 39452 > > > > Error: > > 2015-11-07 01:15:41 URL:https://reviews.apache.org/r/39452/diff/raw/ > > [2542/2542] -> "39452.patch" [1] > > error: docs/scheduler-http-api.md: does not exist in index > > Failed to apply patch uhm... what does this even mean? - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39452/#review10 --- On Nov. 6, 2015, 8:35 p.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39452/ > --- > > (Updated Nov. 6, 2015, 8:35 p.m.) > > > Review request for mesos, Anand Mazumdar and Vinod Kone. > > > Bugs: MESOS-3566 > https://issues.apache.org/jira/browse/MESOS-3566 > > > Repository: mesos > > > Description > --- > > Added the description of the RecordIO format to the HTTP API > document with example code (Python) to decode. > > > Diffs > - > > docs/scheduler-http-api.md de6cfc9e009a857ca45291b2dadce2a3b8199787 > > Diff: https://reviews.apache.org/r/39452/diff/ > > > Testing > --- > > Built the site & tested using: > https://github.com/apache/mesos/tree/master/support/site-docker > > > Thanks, > > Marco Massenzio > >
Re: Review Request 39452: MESOS-3566 Description of RecordIO format
> On Nov. 6, 2015, 8:53 a.m., Adam B wrote: > > Please verify your test results from rendering the website and then I'll > > commit this. thanks for pointers, done! > On Nov. 6, 2015, 8:53 a.m., Adam B wrote: > > docs/scheduler-http-api.md, line 31 > > <https://reviews.apache.org/r/39452/diff/6/?file=1117450#file1117450line31> > > > > s/to be/would be/? or "is"? "is" - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39452/#review105385 ------- On Nov. 6, 2015, 7:45 p.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39452/ > --- > > (Updated Nov. 6, 2015, 7:45 p.m.) > > > Review request for mesos, Anand Mazumdar and Vinod Kone. > > > Bugs: MESOS-3566 > https://issues.apache.org/jira/browse/MESOS-3566 > > > Repository: mesos > > > Description > --- > > Added the description of the RecordIO format to the HTTP API > document with example code (Python) to decode. > > > Diffs > - > > docs/scheduler-http-api.md de6cfc9e009a857ca45291b2dadce2a3b8199787 > > Diff: https://reviews.apache.org/r/39452/diff/ > > > Testing > --- > > Built the site & tested using: > https://github.com/apache/mesos/tree/master/support/site-docker > > > Thanks, > > Marco Massenzio > >
Re: Review Request 39452: MESOS-3566 Description of RecordIO format
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39452/ --- (Updated Nov. 6, 2015, 7:45 p.m.) Review request for mesos, Anand Mazumdar and Vinod Kone. Bugs: MESOS-3566 https://issues.apache.org/jira/browse/MESOS-3566 Repository: mesos Description --- Added the description of the RecordIO format to the HTTP API document with example code (Python) to decode. Diffs - docs/scheduler-http-api.md de6cfc9e009a857ca45291b2dadce2a3b8199787 Diff: https://reviews.apache.org/r/39452/diff/ Testing (updated) --- Built the site & tested using: https://github.com/apache/mesos/tree/master/support/site-docker Thanks, Marco Massenzio
Re: Review Request 39452: MESOS-3566 Description of RecordIO format
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39452/ --- (Updated Nov. 6, 2015, 8:35 p.m.) Review request for mesos, Anand Mazumdar and Vinod Kone. Changes --- Addressed comments / tested on the site-builder Bugs: MESOS-3566 https://issues.apache.org/jira/browse/MESOS-3566 Repository: mesos Description --- Added the description of the RecordIO format to the HTTP API document with example code (Python) to decode. Diffs (updated) - docs/scheduler-http-api.md de6cfc9e009a857ca45291b2dadce2a3b8199787 Diff: https://reviews.apache.org/r/39452/diff/ Testing --- Built the site & tested using: https://github.com/apache/mesos/tree/master/support/site-docker Thanks, Marco Massenzio
Re: Review Request 39452: MESOS-3566 Description of RecordIO format
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39452/ --- (Updated Nov. 5, 2015, 11:50 p.m.) Review request for mesos, Anand Mazumdar and Vinod Kone. Changes --- Addressed all comments. Bugs: MESOS-3566 https://issues.apache.org/jira/browse/MESOS-3566 Repository: mesos Description --- Added the description of the RecordIO format to the HTTP API document with example code (Python) to decode. Diffs (updated) - docs/scheduler-http-api.md de6cfc9e009a857ca45291b2dadce2a3b8199787 Diff: https://reviews.apache.org/r/39452/diff/ Testing --- Thanks, Marco Massenzio
Re: Review Request 37336: Simplified the caller interface to process::Subprocess
> On Nov. 3, 2015, 12:55 a.m., Michael Park wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, line 56 > > <https://reviews.apache.org/r/37336/diff/9/?file=1113944#file1113944line56> > > > > Could you explain in what situations and why the list of arguments > > could/would be modified...? I think I should remove this comment (in fact, I just did): originally, I was hoping to be able to somehow fix the awkward situation in which `arg[0]` is just the `progname` and not actually used as a command argument: while this is "obvious" for any C/C++ developer, it is also undocumented in `os::shell` and leads to unexpected behavior for unaware users of `CommandInfo` (for example). It turns out that that's not possible without breaking a lot of existing code/frameworks, so I guess it's here to stay. > On Nov. 3, 2015, 12:55 a.m., Michael Park wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, line 120 > > <https://reviews.apache.org/r/37336/diff/9/?file=1113944#file1113944line120> > > > > This suggests to me either that `Command` should be named `Invocation`, > > or that this variable should be named `command`. Given that `Command` is > > storing both the command and the arguments, it seems more fitting to name > > it `Invocation`. > > > > Additionally, I think it would make a lot of sense to simply name it > > `Result` and move it into the `Subprocess` class. Similar to > > `Subprocess::IO`, we would have `Subprocess::Result` which captures the > > result of a subprocess call. > > > > What do you think? Done. Renamed `Command` -> `Invocation` Renamed `CommandResult` -> `Subprocess::Result` Thanks for the suggestion! (I was unhappy about naming too) > On Nov. 3, 2015, 12:55 a.m., Michael Park wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, lines 135-152 > > <https://reviews.apache.org/r/37336/diff/9/?file=1113944#file1113944line135> > > > > Could you explain why these aren't symmetric? That is, why is `stdout_` > > not a `Option` or why is `stderr_` not a `std::string`? It is safe to assume that every shell command will *always* emit to `stdout` (possibly an empty string) - while, not necessarily so to `stderr`. It is probably a "distinction without a difference" but I believe it makes the caller's code easier to write and removes unnecessary (and tedious) `.isSome()` and `.get()` for stdout; while allowing for the possibility that there is no `stderr` available. Obviously, we could make `stderr_` a `string` too - but making it an `Option` seemed to me to be more in line with Mesos' practices. > On Nov. 3, 2015, 12:55 a.m., Michael Park wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, line 400 > > <https://reviews.apache.org/r/37336/diff/9/?file=1113944#file1113944line400> > > > > Is there a reason why this is wrapped in a `std::shared_ptr`? yes, because I need to pass it to the lambda and not doing so seemed to cause issues (if memory serves). Wrapping it in a `shared_ptr` (instead of using a `raw` pointer) is also, in my understanding, "best practice" now in C++11? > On Nov. 3, 2015, 12:55 a.m., Michael Park wrote: > > 3rdparty/libprocess/src/subprocess.cpp, lines 499-503 > > <https://reviews.apache.org/r/37336/diff/9/?file=1113945#file1113945line499> > > > > Rather than exposing the `invokedWith_` member variable like this, can > > we introduce a `Subprocess` constructor that takes a `Command` instead? At > > which point we can do something like: > > > > `Subprocess process(path == "sh" ? Command(argv[2]) : Command(path, > > argv));` well, `invokedWith_` is already "exposed" to `subprocess` - as it's a 'friend' anyway. But I have simplified the invocation, so that it reflects closely what you suggested. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/#review104816 --- On Nov. 2, 2015, 7:22 a.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37336/ > --- > > (Updated Nov. 2, 2015, 7:22 a.m.) > > > Review request for mesos, Joris Van Remoortere and Michael Park. > > > Bugs: MESOS-3035 > https://issues.apache.org/jira/browse/MESOS-3035 > > > Repository: mesos > > > Description > --- > > Jira: MESOS-3035 >
Re: Review Request 37336: Simplified the caller interface to process::Subprocess
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/ --- (Updated Nov. 6, 2015, 6:24 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Addressed all of mpark comments Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description (updated) --- The original API for `process::Subprocess` still left a lot of legwork to do for the caller; we have now added an `execute()` method that returns a `Future`. `Subprocess::Result`, also introduced with this patch, contains useful information about the command invocation (an `Invocation` struct); the exit code; `stdout`; and, optionally, `stderr` too. Once the Future completes, if successful, the caller will be able to retrieve stdout/stderr; whether the command was successful; and whether it received a signal Diffs (updated) - 3rdparty/libprocess/include/process/subprocess.hpp f17816e813d5efce1d3bb1ff1e850eeda3ba 3rdparty/libprocess/src/subprocess.cpp efe0018d0414c4137fd833c153eb262232e712bc 3rdparty/libprocess/src/tests/subprocess_tests.cpp ac600a551fb1a7782ff33cce204b7819497ef54a Diff: https://reviews.apache.org/r/37336/diff/ Testing (updated) --- make check (also tested functionality with an anonymous module that exposes an `/execute` endpoint and runs arbitrary commands, asynchronously, on an Agent) Thanks, Marco Massenzio
Re: Review Request 37336: Simplified the caller interface to process::Subprocess
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/ --- (Updated Nov. 2, 2015, 7:22 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description --- Jira: MESOS-3035 The original API for `process::Subprocess` still left a lot of legwork to do for the caller; we have now added an `execute()` method that returns a `Future` (also introduced with this patch) which contains useful information about the command invocation; the exit code; stdout and, optionally, stderr too. Once the Future completes, if successful, the caller will be able to retrieve stdout/stderr; whether the command was successful; and whether it received a signal Diffs (updated) - 3rdparty/libprocess/include/process/subprocess.hpp f17816e813d5efce1d3bb1ff1e850eeda3ba 3rdparty/libprocess/src/subprocess.cpp 459825c188d56d25f6e2832e5a170d806e257d6b 3rdparty/libprocess/src/tests/subprocess_tests.cpp ac600a551fb1a7782ff33cce204b7819497ef54a Diff: https://reviews.apache.org/r/37336/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 39410: Added support for github to apply-reviews.py.
> On Oct. 20, 2015, 5:06 a.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 160
> > <https://reviews.apache.org/r/39410/diff/3/?file=1100642#file1100642line160>
> >
> > you don't really need to escape the quotes, just take advantage of
> > Python being able to use single and double quotes interchangeably (or even
> > use """ if you really want to be hip :)
>
> Artem Harutyunyan wrote:
> I actually do need to escape the quotes becasue {message} is multiline
> and I am executing the `cmd` in a shell.
>
> Marco Massenzio wrote:
> I don't understand your comment, I'm afraid. What I did mean was that you
> can write:
> ```
> cmd = "git commit --author '{author}' -am '{message}'".format(author =
> review['author'], ...)
> ```
> and this will have the exact same meaning as your code (whether that's
> correctly escaped and executed in the shell, I really can't say).
>
> Artem Harutyunyan wrote:
> I see what you're saying. I am using single quotes throughout the code,
> that's why I preferred quoting to switching to double quotes for these
> particular instances.
ah - that sounds reasonable (although, you'll be relieved to know that PEP8 is
silent on that point :)
The general approach is to use both, being consistent across "usage classes."
- Marco
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39410/#review103208
---
On Oct. 23, 2015, 6:18 a.m., Artem Harutyunyan wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39410/
> -------
>
> (Updated Oct. 23, 2015, 6:18 a.m.)
>
>
> Review request for mesos, Adam B, Joris Van Remoortere, Joseph Wu, Marco
> Massenzio, and Vinod Kone.
>
>
> Bugs: MESOS-3468
> https://issues.apache.org/jira/browse/MESOS-3468
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Added support for github to apply-reviews.py.
>
>
> Diffs
> -
>
> support/apply-reviews.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/39410/diff/
>
>
> Testing
> ---
>
> Tested with python 2.7
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script. Added support for amending commit messages.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38883/#review103778 --- Ship it! Ship It! - Marco Massenzio On Oct. 23, 2015, 6:18 a.m., Artem Harutyunyan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38883/ > --- > > (Updated Oct. 23, 2015, 6:18 a.m.) > > > Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, > and Vinod Kone. > > > Bugs: MESOS-3468 > https://issues.apache.org/jira/browse/MESOS-3468 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > support/apply-reviews.py PRE-CREATION > > Diff: https://reviews.apache.org/r/38883/diff/ > > > Testing > --- > > Tested the script with python 2.7. > > > Thanks, > > Artem Harutyunyan > >
Re: Review Request 39420: Added '--parent' option and made apply-review.sh call apply-reviews.py.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39420/#review103779 --- Ship it! Thanks for the "Better Bash" :) - Marco Massenzio On Oct. 23, 2015, 6:58 a.m., Artem Harutyunyan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39420/ > --- > > (Updated Oct. 23, 2015, 6:58 a.m.) > > > Review request for mesos, Joris Van Remoortere, Joseph Wu, Marco Massenzio, > and Vinod Kone. > > > Bugs: MESOS-3468 > https://issues.apache.org/jira/browse/MESOS-3468 > > > Repository: mesos > > > Description > --- > > Added '--parent' option and made apply-review.sh call apply-reviews.py. > > > Diffs > - > > support/apply-review.sh 6391451542e9e8847ec38e2ad9d9acf552afead3 > support/apply-reviews.py PRE-CREATION > > Diff: https://reviews.apache.org/r/39420/diff/ > > > Testing > --- > > Tested with python 2.7. > > - with and without '-p'. > - Tested reviews with and without parents. > > > Thanks, > > Artem Harutyunyan > >
Re: Review Request 39410: Added support for github to apply-reviews.py.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39410/#review103731 --- Ship it! This looks great - a really few minor nits, but then it's good to go! support/apply-reviews.py (lines 47 - 48) <https://reviews.apache.org/r/39410/#comment161807> s/cerror/error However, I think it'd read better as ``` # Trailing slash required by ReviewBoard's REST API ``` support/apply-reviews.py (line 49) <https://reviews.apache.org/r/39410/#comment161808> ``` if 'review_id' in options: ``` is more pythonic (also below: `elif 'github' in options`) support/apply-reviews.py (line 262) <https://reviews.apache.org/r/39410/#comment161813> you don't need the trailing \ also, please indent the second row, keeping the opening quotes aligned: ``` description='foo bar baz ' 'jump the fox') ``` support/apply-reviews.py (lines 287 - 290) <https://reviews.apache.org/r/39410/#comment161814> does this work? (I really am curious, I didn't know about the 'mutually exclusive' option!) in other words, don't you get an exception by trying to access **both** `args.github` AND `args.review_id` (because one of the two is missing)? - Marco Massenzio On Oct. 23, 2015, 6:18 a.m., Artem Harutyunyan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39410/ > --- > > (Updated Oct. 23, 2015, 6:18 a.m.) > > > Review request for mesos, Adam B, Joris Van Remoortere, Joseph Wu, Marco > Massenzio, and Vinod Kone. > > > Bugs: MESOS-3468 > https://issues.apache.org/jira/browse/MESOS-3468 > > > Repository: mesos > > > Description > --- > > Added support for github to apply-reviews.py. > > > Diffs > - > > support/apply-reviews.py PRE-CREATION > > Diff: https://reviews.apache.org/r/39410/diff/ > > > Testing > --- > > Tested with python 2.7 > > > Thanks, > > Artem Harutyunyan > >
Re: Review Request 39420: Added '--parent' option and made apply-review.sh call apply-reviews.py.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39420/#review103729 --- Ship it! Looks good - I'd still be happier with "Better Bash" :) would you mind "fixing" apply-review.sh? Thanks! - Marco Massenzio On Oct. 23, 2015, 6:19 a.m., Artem Harutyunyan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39420/ > --- > > (Updated Oct. 23, 2015, 6:19 a.m.) > > > Review request for mesos, Joris Van Remoortere, Joseph Wu, Marco Massenzio, > and Vinod Kone. > > > Bugs: MESOS-3468 > https://issues.apache.org/jira/browse/MESOS-3468 > > > Repository: mesos > > > Description > --- > > Added '--parent' option and made apply-review.sh call apply-reviews.py. > > > Diffs > - > > support/apply-review.sh 6391451542e9e8847ec38e2ad9d9acf552afead3 > support/apply-reviews.py PRE-CREATION > > Diff: https://reviews.apache.org/r/39420/diff/ > > > Testing > --- > > Tested with python 2.7. > > - with and without '-p'. > - Tested reviews with and without parents. > > > Thanks, > > Artem Harutyunyan > >
Re: Review Request 39410: Added support for github to apply-reviews.py.
> On Oct. 20, 2015, 5:06 a.m., Marco Massenzio wrote: > > support/apply-reviews.py, line 106 > > <https://reviews.apache.org/r/39410/diff/3/?file=1100642#file1100642line106> > > > > it would be really nice if we could make our code work under both 2.7 > > and 3.x Python ;) > > Artem Harutyunyan wrote: > replaced this with `print(output)`, that should work in 3.x, correct? yes, but only by mistake :) (`(output)` is interpreted as a 1-tuple in 2.7 and `print` is still assumed to be without parentheses - in 3.4, as a paren-surrounded string) You will need to add (as the VERY FIRST line after the shebang): ``` from __future__ import print_function ``` - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39410/#review103208 --- On Oct. 23, 2015, 6:18 a.m., Artem Harutyunyan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39410/ > --- > > (Updated Oct. 23, 2015, 6:18 a.m.) > > > Review request for mesos, Adam B, Joris Van Remoortere, Joseph Wu, Marco > Massenzio, and Vinod Kone. > > > Bugs: MESOS-3468 > https://issues.apache.org/jira/browse/MESOS-3468 > > > Repository: mesos > > > Description > --- > > Added support for github to apply-reviews.py. > > > Diffs > - > > support/apply-reviews.py PRE-CREATION > > Diff: https://reviews.apache.org/r/39410/diff/ > > > Testing > --- > > Tested with python 2.7 > > > Thanks, > > Artem Harutyunyan > >
Re: Review Request 39514: HTTP Scheduler API no longer allows FrameworkInfo.user to be empty
> On Oct. 21, 2015, 6:05 p.m., Marco Massenzio wrote: > > include/mesos/v1/mesos.proto, lines 211-213 > > <https://reviews.apache.org/r/39514/diff/1/?file=1102339#file1102339line211> > > > > I know it's not your code, but could you please remove `automagically` > > (there's nothing magic in a string assignment :) > > > > Please use: `SchedulerDriver` > > > > The comment could read more easily if you changed it to: > > ``` > > // When the Scheduler HTTP API is used for registering the framework, > > // the `user` field should be explicitly set, unless the > > // `--switch_user` flag is explicitly set to `false` on > > // the agent nodes (otherwise, the tasks will fail). > > // For the same reason, if the user is set, it should be an > > // existing user on the agent nodes, with the necessary permissions. bad formatting, apologies: ``` // When the Scheduler HTTP API is used for registering the framework, // the `user` field should be explicitly set, unless the // `--switch_user` flag is explicitly set to `false` on // the agent nodes (otherwise, the tasks will fail). // For the same reason, if the user is set, it should be an // existing user on the agent nodes, with the necessary permissions. ``` - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39514/#review103440 --- On Oct. 21, 2015, 8:48 a.m., Liqiang Lin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39514/ > --- > > (Updated Oct. 21, 2015, 8:48 a.m.) > > > Review request for mesos. > > > Bugs: MESOS-3747 > https://issues.apache.org/jira/browse/MESOS-3747 > > > Repository: mesos > > > Description > --- > > Add validation to check whether FrameworkInfo.user be empty. Fail > framework action if framework user is empty. > > > Diffs > - > > include/mesos/v1/mesos.proto 8131778fe5c5f3a47ae9300a811e3d857a22da6a > src/master/validation.cpp 9c7c8c123c5bd13b99a2c1c7318515d687b4bc34 > > Diff: https://reviews.apache.org/r/39514/diff/ > > > Testing > --- > > make check done. > > > Thanks, > > Liqiang Lin > >
Re: Review Request 39514: HTTP Scheduler API no longer allows FrameworkInfo.user to be empty
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39514/#review103440 --- Thanks for this; please add reviewers to this patch, and find a shepherd. include/mesos/v1/mesos.proto (lines 211 - 213) <https://reviews.apache.org/r/39514/#comment161494> I know it's not your code, but could you please remove `automagically` (there's nothing magic in a string assignment :) Please use: `SchedulerDriver` The comment could read more easily if you changed it to: ``` // When the Scheduler HTTP API is used for registering the framework, // the `user` field should be explicitly set, unless the // `--switch_user` flag is explicitly set to `false` on // the agent nodes (otherwise, the tasks will fail). // For the same reason, if the user is set, it should be an // existing user on the agent nodes, with the necessary permissions. src/master/validation.cpp (lines 73 - 76) <https://reviews.apache.org/r/39514/#comment161496> an emtpy user is a valid user case, for when the agent is set to `--switch_user=false`: you should not prevent this outright. however, as this may cause confusing errors, I would be perfectly fine with a LOG(WARN) telling the caller that if he sees tasks failing this may be the reason: ``` LOG(WARN) << "You have not set a user to run the task as on the agent; " << "unless the --switch_user is set to `false` on the agent " << "this may cause tasks to fail"; ``` - Marco Massenzio On Oct. 21, 2015, 8:48 a.m., Liqiang Lin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39514/ > --- > > (Updated Oct. 21, 2015, 8:48 a.m.) > > > Review request for mesos. > > > Bugs: MESOS-3747 > https://issues.apache.org/jira/browse/MESOS-3747 > > > Repository: mesos > > > Description > --- > > Add validation to check whether FrameworkInfo.user be empty. Fail > framework action if framework user is empty. > > > Diffs > - > > include/mesos/v1/mesos.proto 8131778fe5c5f3a47ae9300a811e3d857a22da6a > src/master/validation.cpp 9c7c8c123c5bd13b99a2c1c7318515d687b4bc34 > > Diff: https://reviews.apache.org/r/39514/diff/ > > > Testing > --- > > make check done. > > > Thanks, > > Liqiang Lin > >
Re: Review Request 39452: MESOS-3566 Description of RecordIO format
> On Oct. 19, 2015, 10:34 p.m., Anand Mazumdar wrote: > > docs/scheduler-http-api.md, line 44 > > <https://reviews.apache.org/r/39452/diff/2/?file=1101221#file1101221line44> > > > > Nit: Can we just check for `200` here (since the response from > > `Subscribe` should be `200` for a good response ? we removed this code > On Oct. 19, 2015, 10:34 p.m., Anand Mazumdar wrote: > > docs/scheduler-http-api.md, line 51 > > <https://reviews.apache.org/r/39452/diff/2/?file=1101221#file1101221line51> > > > > Is `int(...)` in python unsigned and 64 bytes ? That is the > > requirements we mention in our docs. no (signed) and yes (64 bits - not bytes :) - but that's now largely irrelevant, as we removed the code. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39452/#review103172 ------- On Oct. 19, 2015, 9:07 p.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39452/ > --- > > (Updated Oct. 19, 2015, 9:07 p.m.) > > > Review request for mesos, Anand Mazumdar and Vinod Kone. > > > Bugs: MESOS-3566 > https://issues.apache.org/jira/browse/MESOS-3566 > > > Repository: mesos > > > Description > --- > > Added the description of the RecordIO format to the HTTP API > document with example code (Python) to decode. > > > Diffs > - > > docs/scheduler-http-api.md de6cfc9e009a857ca45291b2dadce2a3b8199787 > > Diff: https://reviews.apache.org/r/39452/diff/ > > > Testing > --- > > > Thanks, > > Marco Massenzio > >
Re: Review Request 39452: MESOS-3566 Description of RecordIO format
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39452/ --- (Updated Oct. 20, 2015, 4:34 p.m.) Review request for mesos, Anand Mazumdar and Vinod Kone. Bugs: MESOS-3566 https://issues.apache.org/jira/browse/MESOS-3566 Repository: mesos Description --- Added the description of the RecordIO format to the HTTP API document with example code (Python) to decode. Diffs (updated) - docs/scheduler-http-api.md de6cfc9e009a857ca45291b2dadce2a3b8199787 Diff: https://reviews.apache.org/r/39452/diff/ Testing --- Thanks, Marco Massenzio
Re: Review Request 39452: MESOS-3566 Description of RecordIO format
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39452/ --- (Updated Oct. 20, 2015, 9:30 p.m.) Review request for mesos, Anand Mazumdar and Vinod Kone. Bugs: MESOS-3566 https://issues.apache.org/jira/browse/MESOS-3566 Repository: mesos Description --- Added the description of the RecordIO format to the HTTP API document with example code (Python) to decode. Diffs (updated) - docs/scheduler-http-api.md de6cfc9e009a857ca45291b2dadce2a3b8199787 Diff: https://reviews.apache.org/r/39452/diff/ Testing --- Thanks, Marco Massenzio
Re: Review Request 39420: Added '--parent' option and made apply-review.sh call apply-reviews.py.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39420/#review103312 --- Ship it! I guess you want to retain apply-reviews.sh for backward compatibility; however, it can be entirely retired by making apply-reviews.py executable (`chmod a+x`) and adding a shebang at the top: ``` #!/usr/bin/env python ``` support/apply-review.sh (line 3) <https://reviews.apache.org/r/39420/#comment161291> I really dislike this style of conditional running stuff in bash (it also violates Google's style, but that's another story :) ) ``` if [[ ! -f "support/apply-reviews.py" ]]; then echo "..." exit 1 fi python ./support/apply-reviews.py "$@" ``` support/apply-reviews.py (lines 267 - 272) <https://reviews.apache.org/r/39420/#comment161292> did you introduce leading tabs? (it may very well be a RB artifact, difficult to tell) - Marco Massenzio On Oct. 20, 2015, 7:45 a.m., Artem Harutyunyan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39420/ > --- > > (Updated Oct. 20, 2015, 7:45 a.m.) > > > Review request for mesos, Joris Van Remoortere, Joseph Wu, Marco Massenzio, > and Vinod Kone. > > > Bugs: MESOS-3468 > https://issues.apache.org/jira/browse/MESOS-3468 > > > Repository: mesos > > > Description > --- > > Added '--parent' option and made apply-review.sh call apply-reviews.py. > > > Diffs > - > > support/apply-review.sh 6391451542e9e8847ec38e2ad9d9acf552afead3 > support/apply-reviews.py PRE-CREATION > > Diff: https://reviews.apache.org/r/39420/diff/ > > > Testing > --- > > Tested with python 2.7. > > - with and without '-p'. > - Tested reviews with and without parents. > > > Thanks, > > Artem Harutyunyan > >
Re: Review Request 39447: MESOS-3692 - better documented --switch_user flag
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39447/ --- (Updated Oct. 19, 2015, 9:23 p.m.) Review request for mesos and Ben Mahler. Changes --- comments addressed Bugs: MESOS-3692 https://issues.apache.org/jira/browse/MESOS-3692 Repository: mesos Description --- MESOS-3692 - better documented --switch_user flag Added better logging around the point where the error may occur. Diffs (updated) - 3rdparty/libprocess/README.md 8e434c68a2c9bead608324b0e9ad0d31a2a3bb08 docs/configuration.md 69fb37fc38e52277605b82cf67067168a37d79a6 docs/upgrades.md 3854e59f68d53892b2673a4542ea2aaf96936cbb src/examples/persistent_volume_framework.cpp 176ac3df6f6a536affb4853d5d2f0bfda81fd720 src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2 src/slave/paths.cpp fb77e64a6da017d8c9a00916d8935b670da0d374 Diff: https://reviews.apache.org/r/39447/diff/ Testing --- make check Thanks, Marco Massenzio
Review Request 39452: MESOS-3566 Description of RecordIO format
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39452/ --- Review request for mesos, Anand Mazumdar and Vinod Kone. Bugs: MESOS-3566 https://issues.apache.org/jira/browse/MESOS-3566 Repository: mesos Description --- Added the description of the RecordIO format to the HTTP API document with example code (Python) to decode. Diffs - docs/scheduler-http-api.md de6cfc9e009a857ca45291b2dadce2a3b8199787 Diff: https://reviews.apache.org/r/39452/diff/ Testing --- Thanks, Marco Massenzio
Re: Review Request 39447: MESOS-3692 - better documented --switch_user flag
> On Oct. 19, 2015, 7:27 p.m., Kapil Arya wrote: > > src/slave/paths.cpp, lines 427-429 > > <https://reviews.apache.org/r/39447/diff/2/?file=1101169#file1101169line427> > > > > Why not use the same `LOG(WARNING)` statement? That way the entire > > message will appear together instead of being sliced up by some other > > concurrent message. Because a priori I don't know what `chown.error()` will emit (can be a multiline error, whatever) and so it may look pretty awful. However, having looked at it - I've actually moved it to the end of the sentence, so it should still look good. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39447/#review103148 ------- On Oct. 19, 2015, 7:06 p.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39447/ > --- > > (Updated Oct. 19, 2015, 7:06 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-3692 > https://issues.apache.org/jira/browse/MESOS-3692 > > > Repository: mesos > > > Description > --- > > MESOS-3692 - better documented --switch_user flag > Added better logging around the point where the error may occur. > > > Diffs > - > > docs/configuration.md 69fb37fc38e52277605b82cf67067168a37d79a6 > src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2 > src/slave/paths.cpp fb77e64a6da017d8c9a00916d8935b670da0d374 > > Diff: https://reviews.apache.org/r/39447/diff/ > > > Testing > --- > > make check > > > Thanks, > > Marco Massenzio > >
Re: Review Request 39447: MESOS-3692 - better documented --switch_user flag
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39447/ --- (Updated Oct. 19, 2015, 9:27 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3692 https://issues.apache.org/jira/browse/MESOS-3692 Repository: mesos Description --- MESOS-3692 - better documented --switch_user flag Added better logging around the point where the error may occur. Diffs (updated) - docs/configuration.md 69fb37fc38e52277605b82cf67067168a37d79a6 src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2 src/slave/paths.cpp fb77e64a6da017d8c9a00916d8935b670da0d374 Diff: https://reviews.apache.org/r/39447/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 39447: MESOS-3692 - better documented --switch_user flag
> On Oct. 19, 2015, 7:35 p.m., Michael Park wrote: > > Thanks for doing this! It's definitely clearer (at least to me) as to what > > this flag is controlling. > > > > A few general comments here: > > (1) We should be using ``` in some of the cases instead of `'` right? It > > seems to be a bit inconsistent currently. > > (2) Based on existing documentation, I don't think we should go out of our > > way to capitalize `Agent`. agreed - tried to use `` consistently, and replaced ' - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39447/#review103146 --- On Oct. 19, 2015, 7:06 p.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39447/ > --- > > (Updated Oct. 19, 2015, 7:06 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-3692 > https://issues.apache.org/jira/browse/MESOS-3692 > > > Repository: mesos > > > Description > --- > > MESOS-3692 - better documented --switch_user flag > Added better logging around the point where the error may occur. > > > Diffs > - > > docs/configuration.md 69fb37fc38e52277605b82cf67067168a37d79a6 > src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2 > src/slave/paths.cpp fb77e64a6da017d8c9a00916d8935b670da0d374 > > Diff: https://reviews.apache.org/r/39447/diff/ > > > Testing > --- > > make check > > > Thanks, > > Marco Massenzio > >
Re: Review Request 39452: MESOS-3566 Description of RecordIO format
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39452/ --- (Updated Oct. 19, 2015, 9:07 p.m.) Review request for mesos, Anand Mazumdar and Vinod Kone. Bugs: MESOS-3566 https://issues.apache.org/jira/browse/MESOS-3566 Repository: mesos Description --- Added the description of the RecordIO format to the HTTP API document with example code (Python) to decode. Diffs (updated) - docs/scheduler-http-api.md de6cfc9e009a857ca45291b2dadce2a3b8199787 Diff: https://reviews.apache.org/r/39452/diff/ Testing --- Thanks, Marco Massenzio
Re: Review Request 39447: MESOS-3692 - better documented --switch_user flag
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39447/ --- (Updated Oct. 19, 2015, 7:06 p.m.) Review request for mesos and Ben Mahler. Changes --- Added better logging around the point where the error may occur Bugs: MESOS-3692 https://issues.apache.org/jira/browse/MESOS-3692 Repository: mesos Description (updated) --- MESOS-3692 - better documented --switch_user flag Added better logging around the point where the error may occur. Diffs (updated) - docs/configuration.md 69fb37fc38e52277605b82cf67067168a37d79a6 src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2 src/slave/paths.cpp fb77e64a6da017d8c9a00916d8935b670da0d374 Diff: https://reviews.apache.org/r/39447/diff/ Testing (updated) --- make check Thanks, Marco Massenzio
Re: Review Request 39449: Documented order of includes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39449/#review103145 --- docs/c++-style-guide.md (line 241) <https://reviews.apache.org/r/39449/#comment161088> s/seperate/separate docs/c++-style-guide.md (line 243) <https://reviews.apache.org/r/39449/#comment161090> I would specify what .ccp file is this example related to. I think we have the 'related header' rule, where, if this where `master/flags.cpp` then `master/flags.hpp` should be included first. - Marco Massenzio On Oct. 19, 2015, 6:58 p.m., Jan Schlicht wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39449/ > --- > > (Updated Oct. 19, 2015, 6:58 p.m.) > > > Review request for mesos and Michael Park. > > > Bugs: MESOS-2275 > https://issues.apache.org/jira/browse/MESOS-2275 > > > Repository: mesos > > > Description > --- > > Documented order of includes. > > > Diffs > - > > docs/c++-style-guide.md 0b6189174a4f0f1815625f68fb1a743b04a9cdad > > Diff: https://reviews.apache.org/r/39449/diff/ > > > Testing > --- > > make check > > > Thanks, > > Jan Schlicht > >
Review Request 39447: MESOS-3692 - better documented --switch_user flag
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39447/ --- Review request for mesos and Ben Mahler. Bugs: MESOS-3692 https://issues.apache.org/jira/browse/MESOS-3692 Repository: mesos Description --- MESOS-3692 - better documented --switch_user flag Diffs - docs/configuration.md 69fb37fc38e52277605b82cf67067168a37d79a6 src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2 Diff: https://reviews.apache.org/r/39447/diff/ Testing --- Thanks, Marco Massenzio
Re: Review Request 39447: MESOS-3692 - better documented --switch_user flag
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39447/ --- (Updated Oct. 19, 2015, 10:21 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3692 https://issues.apache.org/jira/browse/MESOS-3692 Repository: mesos Description --- MESOS-3692 - better documented --switch_user flag Added better logging around the point where the error may occur. Diffs (updated) - docs/configuration.md 69fb37fc38e52277605b82cf67067168a37d79a6 src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2 src/slave/paths.cpp fb77e64a6da017d8c9a00916d8935b670da0d374 Diff: https://reviews.apache.org/r/39447/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 39447: MESOS-3692 - better documented --switch_user flag
> On Oct. 19, 2015, 10:09 p.m., Michael Park wrote: > > docs/configuration.md, line 1445 > > <https://reviews.apache.org/r/39447/diff/4/?file=1101271#file1101271line1445> > > > > `\Could not chown work directory\` -- is this intentional? If yes, what > > does it do...? good catch! copy & paste & find & replace fail :) - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39447/#review103170 --- On Oct. 19, 2015, 9:27 p.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39447/ > --- > > (Updated Oct. 19, 2015, 9:27 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-3692 > https://issues.apache.org/jira/browse/MESOS-3692 > > > Repository: mesos > > > Description > --- > > MESOS-3692 - better documented --switch_user flag > Added better logging around the point where the error may occur. > > > Diffs > - > > docs/configuration.md 69fb37fc38e52277605b82cf67067168a37d79a6 > src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2 > src/slave/paths.cpp fb77e64a6da017d8c9a00916d8935b670da0d374 > > Diff: https://reviews.apache.org/r/39447/diff/ > > > Testing > --- > > make check > > > Thanks, > > Marco Massenzio > >
Re: Review Request 39447: MESOS-3692 - better documented --switch_user flag
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39447/ --- (Updated Oct. 19, 2015, 11:55 p.m.) Review request for mesos and Ben Mahler. Changes --- Updated the message to reflect that the task fails. Bugs: MESOS-3692 https://issues.apache.org/jira/browse/MESOS-3692 Repository: mesos Description --- MESOS-3692 - better documented --switch_user flag Added better logging around the point where the error may occur. Diffs (updated) - docs/configuration.md 69fb37fc38e52277605b82cf67067168a37d79a6 src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2 src/slave/paths.cpp fb77e64a6da017d8c9a00916d8935b670da0d374 Diff: https://reviews.apache.org/r/39447/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 39410: Added support for github to apply-reviews.py.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39410/#review103208
---
support/apply-reviews.py (line 23)
<https://reviews.apache.org/r/39410/#comment161177>
hey, as mentioned, could you please remove whitespace around `=` in args
call (and default params)
here and everywhere.
thanks!
support/apply-reviews.py (line 27)
<https://reviews.apache.org/r/39410/#comment161178>
is this right? by looking at the args names, looks like it returns the URL
of a PR given its number?
either the comment or the args names are wrong.
also, I would much prefer `pull_request_number`
support/apply-reviews.py (line 44)
<https://reviews.apache.org/r/39410/#comment161179>
could you please add an @param and explain what `options` is and what does
it look like?
(also an @type would be awesome)
support/apply-reviews.py (lines 46 - 47)
<https://reviews.apache.org/r/39410/#comment161181>
is this dead code? please remove.
support/apply-reviews.py (line 48)
<https://reviews.apache.org/r/39410/#comment161180>
this will cause a KeyError if `reveiw_id` is not there.
you can achieve the same result (but better) with:
```
if options.get('review_id'):
```
(this also protects you further below in the call to `format`)
support/apply-reviews.py (line 97)
<https://reviews.apache.org/r/39410/#comment161182>
```
if not options or 'dry_run' not in options:
```
support/apply-reviews.py (line 103)
<https://reviews.apache.org/r/39410/#comment161183>
it would be really nice if we could make our code work under both 2.7 and
3.x Python ;)
support/apply-reviews.py (line 133)
<https://reviews.apache.org/r/39410/#comment161184>
this line exceeds 100 char (I'm almost sure)
you can break it using \
or, even better, build the command via string.join():
```
' '.join(['wget',
'--no-check-certificate',
'--no-verbose',
'-O {}.patch'.format(patch_id(options)),
patch_url(options)])
```
support/apply-reviews.py (line 136)
<https://reviews.apache.org/r/39410/#comment161185>
s/beacuse/because
support/apply-reviews.py (line 145)
<https://reviews.apache.org/r/39410/#comment161186>
unnecessary parentheses; also, please use `get()` instead of `[]`
(unless you are terminally positive both keys are there every time)
support/apply-reviews.py (line 156)
<https://reviews.apache.org/r/39410/#comment161187>
you don't really need to escape the quotes, just take advantage of Python
being able to use single and double quotes interchangeably (or even use """ if
you really want to be hip :)
support/apply-reviews.py (lines 179 - 181)
<https://reviews.apache.org/r/39410/#comment161188>
this looks a bit ugly - prefer:
```
message = '{summary}\n\n{description}\n\nThis closes: {pr}'.
format(summary=title, description=description, pr=pr_number)
```
support/apply-reviews.py (lines 184 - 188)
<https://reviews.apache.org/r/39410/#comment161189>
no space before `:`
(also, it's more "pythonic" to use double quotes for the dict's keys - but
really a micro-nit!)
support/apply-reviews.py (lines 200 - 205)
<https://reviews.apache.org/r/39410/#comment161191>
could you please reformat this code to something more readable?
```
username = review.get('links').get('submitter').get('title')
user = url_to_json(user_url(username)).get('user')
url = review_url(rid)
author = '{author} {email}'.format(author=user.get('fullname'),
email=user.get('email'))
message = '\n\n'.join([review.get('summary'),
review.get('description'),
'Review: {}'.format(url)
```
support/apply-reviews.py (lines 251 - 255)
<https://reviews.apache.org/r/39410/#comment161192>
Much better:
```
options = {
'review_id': args.review_id,
'dry_run': args.dry_run,
'no_amend': args.no_amend,
'github': args.github
}
```
or if you really want to be pythonic:
```
options = dict()
for k in ['review_id', 'dry_run', 'no_amend', 'github']:
options[k] = args.getattr(k)
```
although, I'm not sure right now if this will work with your having made
`github` and `review_id` mutually exclusive (but, then again, the dotted
notation should blow up too if the arg is not there?)
support/apply-reviews.py (lines 265 - 270)
<https://reviews.apache.org/r/39410/#comment161193>
this code look familiar and I r
Re: Review Request 37336: Simplified the caller interface to process::Subprocess
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/ --- (Updated Oct. 18, 2015, 3:22 p.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Fixed compile error which was not caught by local `make && make check` Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description --- Jira: MESOS-3035 The original API for `process::Subprocess` still left a lot of legwork to do for the caller; we have now added an `execute()` method that returns a `Future` (also introduced with this patch) which contains useful information about the command invocation; the exit code; stdout and, optionally, stderr too. Once the Future completes, if successful, the caller will be able to retrieve stdout/stderr; whether the command was successful; and whether it received a signal Diffs (updated) - 3rdparty/libprocess/include/process/subprocess.hpp d2341a53aac7c779668ee80983f73158fd44bff5 3rdparty/libprocess/src/subprocess.cpp a457cbe35ad33531c49f74550cd570cf28758f5d 3rdparty/libprocess/src/tests/subprocess_tests.cpp ac600a551fb1a7782ff33cce204b7819497ef54a Diff: https://reviews.apache.org/r/37336/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 39386: Fix uncorrect launcher dir in docker executor.
> On Oct. 16, 2015, 5:30 p.m., Marco Massenzio wrote: > > Thanks for doing this. > > > > I think you will need to document the flags' usage in `configuration.md` > > (or wherever appropriate) and state clearly that it's **required** and what > > it should point to (in other words, what binary(ies) will we be looking > > for?) - or users will be confused by the error message. Actually - would it be possible to add a unit test to catch the scenario that was reported in the Jira and make sure this fixes it? (when it comes to interaction between OS env vars and flags, it's always tricky) - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39386/#review102939 --- On Oct. 16, 2015, 5:17 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39386/ > --- > > (Updated Oct. 16, 2015, 5:17 a.m.) > > > Review request for mesos and Timothy Chen. > > > Bugs: MESOS-3738 > https://issues.apache.org/jira/browse/MESOS-3738 > > > Repository: mesos > > > Description > --- > > Fix uncorrect launcher dir in docker executor. > > > Diffs > - > > src/docker/executor.hpp 8ab4e98c41017e8420b98c63a59d8e4bc1a99172 > src/docker/executor.cpp 1e4901335854c49e46cd7b132e79ccb11cd72ade > src/slave/containerizer/docker.cpp 702295808475c092dff66417f42af89b90e6d50d > > Diff: https://reviews.apache.org/r/39386/diff/ > > > Testing > --- > > * make check > * make install and then test with marathon to check if launcher_dir passes > correctly. > > > Thanks, > > haosdent huang > >
Re: Review Request 38883: Removed calls to apply-review.sh script. Added support for amending commit messages.
> On Oct. 16, 2015, 4:15 p.m., Marco Massenzio wrote: > > Ship It! Thanks for addressing comments! - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38883/#review102921 --- On Oct. 16, 2015, 3:29 p.m., Artem Harutyunyan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38883/ > --- > > (Updated Oct. 16, 2015, 3:29 p.m.) > > > Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, > and Vinod Kone. > > > Bugs: MESOS-3468 > https://issues.apache.org/jira/browse/MESOS-3468 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > support/apply-reviews.py PRE-CREATION > > Diff: https://reviews.apache.org/r/38883/diff/ > > > Testing > --- > > Tested the script with python 2.7. > > > Thanks, > > Artem Harutyunyan > >
Re: Review Request 38883: Removed calls to apply-review.sh script. Added support for amending commit messages.
> On Oct. 14, 2015, 4:45 p.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 40
> > <https://reviews.apache.org/r/38883/diff/3/?file=1093293#file1093293line40>
> >
> > (setting aside for a second that we should use `requests.get()` instead
> > :)
> >
> > can you please that you get a 200 OK code and that there is actually
> > JSON content?
> >
> > with `requests` this is as easy as:
> > ```
> > import requests
> >
> > ...
> > try:
> > headers = {'AcceptContent': "application/json"}
> > r = requests.get(url, timeout=50, headers=headers)
> > if 200 <= r.status_code < 300 and r.headers.get('content-type') ==
> > 'application/json':
> > return r.json()
> > else:
> > # do something with error
> > pass
> > except requestsTimeoutError:
> > # log the error and probably give up, the server is down
> > pass
> > ```
>
> Artem Harutyunyan wrote:
> In both cases (if it's not a `200` code, or if it's not a JSON) I want to
> terminate, and that's exactly what the script will do for me. Since the
> intended audience are developers I am willingly frugal when it comes to
> writing code, and tend to resort to stack traces wherever I can. What do you
> think?
I don't like relying on stack traces (and Python's aren't particularly useful)
- even as a developer, the message I derive is "the code is buggy" *not*
something went wrong with the service.
Providing a LOG(ERROR) with an explanation of whent went wrong would be a much
better experience (and, in this specific case, I bet the stacktrace would be
something along the lines of "Unknown JSON content" or something equally
misleading).
> On Oct. 14, 2015, 4:45 p.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 91
> > <https://reviews.apache.org/r/38883/diff/3/?file=1093293#file1093293line91>
> >
> > ```
> > from __future__ import print_function:
> >
> > ...
> >
> > print(output)
> > ```
> > (this way it'll all work also for those us using a modern Python
> > interpreter :) )
> >
> > thanks!
>
> Artem Harutyunyan wrote:
> Other scripts in the repo seem to be developed for Python 2.x, and I'd
> like to stay consistent.
that being exactly my point: what I suggested is consistent with 2.7, but can
be used with 3.4 without any change.
have your cake and eat it, too :D
(and it's only good to be consistent with something that's good ;-) )
> On Oct. 14, 2015, 4:45 p.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 102
> > <https://reviews.apache.org/r/38883/diff/3/?file=1093293#file1093293line102>
> >
> > consider using the `sh` module:
> > ```
> > import sh
> >
> > cmd = "{rev_id}.patch".format(rev_id=review_id)
> > sh.rm("-f", cmd)
> >
> > ```
>
> Artem Harutyunyan wrote:
> `sh` does not seem to be a stock module.
good point!
Although really easy to fix:
```
try:
import sh
except ImportError:
print("This script requires the use of the `sh` module; please run: `sudo
pip install sh`")
exit(1)
```
(especially, as you remarked above, that users are developers, so shouldn't be
afraid of doing this)
If we want to be sophisticated, we could even have a `requirements.txt` that
we'd run during the ./bootstrap script or document developers to run once.
`pip install --upgrade -r requirements.txt`
- Marco
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review102624
---
On Oct. 16, 2015, 3:29 p.m., Artem Harutyunyan wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> ---
>
> (Updated Oct. 16, 2015, 3:29 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu,
> and Vinod Kone.
>
>
> Bugs: MESOS-3468
> https://issues.apache.org/jira/browse/MESOS-3468
>
>
> Repository: mesos
>
>
> Description
> ---
>
> See summary.
>
>
> Diffs
> -
>
> support/apply-reviews.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/38883/diff/
>
>
> Testing
> ---
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script. Added support for amending commit messages.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38883/#review102921 --- Ship it! Ship It! - Marco Massenzio On Oct. 16, 2015, 3:29 p.m., Artem Harutyunyan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38883/ > --- > > (Updated Oct. 16, 2015, 3:29 p.m.) > > > Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, > and Vinod Kone. > > > Bugs: MESOS-3468 > https://issues.apache.org/jira/browse/MESOS-3468 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > support/apply-reviews.py PRE-CREATION > > Diff: https://reviews.apache.org/r/38883/diff/ > > > Testing > --- > > Tested the script with python 2.7. > > > Thanks, > > Artem Harutyunyan > >
Re: Review Request 37336: Simplified the caller interface to process::Subprocess
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/ --- (Updated Oct. 16, 2015, 11:03 p.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description (updated) --- Jira: MESOS-3035 The original API for `process::Subprocess` still left a lot of legwork to do for the caller; we have now added an `executed()` method that returns a `Future` (also introduced with this patch) which contains useful information about the command invocation; the exit code; stdout and, optionally, stderr too. Once the Future completes, if successful, the caller will be able to retrieve stdout/stderr; whether the command was successful; and whether it received a signal Diffs - 3rdparty/libprocess/include/process/subprocess.hpp d2341a53aac7c779668ee80983f73158fd44bff5 3rdparty/libprocess/src/subprocess.cpp d6ea62ed1c914d34e0e189395831c86fff8aac22 3rdparty/libprocess/src/tests/subprocess_tests.cpp ab7515325e5db0a4fd222bb982f51243d7b7e39d Diff: https://reviews.apache.org/r/37336/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 37336: Simplified the caller interface to process::Subprocess
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/ --- (Updated Oct. 16, 2015, 11:05 p.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Modified following conversation w/ benh/joris Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description (updated) --- Jira: MESOS-3035 The original API for `process::Subprocess` still left a lot of legwork to do for the caller; we have now added an `execute()` method that returns a `Future` (also introduced with this patch) which contains useful information about the command invocation; the exit code; stdout and, optionally, stderr too. Once the Future completes, if successful, the caller will be able to retrieve stdout/stderr; whether the command was successful; and whether it received a signal Diffs (updated) - 3rdparty/libprocess/include/process/subprocess.hpp d2341a53aac7c779668ee80983f73158fd44bff5 3rdparty/libprocess/src/subprocess.cpp a457cbe35ad33531c49f74550cd570cf28758f5d 3rdparty/libprocess/src/tests/subprocess_tests.cpp ac600a551fb1a7782ff33cce204b7819497ef54a Diff: https://reviews.apache.org/r/37336/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 38883: Removed calls to apply-review.sh script. Added support for amending commit messages.
> On Oct. 16, 2015, 4:15 p.m., Marco Massenzio wrote:
> > Ship It!
>
> Marco Massenzio wrote:
> Thanks for addressing comments!
hey, minor nit: I've just noticed you insert whitespaces around named args:
```
"".format(url = USER_URL, user = username)
```
PEP8[0] requires that there are no spaces around the `=`:
```
format(url=USER_URL, user=username)
```
also in the default values around method params:
```
def do_it(name="foof", bar=None):
```
if you can fix before committing that'd be grand (but no big deal).
Thanks!
[0]
https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements
("""Don't use spaces around the = sign when used to indicate a keyword argument
or a default parameter value.""")
- Marco
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review102921
---
On Oct. 16, 2015, 3:29 p.m., Artem Harutyunyan wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> ---
>
> (Updated Oct. 16, 2015, 3:29 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu,
> and Vinod Kone.
>
>
> Bugs: MESOS-3468
> https://issues.apache.org/jira/browse/MESOS-3468
>
>
> Repository: mesos
>
>
> Description
> ---
>
> See summary.
>
>
> Diffs
> -
>
> support/apply-reviews.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/38883/diff/
>
>
> Testing
> ---
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 39386: Fix uncorrect launcher dir in docker executor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39386/#review102939 --- Ship it! Thanks for doing this. I think you will need to document the flags' usage in `configuration.md` (or wherever appropriate) and state clearly that it's **required** and what it should point to (in other words, what binary(ies) will we be looking for?) - or users will be confused by the error message. src/docker/executor.hpp (line 64) <https://reviews.apache.org/r/39386/#comment160768> Please note it is a "required" option and what it it is meant to be used for (and, ideally, what it should point to). - Marco Massenzio On Oct. 16, 2015, 5:17 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39386/ > --- > > (Updated Oct. 16, 2015, 5:17 a.m.) > > > Review request for mesos and Timothy Chen. > > > Bugs: MESOS-3738 > https://issues.apache.org/jira/browse/MESOS-3738 > > > Repository: mesos > > > Description > --- > > Fix uncorrect launcher dir in docker executor. > > > Diffs > - > > src/docker/executor.hpp 8ab4e98c41017e8420b98c63a59d8e4bc1a99172 > src/docker/executor.cpp 1e4901335854c49e46cd7b132e79ccb11cd72ade > src/slave/containerizer/docker.cpp 702295808475c092dff66417f42af89b90e6d50d > > Diff: https://reviews.apache.org/r/39386/diff/ > > > Testing > --- > > * make check > * make install and then test with marathon to check if launcher_dir passes > correctly. > > > Thanks, > > haosdent huang > >
Re: Review Request 39338: Added code that appends the fetcher log to the agent log upon fetcher failure.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39338/#review102848 --- Ship it! Ship It! - Marco Massenzio On Oct. 15, 2015, 1:11 p.m., Bernd Mathiske wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39338/ > --- > > (Updated Oct. 15, 2015, 1:11 p.m.) > > > Review request for mesos, Benjamin Bannier, Ben Mahler, and Till Toenshoff. > > > Bugs: MESOS-3743 > https://issues.apache.org/jira/browse/MESOS-3743 > > > Repository: mesos > > > Description > --- > > Added an onFailed() clause to the inspection of the fetcher subprocess run. > This clause copies the fetcher log from /stderr and appends it > to the agent log. > > This is to facilitate debugging spurious fetch failures in production or CI. > > Similar, but not the same: https://reviews.apache.org/r/37813/ (see > MESOS-3743 for an explanation). > > > Diffs > - > > src/slave/containerizer/fetcher.cpp > 2b2298c329ed5fb5863cb0fed1491e478c3e5d5a > > Diff: https://reviews.apache.org/r/39338/diff/ > > > Testing > --- > > Ran make check. As expected no change in behavior. > When I modified the fetcher to fail, > I observed the expected extra output. > > > Thanks, > > Bernd Mathiske > >
Re: Review Request 38883: Removed calls to apply-review.sh script. Added support for amending commit messages.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review102624
---
Again lots of nit-picking, feel free to ignore what you disagree with strongly
:)
Also, instead of using your homemade `shell()` method and `Subprocess`, given
the usage pattern I've noticed, you may want to consider the `sh` module:
http://amoffat.github.io/sh/
once you start using it, it gets pretty awesome!
support/apply-reviews.py (line 29)
<https://reviews.apache.org/r/38883/#comment160357>
always best to prefer `str.join()` to join strings; in this case, however,
as you need to terminate with the `/` it's best to use `format()`:
```
return "{url}/{user}/".format(url=USER_URL, user=user)
```
of course:
```
'/'.join([USER_URL, user, ''])
```
would have also been acceptable.
support/apply-reviews.py (line 34)
<https://reviews.apache.org/r/38883/#comment160358>
same here, please
support/apply-reviews.py (line 40)
<https://reviews.apache.org/r/38883/#comment160361>
(setting aside for a second that we should use `requests.get()` instead :)
can you please that you get a 200 OK code and that there is actually JSON
content?
with `requests` this is as easy as:
```
import requests
...
try:
headers = {'AcceptContent': "application/json"}
r = requests.get(url, timeout=50, headers=headers)
if 200 <= r.status_code < 300 and r.headers.get('content-type') ==
'application/json':
return r.json()
else:
# do something with error
pass
except requestsTimeoutError:
# log the error and probably give up, the server is down
pass
```
support/apply-reviews.py (line 59)
<https://reviews.apache.org/r/38883/#comment160366>
this will throw a KeyError if either of the keys are missing (and the user
will be none the wiser):
```
if 'review_request' in json_obj and not
json_obj['review_request].get('depends_on'):
... do something
```
(also see my comments in the other review about the lines above this)
support/apply-reviews.py (line 78)
<https://reviews.apache.org/r/38883/#comment160367>
```
from __future__ import print_function:
...
print(output)
```
(this way it'll all work also for those us using a modern Python
interpreter :) )
thanks!
support/apply-reviews.py (line 86)
<https://reviews.apache.org/r/38883/#comment160390>
please don't do this - prefer the use of `**kwargs` or, in this case, it's
much easier to do something like:
```
def remove_patch(review_id, options=None):
options = options or {'dry_run': False}
...
```
support/apply-reviews.py (line 89)
<https://reviews.apache.org/r/38883/#comment160368>
consider using the `sh` module:
```
import sh
cmd = "{rev_id}.patch".format(rev_id=review_id)
sh.rm("-f", cmd)
```
support/apply-reviews.py (line 93)
<https://reviews.apache.org/r/38883/#comment160392>
can you please explains what is `options` supposed to be / contain?
I think you expect a `dict` here, it would be great to have something
like...
```
""" blah blah
@param options: these are the fuz bits that god the baz, in a map that
can contain the following keys: [`dry_run`, `verbose`, `bitz`] and if not
specified assumes `dry_run` to be False.
@type options: dict
"""
```
support/apply-reviews.py (line 107)
<https://reviews.apache.org/r/38883/#comment160394>
prefer the use of `str.format()` instead
(here and everywhere else)
support/apply-reviews.py (lines 126 - 130)
<https://reviews.apache.org/r/38883/#comment160399>
I really really don't like this 'leading underscore' and same-naming: makes
reading the code really difficult and, thanks to Python happily ignoring
different types, the likelihood of bugs super-high.
also, seems to me that you never use `_review` anywhere else, so that's
what I'd do:
```
data = url_to_json(review_url(review_id))
if data:
review = data.get('review_request')
else:
# something bad happened bail
```
same goes for `_user` too.
support/apply-reviews.py (lines 133 - 139)
<https://reviews.apache.org/r/38883/#comment160400>
more pythonic:
```
review_data = {
'summary': review['summary'],
'description': review['description'],
'url': review_url(review_id),
'author': user['fullname'],
'email': user['email']
'message': '\n\n'.join([review['summary'],
review['descripti
Re: Review Request 38705: Added support for applying a review chain (apply-reviews.py).
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review101980
---
support/apply-reviews.py (line 7)
<https://reviews.apache.org/r/38705/#comment159488>
uhm... could we use `requests` instead?
much more modern API and widespread use.
support/apply-reviews.py (line 11)
<https://reviews.apache.org/r/38705/#comment159484>
nit: you are 'masking' the global builtin `id()` here - that's a PEP8
violation, could you please rename to something like `rid`?
support/apply-reviews.py (line 13)
<https://reviews.apache.org/r/38705/#comment159486>
don't concatenate string, prefer the use of os.path.join():
```
import os
os.path.join(REVIEW_URL, rid)
```
(why do you need the trailing slash?)
support/apply-reviews.py (line 19)
<https://reviews.apache.org/r/38705/#comment159489>
you should check for a non 4xx/5xx return code (or just check for a 2xx if
you know for sure what the API returns).
as you expect JSON, you should probably specify that in the
`Accept-content` header too?
support/apply-reviews.py (line 24)
<https://reviews.apache.org/r/38705/#comment159490>
if you are doing this often enough, it's much better to compile this to a
constant Pattern and then use that instead.
support/apply-reviews.py (line 29)
<https://reviews.apache.org/r/38705/#comment159495>
you should not call a param with the name of a type (`list` is a type in
Python).
also, it's bad practice to initialize it as you do there.
BTW - can you please use the @param to explain what the method expects?
Prefer:
```
def parent_review(rid, revs_list=None):
"""Returns a list with reversed chain of review requests for a given
Review ID.
"""
result = revs_list or []
json_str = review_json(review_url(rid))
json_obj = json.loads(json_str)
# Stop as soon as we stumble upon a submitted request.
rr = json_obj.get('review_request')
if rr and rr.get('status') == "submitted":
return result
for deps in rr.get('summary'):
...
```
Also, worth always using `get()` instead of accessor (`[]`) for data that
you are not sure may or may not be there (it's an API response after all) - the
latter will throw a KeyError if the key is missing; `get` will just give you
back None (or you can pass a second optional default return value if you want).
support/apply-reviews.py (line 40)
<https://reviews.apache.org/r/38705/#comment159497>
is this a recursion inside an iteration?
can you please add a comment? I'm sure folks who may one day try to
fix/augment this script will be just as confused...
support/apply-reviews.py (line 45)
<https://reviews.apache.org/r/38705/#comment159496>
is this right?
you return after the first iteration?
if so, why not just getting the first item in `depends_on`?
support/apply-reviews.py (line 56)
<https://reviews.apache.org/r/38705/#comment159499>
if you do this, this script will be probably useful to folks who use Python
3 too :)
```
from __future__ import print_function
...
print('foo bar')
```
Also (but that's just something a bunch of us insisted upon) I prefer the
use of `string.format()` to `%`:
```
print("Applying review {}: {}".format(review_id, summary))
```
(same also below to build `cmd`)
support/apply-reviews.py (line 90)
<https://reviews.apache.org/r/38705/#comment159501>
nit: `if r not in applied:` is more pythonic :)
(also, PEP8 doesn't really like 1- and 2-letter variables)
support/apply-reviews.py (line 91)
<https://reviews.apache.org/r/38705/#comment159502>
do you need a counter or something like that, or a flag would suffice?
I'm not sure how you use `applied` as a dict (looks like you are just using
it as a `set`?):
```
applied = set()
for review, summary in reviews:
if review not in applied:
applied.add(review)
print(apply_review(...))
```
- Marco Massenzio
On Oct. 8, 2015, 6:26 p.m., Artem Harutyunyan wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> ---
>
> (Updated Oct. 8, 2015, 6:26 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu,
> and Vinod Kone.
>
>
> Bugs: MESOS-3468
> https://issues.apache.org/jira/browse/MESOS-3468
>
>
> Repository: mesos
>
>
&
Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38627/#review102245
---
Would it be possible to add a few unit tests, also to show usage patterns?
especially given the absence of any documentation, it's kinda difficult to
figure out how is this "intended to work" and, without tests, whether it works
at all.
src/module/manager.hpp (line 102)
<https://reviews.apache.org/r/38627/#comment159819>
would it be possible to have proper, full javadoc for this method (as well
as the other one)?
these will be used by external module developers, so having them
well-documented would be really great.
src/tests/module.hpp (lines 76 - 77)
<https://reviews.apache.org/r/38627/#comment159821>
same comment here - it would be great to have fully-documented,
properly-formatted javadoc here, also explaining what could go into
`parameters` how will this affect creating the module, etc.
while this may all appear obvious to you, it may not be so to external
developers using this code to launch/test modules.
thanks!
src/tests/module.hpp (line 80)
<https://reviews.apache.org/r/38627/#comment159823>
(I understand this may not be your choice, but...)
I find `N` as an identifier is a poor choice because (a) it's entirely
non-obvious *what* is it and (b) I'd expect it anyway to be an `int` of some
sort.
Could you please consider renaming it to be something more expressive?
thanks!
- Marco Massenzio
On Oct. 2, 2015, 8:11 p.m., Alexander Rojas wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
>
> (Updated Oct. 2, 2015, 8:11 p.m.)
>
>
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till
> Toenshoff.
>
>
> Bugs: MESOS-3232
> https://issues.apache.org/jira/browse/MESOS-3232
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Allows developers to provide their own parameters when loading modules
> instead of using the ones provided by the user when loading Mesos. This helps
> to deal with default modules (those used when the user doesn't provide any),
> and for testing of the modules.
>
>
> Diffs
> -
>
> src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef
> src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2
>
> Diff: https://reviews.apache.org/r/38627/diff/
>
>
> Testing
> ---
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37336: Simplified the caller interface to process::Subprocess
> On Oct. 12, 2015, 4:08 p.m., Joris Van Remoortere wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, line 281 > > <https://reviews.apache.org/r/37336/diff/6/?file=1044858#file1044858line281> > > > > Is this API something you agreed on with your shepherd? I'm curious why > > we provide a blocking function rather than returning a future with the > > provided timeout. uhm... `wait()` is by definition blocking :) following our conversation, I'm going to change the name (and the signature) to return a Future instead > On Oct. 12, 2015, 4:08 p.m., Joris Van Remoortere wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, line 292 > > <https://reviews.apache.org/r/37336/diff/6/?file=1044858#file1044858line292> > > > > If this value is not meaningful until wait is finished, why not make > > this a future bound by that condition rather than providing undefined > > behavior? With the new signature, yes this will change too. BTW this is not "undefined" - it is created in a way that it makes sense even in the case of failures. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/#review102243 --- On Oct. 12, 2015, 12:29 p.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37336/ > --- > > (Updated Oct. 12, 2015, 12:29 p.m.) > > > Review request for mesos, Joris Van Remoortere and Michael Park. > > > Bugs: MESOS-3035 > https://issues.apache.org/jira/browse/MESOS-3035 > > > Repository: mesos > > > Description > --- > > Jira: MESOS-3035 > > The original API for `process::Subprocess` still left a lot of legwork > to do for the caller; we have now added a `wait(Duration timeout)` method > that returns a `CommandResult` (also introduced with this patch) which > contains useful information about the command invocation; the exit code; > stdout and, optionally, stderr too. > > The `wait()` method will wait for both the process to terminate, and > stdout/stderr to be available to read from; it also "unpacks" them into > ready-to-use `string`s. > > > Diffs > - > > 3rdparty/libprocess/include/process/subprocess.hpp > d2341a53aac7c779668ee80983f73158fd44bff5 > 3rdparty/libprocess/src/subprocess.cpp > d6ea62ed1c914d34e0e189395831c86fff8aac22 > 3rdparty/libprocess/src/tests/subprocess_tests.cpp > ab7515325e5db0a4fd222bb982f51243d7b7e39d > > Diff: https://reviews.apache.org/r/37336/diff/ > > > Testing > --- > > make check > > > Thanks, > > Marco Massenzio > >
Re: Review Request 37024: Exposes mesos version information in components.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37024/#review101506
---
Ship it!
Ship It!
- Marco Massenzio
On Oct. 5, 2015, 8 a.m., haosdent huang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37024/
> ---
>
> (Updated Oct. 5, 2015, 8 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1841
> https://issues.apache.org/jira/browse/MESOS-1841
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Add an endpoint that exposes component version.
>
>
> Diffs
> -
>
> src/Makefile.am e69892736b0edc8c264eaccd52a04d44d01f53ba
> src/exec/exec.cpp 7b51baaa8c08d248918974a3a22b6217e388bcb1
> src/local/main.cpp 18b2f0187637cd425d55c220f73faac5a1218f0f
> src/master/main.cpp bafc605d6c20bd264b932e44ee80373a3f692734
> src/sched/sched.cpp 571e00d303009a940f17c8ed4582749a718e846d
> src/slave/main.cpp 364dc7fc7ab2e3cef01aea7267dafa014b60e2b9
> src/version/version.hpp PRE-CREATION
> src/version/version.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37024/diff/
>
>
> Testing
> ---
>
> Manual test result:
>
> ```
> $ curl http://localhost:5050/version 2>/dev/null|jq .
>
> {
> "version": "0.24.0",
> "build_user": "haosdent",
> "build_time": 1439702338,
> "build_date": "2015-08-16 13:18:58"
> }
> ```
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 37024: Exposes mesos version information in components.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37024/#review101508
---
src/version/version.cpp (lines 81 - 82)
<https://reviews.apache.org/r/37024/#comment158928>
Minor nit: s/mesos/Apache Mesos and I think you can drop the "This
endpoint" - "Shows the current version and build information for Apache Mesos"
or something like that...
- Marco Massenzio
On Oct. 5, 2015, 8 a.m., haosdent huang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37024/
> ---
>
> (Updated Oct. 5, 2015, 8 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1841
> https://issues.apache.org/jira/browse/MESOS-1841
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Add an endpoint that exposes component version.
>
>
> Diffs
> -
>
> src/Makefile.am e69892736b0edc8c264eaccd52a04d44d01f53ba
> src/exec/exec.cpp 7b51baaa8c08d248918974a3a22b6217e388bcb1
> src/local/main.cpp 18b2f0187637cd425d55c220f73faac5a1218f0f
> src/master/main.cpp bafc605d6c20bd264b932e44ee80373a3f692734
> src/sched/sched.cpp 571e00d303009a940f17c8ed4582749a718e846d
> src/slave/main.cpp 364dc7fc7ab2e3cef01aea7267dafa014b60e2b9
> src/version/version.hpp PRE-CREATION
> src/version/version.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37024/diff/
>
>
> Testing
> ---
>
> Manual test result:
>
> ```
> $ curl http://localhost:5050/version 2>/dev/null|jq .
>
> {
> "version": "0.24.0",
> "build_user": "haosdent",
> "build_time": 1439702338,
> "build_date": "2015-08-16 13:18:58"
> }
> ```
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 37024: Exposes mesos version information in components.
> On Oct. 5, 2015, 6:48 p.m., Marco Massenzio wrote:
> > src/version/version.cpp, lines 81-82
> > <https://reviews.apache.org/r/37024/diff/6/?file=1090173#file1090173line81>
> >
> > Minor nit: s/mesos/Apache Mesos and I think you can drop the "This
> > endpoint" - "Shows the current version and build information for Apache
> > Mesos"
> >
> > or something like that...
Also, can you please make sure that in the commit comment we have a full
description of the endpoint's returned result (I also think that the
description above is no longer current?).
- Marco
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37024/#review101508
---
On Oct. 5, 2015, 8 a.m., haosdent huang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37024/
> ---
>
> (Updated Oct. 5, 2015, 8 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1841
> https://issues.apache.org/jira/browse/MESOS-1841
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Add an endpoint that exposes component version.
>
>
> Diffs
> -
>
> src/Makefile.am e69892736b0edc8c264eaccd52a04d44d01f53ba
> src/exec/exec.cpp 7b51baaa8c08d248918974a3a22b6217e388bcb1
> src/local/main.cpp 18b2f0187637cd425d55c220f73faac5a1218f0f
> src/master/main.cpp bafc605d6c20bd264b932e44ee80373a3f692734
> src/sched/sched.cpp 571e00d303009a940f17c8ed4582749a718e846d
> src/slave/main.cpp 364dc7fc7ab2e3cef01aea7267dafa014b60e2b9
> src/version/version.hpp PRE-CREATION
> src/version/version.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37024/diff/
>
>
> Testing
> ---
>
> Manual test result:
>
> ```
> $ curl http://localhost:5050/version 2>/dev/null|jq .
>
> {
> "version": "0.24.0",
> "build_user": "haosdent",
> "build_time": 1439702338,
> "build_date": "2015-08-16 13:18:58"
> }
> ```
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 38416: Allow HTTP response codes to be checked with code.
> On Oct. 1, 2015, 7:40 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/src/http.cpp, lines 75-160
> > <https://reviews.apache.org/r/38416/diff/3/?file=1084845#file1084845line75>
> >
> > +1 for the map
> >
> > I am also wondering if it can be templatized or something? maybe
> > augmenting the `StatusCode` class?
> >
> > (I've always felt that we should split the `code` -eg, 200; an `int`-
> > and the `reason` -`"OK"`; a `string`) - it makes comparing "classes" of
> > responses much easier.
> >
> > For example, in Python's `requests` this is what one can typically do:
> > ```
> > r = req.get('http://google.com')
> > if 200 <= r.status_code < 300:
> > # all is well
> > elif r.status_code >= 400:
> > # yuk! we hit the skids
> > ```
>
> Timothy Chen wrote:
> It's going to be an int as Ben suggested. About the status string, we
> currently just use the status string directly to assign to HTTP responses, so
> we could modify that we always return a code + status string. But I think
> until we see a use case where we really just want the status text portion
> I'll leave it as is.
Ok - whatever we return, let's make sure we are consistent with the RFC (2616)
so that we don't confuse client libraries: https://www.ietf.org/rfc/rfc2616.txt
(see section 6.1):
```
Status-Line = HTTP-Version SP Status-Code SP Reason-Phrase CRLF
```
(and I'm sorry, I'm missing context here, so this may not be applicable).
- Marco
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38416/#review101271
---
On Sept. 25, 2015, 10:38 p.m., Timothy Chen wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38416/
> ---
>
> (Updated Sept. 25, 2015, 10:38 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Jie Yu, Jojy Varghese,
> and Jiang Yan Xu.
>
>
> Bugs: MESOS-3429
> https://issues.apache.org/jira/browse/MESOS-3429
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Allow HTTP response codes to be checked with code.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/include/process/http.hpp
> fbd6cf7967173495875a8ea90ed28ade88b982a2
> 3rdparty/libprocess/src/decoder.hpp
> 67a5135f302153e376e8dfe8db82aa0b15449389
> 3rdparty/libprocess/src/http.cpp 9ad613a16c379b6d76a9a0f8d6160fe23a182fd4
> 3rdparty/libprocess/src/process.cpp
> 4afa30569b4d235637b49a624602e6b199c32e0e
> 3rdparty/libprocess/src/tests/benchmarks.cpp
> 97c81b8e61a75771e5bf7d46505cec4e0c0f404a
> 3rdparty/libprocess/src/tests/http_tests.cpp
> d0b9399d38fa284466a012a21080b1d9007af98b
> 3rdparty/libprocess/src/tests/process_tests.cpp
> 1023500ac2e3c55be955c4686e7f720eba6b4cc9
>
> Diff: https://reviews.apache.org/r/38416/diff/
>
>
> Testing
> ---
>
> make
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 38473: Add flag to disable hostname lookup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38473/ --- (Updated Sept. 22, 2015, 12:22 a.m.) Review request for mesos, Benjamin Hindman, Cody Maloney, and Neil Conway. Changes --- Removed dependency from LIBPROCESS_IP global invariant, after conversation with benh. Using instead the Master::info::ip Bugs: MESOS-3457 https://issues.apache.org/jira/browse/MESOS-3457 Repository: mesos Description --- Under certain circumstances, dynamic lookup of hostname, while successful, provides undesirable results; we would also like, in those circumstances, be able to just set the hostname to the chosen IP address (possibly set via the --ip_discovery_command method). The flag we add here, --[no]-hostname_lookup is `true` by default (which is the existing behavior) and will work under most circumstances: however, by disabling lookup (using, for example, --no_hostname_lookup) the hostname used will be the same as the one configured (possibly, via --ip or MESOS_IP) in `LIBPROCESS_IP`. This change affects both Master/Agent nodes. WARNING - the `Address::hostname()` method always does a dynamic hostname lookup, which may in fact return inconsistent results wrt the Master's configuration (this is *not* affected by this change) and should be avoided; use instead `Master::info()::hostname()` which is always consistent with the Master's configuration. Diffs (updated) - docs/configuration.md dd7f4aa806a3c1a8653a0fda9a326a3707308e7c src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf src/master/flags.cpp 80879611fbcfd764c9fc8f60a31613a9c8fc2364 src/master/master.cpp 90ef8c663c90ffbdcb4aa2377bfba65ea5d3fda9 src/slave/flags.hpp e31a4183170c3442ac4a15365c229391e7e91480 src/slave/flags.cpp add4196dfd06c0f602ff5ebd39960dc05c4cd11f src/slave/slave.cpp ad710d7b930a2f115d503ceb8f8fd7421ad30287 src/tests/cluster.hpp 114583de8c867495a2b7a953e6826708838e5d23 src/tests/master_tests.cpp f26344d39543f65f2b0a94b8ff566836c8256bf7 Diff: https://reviews.apache.org/r/38473/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 38473: Add flag to disable hostname lookup.
> On Sept. 18, 2015, 10:57 p.m., Cong Wang wrote: > > Why not just set --host_name=$LIBPROCESS_IP for your case since you anyway > > need to provide a flag? > > Guangya Liu wrote: > I also have the same question with Cong, @Marco, can you please show more > detail for why not using the solution as above? What is the benefit of this > compared to using --host_name=$LIBPROCESS_IP? Thanks. > > Marco Massenzio wrote: > `$LIBPROCESS_IP` may not be set by the time we run the binary - it is > actually set in the `main()` method (of both Master/Agent) after parsing the > `--ip` (and `--ip_discovery_command`) flags. > > The reason I use `LIBPROCESS_IP` here is to ensure that the IP used by > `libprocess` (which is initialized before creating the `Master` or `Slave` > object) is consistent with what the `Master/Agent` will be configured with. > > Please also refer to the conversation about the introduction of > `--ip_discovery_command` as to why we need a "dynamic" way of configuring > hostname/IP - it all boils down to make Mesos play nice in various Cloud (and > Enterprise data centers) where the DNS resolution (and hostname configuration > of VMs) is vastly different and may not necessarily conform to the generally > assumed scenario, where IP/hostname are kept in sync / resolved by the OS > and/or external services (DHCP/DNS). > > Qian Zhang wrote: > If you want to set hostname to the chosen IP, then I think you can just > use the existing flag "--hostname" (set it to the chosen IP manually). And if > "--hostname" is set, then I think the hostname lookup will be bypassed. So > why do we need to introduce a new flag "--no_hostname_lookup"? Please refer to the conversation on https://issues.apache.org/jira/browse/MESOS-3154 for why this may not be possible in the above-mentioned environments. (and, yes, we do know about the `--hostname` flag - unfortunately, under certain scenarios, it's not possible to set it to a "static" value) - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38473/#review99628 --- On Sept. 19, 2015, 1:25 a.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38473/ > --- > > (Updated Sept. 19, 2015, 1:25 a.m.) > > > Review request for mesos, Benjamin Hindman, Cody Maloney, and Neil Conway. > > > Bugs: MESOS-3457 > https://issues.apache.org/jira/browse/MESOS-3457 > > > Repository: mesos > > > Description > --- > > Under certain circumstances, dynamic lookup of hostname, while > successful, provides undesirable results; we would also like, in > those circumstances, be able to just set the hostname to the chosen > IP address (possibly set via the --ip_discovery_command method). > > The flag we add here, --[no]-hostname_lookup is `true` by default > (which is the existing behavior) and will work under most > circumstances: however, by disabling lookup (using, for example, > --no_hostname_lookup) the hostname used will be the same as the > one configured (possibly, via --ip or MESOS_IP) in `LIBPROCESS_IP`. > > This change affects both Master/Agent nodes. > > WARNING - the `Address::hostname()` method always does a dynamic > hostname lookup, which may in fact return inconsistent results > wrt the Master's configuration (this is *not* affected by > this change) and should be avoided; use instead > `Master::info()::hostname()` which is always consistent with > the Master's configuration. > > > Diffs > - > > docs/configuration.md dd7f4aa806a3c1a8653a0fda9a326a3707308e7c > src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf > src/master/flags.cpp 80879611fbcfd764c9fc8f60a31613a9c8fc2364 > src/master/master.cpp ca4d5876dcd427964111428edc22d567ddaede0b > src/slave/flags.hpp e31a4183170c3442ac4a15365c229391e7e91480 > src/slave/flags.cpp add4196dfd06c0f602ff5ebd39960dc05c4cd11f > src/slave/slave.cpp ad710d7b930a2f115d503ceb8f8fd7421ad30287 > src/tests/cluster.hpp 114583de8c867495a2b7a953e6826708838e5d23 > src/tests/master_tests.cpp a477794df37c658b80d79dea8555b001415f7b6a > src/tests/mesos.hpp 3db97aca921c9216d90384e1eb17030849516454 > > Diff: https://reviews.apache.org/r/38473/diff/ > > > Testing > --- > > make check > > > Thanks, > > Marco Massenzio > >
Re: Review Request 38457: CMake: Fix MESOS-3250, a dynamic load error in Stout tests on OS X.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38457/#review99757 --- Ship it! Ship It! - Marco Massenzio On Sept. 21, 2015, 2:43 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38457/ > --- > > (Updated Sept. 21, 2015, 2:43 a.m.) > > > Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph > Wu. > > > Bugs: MESOS-3250 > https://issues.apache.org/jira/browse/MESOS-3250 > > > Repository: mesos > > > Description > --- > > A few third-party libraries (libev, gmock) do not have `make install` > commands available, so below we have to add our own "install" commands. > > The reason is: if we do not, we get runtime library load problems on OS > X. In particular, `dydl` will look for these libraries at the prefix we > passed to `configure` (or in `/usr/local` if we did not pass a prefix > in), but since they don't have a `make install` step, they never get > placed in the prefix folder. > > Our solution is to: > (1) make a lib directory inside the Mesos folder for each of the > libraries that has no install step, and > (2) copy all such libraries into their respective directories. > > (Note that step (1) is not only convenient, but important: make will add > a `lib` to the end of your prefix path when linking, and since the built > libraries end up in a `.libs` folder, it's not enough to simply pass the > build directory into `configure` as a prefix; so if we're going to move > the libraries, we might as well move them to a library folder.) > > > Diffs > - > > 3rdparty/libprocess/3rdparty/CMakeLists.txt > b9c9fae7d448906e9c9f5ab0ee3fe138a0171a7d > > Diff: https://reviews.apache.org/r/38457/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 38473: Add flag to disable hostname lookup.
> On Sept. 21, 2015, 7:39 p.m., Benjamin Hindman wrote: > > src/master/master.cpp, line 345 > > <https://reviews.apache.org/r/38473/diff/3/?file=1077335#file1077335line345> > > > > Why are you reading LIBPROCESS_IP here? Why not `flags.ip`? There > > appears to be a very non-obvious global invariant and I'd like to capture > > the dependency here as a comment (for posterity!) as well as where we're > > setting LIBPROCESS_IP in the main.cpp files. I've added the following comment, please let me know if (a) I'm getting this wrong and (b) whether it requires a more detailed explanation (but the underlying rationale is - libprocess will use that (and not the flags) and we need to be consistent with it). ``` // We need to use LIBPROCESS_IP here, instead of 'flags.ip' because the // latter may not have been set, and the IP may have been set by other // means (eg, auto-discovery; the --ip_discovery_command) and we need to // be consistent with what 'libprocess' is initialized with. ``` > On Sept. 21, 2015, 7:39 p.m., Benjamin Hindman wrote: > > src/master/master.cpp, line 349 > > <https://reviews.apache.org/r/38473/diff/3/?file=1077335#file1077335line349> > > > > In circumstances where a user can easily avoid the master from crashing > > we prefer NOT to use LOG(FATAL) because it prints a stack trace which can > > hide the actual error message. Instead, an EXIT(EXIT_FAILURE) is a good > > thing to use here. > > > > Same for the LOG(FATAL) in the slave below. ah! I was trying to be "consistent" with the (existing) code above. Would you like me to change that one too? > On Sept. 21, 2015, 7:39 p.m., Benjamin Hindman wrote: > > src/tests/master_tests.cpp, line 1121 > > <https://reviews.apache.org/r/38473/diff/3/?file=1077340#file1077340line1121> > > > > Why not just do `cluster.find`? Not sure I understand the need for this > > alias. You are right, no need - I started out adding this, then eventually gone down the rabbit hole of same-named classes and hadn't realized that I could actually get rid of the alias. It's gone. > On Sept. 21, 2015, 7:39 p.m., Benjamin Hindman wrote: > > src/tests/master_tests.cpp, lines 1122-1123 > > <https://reviews.apache.org/r/38473/diff/3/?file=1077340#file1077340line1122> > > > > What does someone running the test get from this extra output > > information? Mostly the hostname that was set up and then not found via the UPID lookup - it may help debug weird network configuration issues and/or situations where we have broken the hostname lookup/configuration. (I think? I like to add extra info when tests go wrong, so as to take out (some of) the guesswork from the poor soul who'll have to fix my code many months/years in the future... I believe in karma :) ) Anyways, figured out that (a) this needs to be an `ASSERT_EQ` (as we `master.get()` in the following EXPECT) and (b) the comment here may actually be unnecessary, so I've removed it. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38473/#review99812 --- On Sept. 19, 2015, 1:25 a.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38473/ > --- > > (Updated Sept. 19, 2015, 1:25 a.m.) > > > Review request for mesos, Benjamin Hindman, Cody Maloney, and Neil Conway. > > > Bugs: MESOS-3457 > https://issues.apache.org/jira/browse/MESOS-3457 > > > Repository: mesos > > > Description > --- > > Under certain circumstances, dynamic lookup of hostname, while > successful, provides undesirable results; we would also like, in > those circumstances, be able to just set the hostname to the chosen > IP address (possibly set via the --ip_discovery_command method). > > The flag we add here, --[no]-hostname_lookup is `true` by default > (which is the existing behavior) and will work under most > circumstances: however, by disabling lookup (using, for example, > --no_hostname_lookup) the hostname used will be the same as the > one configured (possibly, via --ip or MESOS_IP) in `LIBPROCESS_IP`. > > This change affects both Master/Agent nodes. > > WARNING - the `Address::hostname()` method always does a dynamic > hostname lookup, which may in fact return inconsistent results > wrt the Master's configuration (this is *not* affected by > this change) and should be avoided; use instead
Re: Review Request 38473: Add flag to disable hostname lookup.
> On Sept. 18, 2015, 10:57 p.m., Cong Wang wrote: > > Why not just set --host_name=$LIBPROCESS_IP for your case since you anyway > > need to provide a flag? > > Guangya Liu wrote: > I also have the same question with Cong, @Marco, can you please show more > detail for why not using the solution as above? What is the benefit of this > compared to using --host_name=$LIBPROCESS_IP? Thanks. > > Marco Massenzio wrote: > `$LIBPROCESS_IP` may not be set by the time we run the binary - it is > actually set in the `main()` method (of both Master/Agent) after parsing the > `--ip` (and `--ip_discovery_command`) flags. > > The reason I use `LIBPROCESS_IP` here is to ensure that the IP used by > `libprocess` (which is initialized before creating the `Master` or `Slave` > object) is consistent with what the `Master/Agent` will be configured with. > > Please also refer to the conversation about the introduction of > `--ip_discovery_command` as to why we need a "dynamic" way of configuring > hostname/IP - it all boils down to make Mesos play nice in various Cloud (and > Enterprise data centers) where the DNS resolution (and hostname configuration > of VMs) is vastly different and may not necessarily conform to the generally > assumed scenario, where IP/hostname are kept in sync / resolved by the OS > and/or external services (DHCP/DNS). > > Qian Zhang wrote: > If you want to set hostname to the chosen IP, then I think you can just > use the existing flag "--hostname" (set it to the chosen IP manually). And if > "--hostname" is set, then I think the hostname lookup will be bypassed. So > why do we need to introduce a new flag "--no_hostname_lookup"? > > Marco Massenzio wrote: > Please refer to the conversation on > https://issues.apache.org/jira/browse/MESOS-3154 for why this may not be > possible in the above-mentioned environments. > (and, yes, we do know about the `--hostname` flag - unfortunately, under > certain scenarios, it's not possible to set it to a "static" value) > > Cong Wang wrote: > Makes sense for me, can you document it in configuration.md too? Thanks. >From what I've seen, configuration.md is essentially a verbatim list of what >is emitted by --help, not sure what more to add there; the >`--no-hostname_lookup` flag will be a no-op for anyone who doesn't know how to >use - and completely obvious to those (such our SRE team) who need it :) But I will definitely add this to the Jira (which will be referenced to by this commit) so that folks can understand where did this come from, no problem! - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38473/#review99628 --- On Sept. 19, 2015, 1:25 a.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38473/ > --- > > (Updated Sept. 19, 2015, 1:25 a.m.) > > > Review request for mesos, Benjamin Hindman, Cody Maloney, and Neil Conway. > > > Bugs: MESOS-3457 > https://issues.apache.org/jira/browse/MESOS-3457 > > > Repository: mesos > > > Description > --- > > Under certain circumstances, dynamic lookup of hostname, while > successful, provides undesirable results; we would also like, in > those circumstances, be able to just set the hostname to the chosen > IP address (possibly set via the --ip_discovery_command method). > > The flag we add here, --[no]-hostname_lookup is `true` by default > (which is the existing behavior) and will work under most > circumstances: however, by disabling lookup (using, for example, > --no_hostname_lookup) the hostname used will be the same as the > one configured (possibly, via --ip or MESOS_IP) in `LIBPROCESS_IP`. > > This change affects both Master/Agent nodes. > > WARNING - the `Address::hostname()` method always does a dynamic > hostname lookup, which may in fact return inconsistent results > wrt the Master's configuration (this is *not* affected by > this change) and should be avoided; use instead > `Master::info()::hostname()` which is always consistent with > the Master's configuration. > > > Diffs > - > > docs/configuration.md dd7f4aa806a3c1a8653a0fda9a326a3707308e7c > src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf > src/master/flags.cpp 80879611fbcfd764c9fc8f60a31613a9c8fc2364 > src/master/master.c
Re: Review Request 38473: Add flag to disable hostname lookup.
> On Sept. 18, 2015, 10:57 p.m., Cong Wang wrote: > > Why not just set --host_name=$LIBPROCESS_IP for your case since you anyway > > need to provide a flag? > > Guangya Liu wrote: > I also have the same question with Cong, @Marco, can you please show more > detail for why not using the solution as above? What is the benefit of this > compared to using --host_name=$LIBPROCESS_IP? Thanks. `$LIBPROCESS_IP` may not be set by the time we run the binary - it is actually set in the `main()` method (of both Master/Agent) after parsing the `--ip` (and `--ip_discovery_command`) flags. The reason I use `LIBPROCESS_IP` here is to ensure that the IP used by `libprocess` (which is initialized before creating the `Master` or `Slave` object) is consistent with what the `Master/Agent` will be configured with. Please also refer to the conversation about the introduction of `--ip_discovery_command` as to why we need a "dynamic" way of configuring hostname/IP - it all boils down to make Mesos play nice in various Cloud (and Enterprise data centers) where the DNS resolution (and hostname configuration of VMs) is vastly different and may not necessarily conform to the generally assumed scenario, where IP/hostname are kept in sync / resolved by the OS and/or external services (DHCP/DNS). - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38473/#review99628 --- On Sept. 19, 2015, 1:25 a.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38473/ > --- > > (Updated Sept. 19, 2015, 1:25 a.m.) > > > Review request for mesos, Benjamin Hindman, Cody Maloney, and Neil Conway. > > > Bugs: MESOS-3457 > https://issues.apache.org/jira/browse/MESOS-3457 > > > Repository: mesos > > > Description > --- > > Under certain circumstances, dynamic lookup of hostname, while > successful, provides undesirable results; we would also like, in > those circumstances, be able to just set the hostname to the chosen > IP address (possibly set via the --ip_discovery_command method). > > The flag we add here, --[no]-hostname_lookup is `true` by default > (which is the existing behavior) and will work under most > circumstances: however, by disabling lookup (using, for example, > --no_hostname_lookup) the hostname used will be the same as the > one configured (possibly, via --ip or MESOS_IP) in `LIBPROCESS_IP`. > > This change affects both Master/Agent nodes. > > WARNING - the `Address::hostname()` method always does a dynamic > hostname lookup, which may in fact return inconsistent results > wrt the Master's configuration (this is *not* affected by > this change) and should be avoided; use instead > `Master::info()::hostname()` which is always consistent with > the Master's configuration. > > > Diffs > - > > docs/configuration.md dd7f4aa806a3c1a8653a0fda9a326a3707308e7c > src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf > src/master/flags.cpp 80879611fbcfd764c9fc8f60a31613a9c8fc2364 > src/master/master.cpp ca4d5876dcd427964111428edc22d567ddaede0b > src/slave/flags.hpp e31a4183170c3442ac4a15365c229391e7e91480 > src/slave/flags.cpp add4196dfd06c0f602ff5ebd39960dc05c4cd11f > src/slave/slave.cpp ad710d7b930a2f115d503ceb8f8fd7421ad30287 > src/tests/cluster.hpp 114583de8c867495a2b7a953e6826708838e5d23 > src/tests/master_tests.cpp a477794df37c658b80d79dea8555b001415f7b6a > src/tests/mesos.hpp 3db97aca921c9216d90384e1eb17030849516454 > > Diff: https://reviews.apache.org/r/38473/diff/ > > > Testing > --- > > make check > > > Thanks, > > Marco Massenzio > >
Re: Review Request 38457: CMake: Fix MESOS-3250, a dynamic load error in Stout tests on OS X.
> On Sept. 17, 2015, 4:44 p.m., Marco Massenzio wrote: > > I'm excited about this! > > Tested on OSX and (at least, as far as stout_tests go, it works, yay!) > > > > A quick question: why, instead of moving the files, we don't simply add a > > symlink that will "look right" to the linker etc. > > (this may be an exceedingly dumb question, due to my extremely limited > > understanding of the build system) > > > > Thanks, Alex, for doing this - can't wait to bid farewell to CDT for good. > > Alex Clemmer wrote: > I don't know that there's a particular reason to _not_ use a symlink, but > the reasons I went with a hard copy are: > > (1) It's not clear that it was easier or cleaner to symlink. > (2) Every other install command does a hard copy and it might lead to > weird problems if we're not consistent. > (3) Not 100% sure about the differences between Windows and Linux > symlinks, and it's better to be safe than sorry. > > Would love to hear from you if you think these are not good reasons, as I > actually (in spite of how it may appear) basically have no idea what I'm > doing. (1) it is probably "cleaner" to the extent you only have a copy of the actual files - everything else points to that "source of truth" (while I type this, I realize that in the era of 5TB for < 100 bucks HDs, I sound like my great-granpa :) (2) I have my own opinion about consistency... (I totally know you'll love it: I did when I was at G) http://despair.com/collections/demotivators/products/consistency (3) You're on your own there - I only found out Windows had symlinks last year (and I only found out because they didn't work the way one expected them to :) In general, I prefer *soft* links (`ln -s`) in Linux as they are fast and inexpensive and easy to manage - copying files always leaves me with the worry the copies will go out of sync. But #3 seems like a winner in the argument. For now, I'd say, let's go with copies - let's get it to a "known good state" where it all works, then we can tinker with it to our heart's content later on and optimize it, if need be. Thanks! - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38457/#review99401 --- On Sept. 20, 2015, 2:03 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38457/ > --- > > (Updated Sept. 20, 2015, 2:03 a.m.) > > > Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph > Wu. > > > Bugs: MESOS-3250 > https://issues.apache.org/jira/browse/MESOS-3250 > > > Repository: mesos > > > Description > --- > > A few third-party libraries (libev, gmock) do not have `make install` > commands available, so below we have to add our own "install" commands. > > The reason is: if we do not, we get runtime library load problems on OS > X. In particular, `dydl` will look for these libraries at the prefix we > passed to `configure` (or in `/usr/local` if we did not pass a prefix > in), but since they don't have a `make install` step, they never get > placed in the prefix folder. > > Our solution is to: > (1) make a lib directory inside the Mesos folder for each of the > libraries that has no install step, and > (2) copy all such libraries into their respective directories. > > (Note that step (1) is not only convenient, but important: make will add > a `lib` to the end of your prefix path when linking, and since the built > libraries end up in a `.libs` folder, it's not enough to simply pass the > build directory into `configure` as a prefix; so if we're going to move > the libraries, we might as well move them to a library folder.) > > > Diffs > - > > 3rdparty/libprocess/3rdparty/CMakeLists.txt > b9c9fae7d448906e9c9f5ab0ee3fe138a0171a7d > > Diff: https://reviews.apache.org/r/38457/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 38473: Add flag to disable hostname lookup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38473/ --- (Updated Sept. 18, 2015, 9:53 p.m.) Review request for mesos, Benjamin Hindman, Cody Maloney, and Neil Conway. Changes --- Addressed comments + minor cosmetic changes. Bugs: MESOS-3457 https://issues.apache.org/jira/browse/MESOS-3457 Repository: mesos Description --- Under certain circumstances, dynamic lookup of hostname, while successful, provides undesirable results; we would also like, in those circumstances, be able to just set the hostname to the chosen IP address (possibly set via the --ip_discovery_command method). The flag we add here, --[no]-hostname_lookup is `true` by default (which is the existing behavior) and will work under most circumstances: however, by disabling lookup (using, for example, --no_hostname_lookup) the hostname used will be the same as the one configured (possibly, via --ip or MESOS_IP) in `LIBPROCESS_IP`. This change affects both Master/Agent nodes. WARNING - the `Address::hostname()` method always does a dynamic hostname lookup, which may in fact return inconsistent results wrt the Master's configuration (this is *not* affected by this change) and should be avoided; use instead `Master::info()::hostname()` which is always consistent with the Master's configuration. Diffs (updated) - docs/configuration.md dd7f4aa806a3c1a8653a0fda9a326a3707308e7c src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf src/master/flags.cpp 80879611fbcfd764c9fc8f60a31613a9c8fc2364 src/master/master.cpp ca4d5876dcd427964111428edc22d567ddaede0b src/slave/flags.hpp e31a4183170c3442ac4a15365c229391e7e91480 src/slave/flags.cpp add4196dfd06c0f602ff5ebd39960dc05c4cd11f src/slave/slave.cpp ad710d7b930a2f115d503ceb8f8fd7421ad30287 src/tests/cluster.hpp 114583de8c867495a2b7a953e6826708838e5d23 src/tests/master_tests.cpp a477794df37c658b80d79dea8555b001415f7b6a src/tests/mesos.hpp 3db97aca921c9216d90384e1eb17030849516454 Diff: https://reviews.apache.org/r/38473/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 38454: Fix failed test LimitedCpuIsolatorTest.ROOT_CGROUPS_Pids_and_Tids
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38454/#review99400 --- Thanks for fixing this, but I'm wondering how this fixes the issue. (in fact, not even what is it testing). If you invoke `cat` without input, it will just hang there waiting on STDIN, how does this make the test work? (sorry for all the questions, but unfortunately I'm not familiar w/ this particular test) Can you please add more information in the Description field? It's pretty obvious (or it should be :) ) that a patch fixes the bug it's related to, but what folks need to understand is: (a) what the problem was; (b) how it affected the test and (c) what you did to fix it. Also, how does this preserve the spirit of the test? I can fix any test by just having ASSERT(true) :) BTW - it's fantastic this fixed the issue and you did the repeat-cycle test! - Marco Massenzio On Sept. 17, 2015, 9:55 a.m., Jian Qiu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38454/ > --- > > (Updated Sept. 17, 2015, 9:55 a.m.) > > > Review request for mesos, haosdent huang, Marco Massenzio, and Vinod Kone. > > > Bugs: MESOS-3293 > https://issues.apache.org/jira/browse/MESOS-3293 > > > Repository: mesos > > > Description > --- > > Fix failed test LimitedCpuIsolatorTest.ROOT_CGROUPS_Pids_and_Tids > > > Diffs > - > > src/tests/containerizer/isolator_tests.cpp > a25ae97a519feb8ead6177da160df8a276ca15bf > > Diff: https://reviews.apache.org/r/38454/diff/ > > > Testing > --- > > ./mesos-tests.sh > --gtest_filter="LimitedCpuIsolatorTest.ROOT_CGROUPS_Pids_and_Tids" > --gtest_repeat=1000 --gtest_break_on_failure > > > Thanks, > > Jian Qiu > >
Re: Review Request 38457: CMake: Fix MESOS-3250, a dynamic load error in Stout tests on OS X.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38457/#review99401 --- I'm excited about this! Tested on OSX and (at least, as far as stout_tests go, it works, yay!) A quick question: why, instead of moving the files, we don't simply add a symlink that will "look right" to the linker etc. (this may be an exceedingly dumb question, due to my extremely limited understanding of the build system) Thanks, Alex, for doing this - can't wait to bid farewell to CDT for good. - Marco Massenzio On Sept. 17, 2015, 10:03 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38457/ > --- > > (Updated Sept. 17, 2015, 10:03 a.m.) > > > Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph > Wu. > > > Repository: mesos > > > Description > --- > > A few third-party libraries (libev, gmock) do not have `make install` > commands available, so below we have to add our own "install" commands. > > The reason is: if we do not, we get runtime library load problems on OS > X. In particular, `dydl` will look for these libraries at the prefix we > passed to `configure` (or in `/usr/local` if we did not pass a prefix > in), but since they don't have a `make install` step, they never get > placed in the prefix folder. > > Our solution is to: > (1) make a lib directory inside the Mesos folder for each of the > libraries that has no install step, and > (2) copy all such libraries into their respective directories. > > (Note that step (1) is not only convenient, but important: make will add > a `lib` to the end of your prefix path when linking, and since the built > libraries end up in a `.libs` folder, it's not enough to simply pass the > build directory into `configure` as a prefix; so if we're going to move > the libraries, we might as well move them to a library folder.) > > > Diffs > - > > 3rdparty/libprocess/3rdparty/CMakeLists.txt > d13ba666740b4f2e382a0b1852724cfd519f8f64 > > Diff: https://reviews.apache.org/r/38457/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38259/#review98983 --- LGTM Can you please add a test with a few vars defined in Env and then in Cmd Line too, and verify that it "works as intended"? Thanks for bearing with me and changing your code. I think it looks clean and nice now :) - Marco Massenzio On Sept. 13, 2015, 5:30 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38259/ > --- > > (Updated Sept. 13, 2015, 5:30 a.m.) > > > Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park. > > > Bugs: MESOS-3340 > https://issues.apache.org/jira/browse/MESOS-3340 > > > Repository: mesos > > > Description > --- > > Currently, it appears that re-defining a flag on the command-line that was > already defined via a OS Env var (MESOS_*) causes the Master to fail with a > not very helpful message. > > For example, if one has MESOS_QUORUM defined, this happens: > > $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1 > --hostname=192.168.1.4 --ip=192.168.1.4 > Duplicate flag 'quorum' on command line > > which is not very helpful. > > Current solution is to throw error if any duplication; over-write may make > user confused about the result. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd6 > > Diff: https://reviews.apache.org/r/38259/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38259/#review98568
---
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 600 -
601)
<https://reviews.apache.org/r/38259/#comment155076>
the `+` should be at the end of the line (before the newline).
Can you also please change the error message to:
```
"Command line flag '" + name +
" duplicates environment variable definition with the same name"
```
also, please feel free to use all 80 columns.
Now, an interesting question (for the shepherd/committer to decide) is
whether we should just emit a WARN and have the command line (explicit) setting
override the OS Env (implicit).
Also, whether we should only WARN, but proceed, if the **values** are the
same:
```MESOS_IP=127.0.0.1
... --ip=127.0.0.1
```
could (arguably) be considered a valid setting.
As a general approach, I'd suggest however to stick with current behavior,
to avoid breaking stuff that is currently working just fine (and relying on the
executable to error out in case of misconfigurations).
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 605)
<https://reviews.apache.org/r/38259/#comment155077>
nit: please retain `on` as in the original message.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 672 -
681)
<https://reviews.apache.org/r/38259/#comment155078>
would it be possible to factor out all this into a helper method?
It's essentialy a copy & paste of the same code above.
so long as it was only a couple of lines of code (maybe) it made sense to
repeat it - here, it seems redundant.
(and, obviously, same comments apply)
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp (line 406)
<https://reviews.apache.org/r/38259/#comment155079>
this would be a good opportunity to replace this test with something that
relies less on exact string match, maybe?
in pseudo-code, I'd do something like:
```
EXPECT_TRUE(lower(load.error()).contains("duplicate") &&
contains(flag_name))
```
(also, if you have a helper method, you can just unit test that one - once
- for both cases; without having to repeat the test, below)
Thanks for doing this!
A few comments, but generally good.
Not sure if you have a shepherd for this - if not, please let me know and I can
help find one.
- Marco Massenzio
On Sept. 11, 2015, 5:24 a.m., Klaus Ma wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> ---
>
> (Updated Sept. 11, 2015, 5:24 a.m.)
>
>
> Review request for mesos, Bernd Mathiske and Marco Massenzio.
>
>
> Bugs: MESOS-3340
> https://issues.apache.org/jira/browse/MESOS-3340
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Currently, it appears that re-defining a flag on the command-line that was
> already defined via a OS Env var (MESOS_*) causes the Master to fail with a
> not very helpful message.
>
> For example, if one has MESOS_QUORUM defined, this happens:
>
> $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1
> --hostname=192.168.1.4 --ip=192.168.1.4
> Duplicate flag 'quorum' on command line
>
> which is not very helpful.
>
> Current solution is to throw error if any duplication; over-write may make
> user confused about the result.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd6
>
> Diff: https://reviews.apache.org/r/38259/diff/
>
>
> Testing
> ---
>
> make
> make check
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables
On Sept. 11, 2015, 6:56 a.m., Klaus Ma wrote: > > Thanks for doing this! > > A few comments, but generally good. > > > > Not sure if you have a shepherd for this - if not, please let me know and I > > can help find one. > > Klaus Ma wrote: > No shepherd now; that'll be great if you or Bernd can find a shepherd for > this one. ah - just noticed, it's @bernd -- all good, but please be aware he's away next week, he may take some time before he gets round to this. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38259/#review98568 --- On Sept. 11, 2015, 9:54 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38259/ > --- > > (Updated Sept. 11, 2015, 9:54 a.m.) > > > Review request for mesos, Bernd Mathiske and Marco Massenzio. > > > Bugs: MESOS-3340 > https://issues.apache.org/jira/browse/MESOS-3340 > > > Repository: mesos > > > Description > --- > > Currently, it appears that re-defining a flag on the command-line that was > already defined via a OS Env var (MESOS_*) causes the Master to fail with a > not very helpful message. > > For example, if one has MESOS_QUORUM defined, this happens: > > $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1 > --hostname=192.168.1.4 --ip=192.168.1.4 > Duplicate flag 'quorum' on command line > > which is not very helpful. > > Current solution is to throw error if any duplication; over-write may make > user confused about the result. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd6 > > Diff: https://reviews.apache.org/r/38259/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables
> On Sept. 11, 2015, 6:56 a.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines
> > 672-681
> > <https://reviews.apache.org/r/38259/diff/2/?file=1068027#file1068027line672>
> >
> > would it be possible to factor out all this into a helper method?
> > It's essentialy a copy & paste of the same code above.
> >
> > so long as it was only a couple of lines of code (maybe) it made sense
> > to repeat it - here, it seems redundant.
> >
> > (and, obviously, same comments apply)
>
> Klaus Ma wrote:
> I'm thinking to define a const massage format for re-usage; but not sure
> where to add such a const format. For example:
>
> const string dup_err_msg("Duplicate flag '%s' in command line with
> environment"); // No sure where to put this const string :(.
> ...
> return Error(strings::format(dup_err_msg, name));
>
> And re-check those two log/function, it seems we can re-use some logic:
> in `FlagsBase::load(... const char* const* argv ...)`, dump `argv` and call
> `FlagsBase::load(... char*** argv ...)` to load the flags, the args that are
> not a flag.
there is a `master/constants.hpp` but it does not seem to be used for this - in
fact, all error messages/logs are hard-coded strings (I don't guess anyone ever
worried about internationalizing Mesos :D ) - I think your only option would be
to put it at the head of this file.
please make sure to use ALL_CAPS for the const name.
Also, please make sure to modify the message with the one I suggested, thanks.
- Marco
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38259/#review98568
---
On Sept. 11, 2015, 9:54 a.m., Klaus Ma wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> ---
>
> (Updated Sept. 11, 2015, 9:54 a.m.)
>
>
> Review request for mesos, Bernd Mathiske and Marco Massenzio.
>
>
> Bugs: MESOS-3340
> https://issues.apache.org/jira/browse/MESOS-3340
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Currently, it appears that re-defining a flag on the command-line that was
> already defined via a OS Env var (MESOS_*) causes the Master to fail with a
> not very helpful message.
>
> For example, if one has MESOS_QUORUM defined, this happens:
>
> $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1
> --hostname=192.168.1.4 --ip=192.168.1.4
> Duplicate flag 'quorum' on command line
>
> which is not very helpful.
>
> Current solution is to throw error if any duplication; over-write may make
> user confused about the result.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd6
>
> Diff: https://reviews.apache.org/r/38259/diff/
>
>
> Testing
> ---
>
> make
> make check
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 37336: Simplified the caller interface to process::Subprocess
he fact that it seems to "work as intended") would you be happy as to where it currently stands? or am I missing something that will come back and bite us? Thanks! - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/#review97945 --- On Aug. 20, 2015, 8:03 a.m., Marco Massenzio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37336/ > --- > > (Updated Aug. 20, 2015, 8:03 a.m.) > > > Review request for mesos and Joris Van Remoortere. > > > Bugs: MESOS-3035 > https://issues.apache.org/jira/browse/MESOS-3035 > > > Repository: mesos > > > Description > --- > > Jira: MESOS-3035 > > The original API for `process::Subprocess` still left a lot of legwork > to do for the caller; we have now added a `wait(Duration timeout)` method > that returns a `CommandResult` (also introduced with this patch) which > contains useful information about the command invocation; the exit code; > stdout and, optionally, stderr too. > > The `wait()` method will wait for both the process to terminate, and > stdout/stderr to be available to read from; it also "unpacks" them into > ready-to-use `string`s. > > > Diffs > - > > 3rdparty/libprocess/include/process/subprocess.hpp > d2341a53aac7c779668ee80983f73158fd44bff5 > 3rdparty/libprocess/src/subprocess.cpp > d6ea62ed1c914d34e0e189395831c86fff8aac22 > 3rdparty/libprocess/src/tests/subprocess_tests.cpp > ab7515325e5db0a4fd222bb982f51243d7b7e39d > > Diff: https://reviews.apache.org/r/37336/diff/ > > > Testing > --- > > make check > > > Thanks, > > Marco Massenzio > >
Re: Review Request 37532: Add QUIESCE call interface to the scheduler
> On Aug. 20, 2015, 7:39 p.m., Marco Massenzio wrote: > > include/mesos/scheduler/scheduler.proto, lines 314-315 > > <https://reviews.apache.org/r/37532/diff/1/?file=1042006#file1042006line314> > > > > This comments does not read well: what is the timeout? also, it would > > be good to have a bit of info about what the filters are > > > > (eg, are they 'inclusion' or 'exclusion' filters? etc.) > > Guangya Liu wrote: > Marco, can you pls show more the difference between "inclusion" and > "exclusion" filters? I'm not quite catch this point. Thanks. Well, I am assuming that the `filters` here will take a conditional action based on a `Predicate`? (I am sorry, I don't really know what the `Filters` are for here - this was part of what I was asking for more documentation about this - please assume that the reader may not have access to all the source code - this is certainly true for people using Java bindings, who only usually look at the javadoc). So, if you have method `foo()` that takes some `filters` to take some `action` on a given set of objects - the filters are "inclusive" if they will result in `action` being taken only on those objects for which `Predicate(Object) == true` - "exclusion" filters will instead **not** take action under the same circumstance. Think of it as `-v` in `grep` :) > On Aug. 20, 2015, 7:39 p.m., Marco Massenzio wrote: > > src/tests/scheduler_tests.cpp, line 1003 > > <https://reviews.apache.org/r/37532/diff/1/?file=1042011#file1042011line1003> > > > > so, this is a good test, but I would really like to see one where we > > ask Master to keep quiet for a while and we don't get offers during that > > period of time, then we start getting again. > > > > It may require some fiddling around with `Clock`s and all that jazz, > > but it would give us more confidence that this whole thing works. > > > > Also - some tests around more complex filtering (if any? maybe this is > > there is, then it's fine). > > Guangya Liu wrote: > I'm planning to add more test cases in another patch, make sense? Thanks. Ok - so long as the patches are related and will be committed together. Please make sure to mark them accordingly. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37532/#review95997 --- On Aug. 31, 2015, 1:23 a.m., Guangya Liu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37532/ > --- > > (Updated Aug. 31, 2015, 1:23 a.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-3037 > https://issues.apache.org/jira/browse/MESOS-3037 > > > Repository: mesos > > > Description > --- > > This is just part of MESOS-3037, this patch only add the interface > of QUIESCE call. > > > Diffs > - > > include/mesos/scheduler.hpp ee198b6955882f4f31466ca05429ca16fbf2f5cd > include/mesos/scheduler/scheduler.proto > 89daf8a6b74057ee156b3ad691397e76fcb835b8 > src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc > src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf > src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 > > Diff: https://reviews.apache.org/r/37532/diff/ > > > Testing > --- > > > Thanks, > > Guangya Liu > >
Re: Review Request 37532: Add QUIESCE call interface to the scheduler
> On Aug. 20, 2015, 7:39 p.m., Marco Massenzio wrote: > > include/mesos/scheduler/scheduler.proto, lines 314-315 > > <https://reviews.apache.org/r/37532/diff/1/?file=1042006#file1042006line314> > > > > This comments does not read well: what is the timeout? also, it would > > be good to have a bit of info about what the filters are > > > > (eg, are they 'inclusion' or 'exclusion' filters? etc.) > > Guangya Liu wrote: > Marco, can you pls show more the difference between "inclusion" and > "exclusion" filters? I'm not quite catch this point. Thanks. > > Marco Massenzio wrote: > Well, I am assuming that the `filters` here will take a conditional > action based on a `Predicate`? (I am sorry, I don't really know what the > `Filters` are for here - this was part of what I was asking for more > documentation about this - please assume that the reader may not have access > to all the source code - this is certainly true for people using Java > bindings, who only usually look at the javadoc). > > So, if you have method `foo()` that takes some `filters` to take some > `action` on a given set of objects - the filters are "inclusive" if they will > result in `action` being taken only on those objects for which > `Predicate(Object) == true` - "exclusion" filters will instead **not** take > action under the same circumstance. > > Think of it as `-v` in `grep` :) > > Guangya Liu wrote: > Thanks Marco. Here the filter is just a refused time out, when this time > out reached, the frameworks can get resource offers again. Quiesce offer will > set filters to frameworks to disable resource offers and revive offer will > clear the filter to enable resource offering. The quiesce offer will also be > revived when the filter time out reached. Great, thanks - I'd suggest to add this information to the javadoc: ``` // The quiesce message from the framework will disable resource offers until a Revive message clears the filter to enable resource offering. // When the timeout expires, the frameworks will start receiving resource offers again. ``` Or something to this effect (I'm sorry, I'm not that familiar with this part yet, so I may have gotten the finer details wrong). - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37532/#review95997 --- On Aug. 31, 2015, 1:23 a.m., Guangya Liu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37532/ > --- > > (Updated Aug. 31, 2015, 1:23 a.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-3037 > https://issues.apache.org/jira/browse/MESOS-3037 > > > Repository: mesos > > > Description > --- > > This is just part of MESOS-3037, this patch only add the interface > of QUIESCE call. > > > Diffs > - > > include/mesos/scheduler.hpp ee198b6955882f4f31466ca05429ca16fbf2f5cd > include/mesos/scheduler/scheduler.proto > 89daf8a6b74057ee156b3ad691397e76fcb835b8 > src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc > src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf > src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 > > Diff: https://reviews.apache.org/r/37532/diff/ > > > Testing > --- > > > Thanks, > > Guangya Liu > >
Re: Review Request 37684: Added symlink test for /bin, lib, and /lib64.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37684/#review96054 --- Ship it! Ship It! - Marco Massenzio On Aug. 21, 2015, 4:51 p.m., Greg Mann wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37684/ --- (Updated Aug. 21, 2015, 4:51 p.m.) Review request for mesos, Marco Massenzio and Vinod Kone. Bugs: MESOS-3296 and MESOS-3297 https://issues.apache.org/jira/browse/MESOS-3296 https://issues.apache.org/jira/browse/MESOS-3297 Repository: mesos Description --- Added symlink test for /bin, /lib, and /lib64. Diffs - src/tests/containerizer/rootfs.hpp 961003fd6768ee0f7effd2804d1c453f8d45e058 Diff: https://reviews.apache.org/r/37684/diff/ Testing --- On CentOS 7.1 and other distros with symlinked /bin, /lib, /lib64: sudo make check Some tests will fail due to other root test issues being tracked at MESOS-3292, MESOS-3293, MESOS-3294, MESOS-3295 This patch solves bugs in MesosContainerizerLaunchTest.ROOT_ChangeRootfs and the LinuxFilesystemIsolatorTest.* tests, so those should all pass. Thanks, Greg Mann
Re: Review Request 37336: [WIP] Added `wait()` method to process::Subprocess
On Aug. 17, 2015, 11:01 p.m., Ben Mahler wrote: In the same vein as os::shell, we should probably introduce an 'os' namespace in libprocess for asynchronous os utilities. In this case, process::os::shell which returns a Future of the output (although, ideally status, output, error). Sounds good. If I understand this correctly, the `process::os::shell` would take advantage of this refactoring, correct? (also, how about returning a `FutureCommandResult`: this has all of status, output, error and a bit more). Could you please take a look at the next revision I'm about to post: it now all works and passes the tests, but I'm somewhat confused as to why the `onFailed` never got called and another nit - they are marked as FIXME(marco) and will be obviously removed - I needed a way to mark those areas visibly: they are not TODOs). Thanks! - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/#review95658 --- On Aug. 15, 2015, 2:02 a.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/ --- (Updated Aug. 15, 2015, 2:02 a.m.) Review request for mesos and Joris Van Remoortere. Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description --- Jira: MESOS-3035 The original API for `process::Subprocess` still left a lot of legwork to do for the caller; we have now added a `wait(Duration timeout)` method that returns a `CommandResult` (also introduced with this patch) which contains useful information about the command invocation; the exit code; stdout and, optionally, stderr too. The `wait()` method will wait for both the process to terminate, and stdout/stderr to be available to read from; it also unpacks them into ready-to-use `string`s. This is still WIP as I'm seeing some unusual behavior and I'd like to discuss with someone more expert on libprocess. Diffs - 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 3rdparty/libprocess/src/subprocess.cpp d6ea62ed1c914d34e0e189395831c86fff8aac22 3rdparty/libprocess/src/tests/subprocess_tests.cpp ab7515325e5db0a4fd222bb982f51243d7b7e39d Diff: https://reviews.apache.org/r/37336/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 37532: Add QUIESCE call interface to the scheduler
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37532/#review95997 --- include/mesos/scheduler.hpp (line 273) https://reviews.apache.org/r/37532/#comment151190 indent is off? include/mesos/scheduler.hpp (line 424) https://reviews.apache.org/r/37532/#comment151195 indent include/mesos/scheduler/scheduler.proto (line 173) https://reviews.apache.org/r/37532/#comment151191 micro-nit: the // is not aligned with lines above. include/mesos/scheduler/scheduler.proto (lines 314 - 315) https://reviews.apache.org/r/37532/#comment151192 This comments does not read well: what is the timeout? also, it would be good to have a bit of info about what the filters are (eg, are they 'inclusion' or 'exclusion' filters? etc.) src/master/master.cpp (line 2507) https://reviews.apache.org/r/37532/#comment151193 IMO we should LOG(INFO) here to state this is not implemented yet or something. Otherwise, say people see this and start using it, then nothing happens, look at the logs (which give the impression that the Processing QUIESCE was successfully called) and will report it as a bug. src/sched/sched.cpp (line 1953) https://reviews.apache.org/r/37532/#comment151194 indent off src/tests/scheduler_tests.cpp (line 1003) https://reviews.apache.org/r/37532/#comment151196 so, this is a good test, but I would really like to see one where we ask Master to keep quiet for a while and we don't get offers during that period of time, then we start getting again. It may require some fiddling around with `Clock`s and all that jazz, but it would give us more confidence that this whole thing works. Also - some tests around more complex filtering (if any? maybe this is there is, then it's fine). - Marco Massenzio On Aug. 17, 2015, 1:47 p.m., Guangya Liu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37532/ --- (Updated Aug. 17, 2015, 1:47 p.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-3037 https://issues.apache.org/jira/browse/MESOS-3037 Repository: mesos Description --- This is just part of MESOS-3037, this patch only add the interface of QUIESCE call. Diffs - include/mesos/scheduler.hpp ee198b6955882f4f31466ca05429ca16fbf2f5cd include/mesos/scheduler/scheduler.proto 89daf8a6b74057ee156b3ad691397e76fcb835b8 include/mesos/v1/scheduler/scheduler.proto bd5e82a614b1163b29f9b20e562208efa1ba4b55 src/master/master.hpp 0432842d77beba024c7895291ca410964bae96be src/master/master.cpp c5e6c6f3304060d4c92d52851951f10bc432500e src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 src/tests/scheduler_tests.cpp 77c26353afc33f5099be2d1e597ffc630e559968 Diff: https://reviews.apache.org/r/37532/diff/ Testing --- Thanks, Guangya Liu
Re: Review Request 37617: Added an error message to MesosContainerizerLaunch command.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37617/#review95891 --- Ship it! Ship It! - Marco Massenzio On Aug. 19, 2015, 6:26 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37617/ --- (Updated Aug. 19, 2015, 6:26 p.m.) Review request for mesos, Marco Massenzio and Vinod Kone. Bugs: MESOS-3296 https://issues.apache.org/jira/browse/MESOS-3296 Repository: mesos Description --- Added an error message to MesosContainerizerLaunch command. So that we can triage MESOS-3296. Diffs - src/slave/containerizer/mesos/launch.cpp bd594d3f3d729aa000a1a7fbf9038f6cfcbb169b Diff: https://reviews.apache.org/r/37617/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 37584: Fix bug accessing error() when no Error
On Aug. 18, 2015, 6:34 p.m., Vinod Kone wrote: src/launcher/fetcher.cpp, lines 100-106 https://reviews.apache.org/r/37584/diff/1/?file=1043203#file1043203line100 just do return Error(Skipping fetch with Hadoop client: + (available.isError() ? availabe.error() : client not found)); So, that was my first choice, but I then reflected that my finding out the site of the error was made more complicated due to the available log lines being far away from it; so I had to do some digging and investigating. In general, I have been taught to prefer to have a LOG(ERROR) near the site where the error actually happens, as that also avoids running wild goose chases :) Especially if it so happens that the error message may be (unintentionally) misleading. What do you think? - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37584/#review95754 --- On Aug. 18, 2015, 5:24 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37584/ --- (Updated Aug. 18, 2015, 5:24 p.m.) Review request for mesos and Adam B. Bugs: MESOS-3287 https://issues.apache.org/jira/browse/MESOS-3287 Repository: mesos Description --- When trying to download an artifact with the Hadoop client, and the client is not `available()` we correctly return a false value, but then in the error message, we tried to make a call to `Try::error` which failed and crashed Master. This fixes it. Diffs - src/launcher/fetcher.cpp f8b46b8f957942d0cfdc45d3361f52dae3e514a3 Diff: https://reviews.apache.org/r/37584/diff/ Testing --- make check Thanks, Marco Massenzio
Review Request 37584: Fix bug accessing error() when no Error
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37584/ --- Review request for mesos and Adam B. Bugs: MESOS-3287 https://issues.apache.org/jira/browse/MESOS-3287 Repository: mesos Description --- When trying to download an artifact with the Hadoop client, and the client is not `available()` we correctly return a false value, but then in the error message, we tried to make a call to `Try::error` which failed and crashed Master. This fixes it. Diffs - src/launcher/fetcher.cpp f8b46b8f957942d0cfdc45d3361f52dae3e514a3 Diff: https://reviews.apache.org/r/37584/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 37208: Fix the spell error in help message of slave component.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37208/#review95602 --- Sorry it's taking so long - but I can't commit. I've asked one of my colleagues at Mesosphere to commit this. In general, no matter how small the patch, it's always advisable to have a shepherd (ie, someone with 'committer' status) to help commit the patch. - Marco Massenzio On Aug. 17, 2015, 6:50 a.m., Yong Qiao Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37208/ --- (Updated Aug. 17, 2015, 6:50 a.m.) Review request for mesos and Marco Massenzio. Bugs: MESOS-3228 https://issues.apache.org/jira/browse/MESOS-3228 Repository: mesos Description --- Fix the spell error in help message of slave component. Diffs - src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e Diff: https://reviews.apache.org/r/37208/diff/ Testing --- Thanks, Yong Qiao Wang
Re: Review Request 33339: Add a Java example framework to test persistent volumes.
throw, but you can just return)
src/examples/java/TestPersistentVolumeFramework.java (line 754)
https://reviews.apache.org/r/9/#comment150711
use `diamond pattern` of Java 7
src/examples/java/TestPersistentVolumeFramework.java (line 769)
https://reviews.apache.org/r/9/#comment150712
is there anything you should do here?
if not, can you please add a comment as to why you ignore this?
also, the log is pretty much irrelevant without a bit more info, if at all?
src/examples/java/TestPersistentVolumeFramework.java (line 860)
https://reviews.apache.org/r/9/#comment150713
factor this out into its own class
src/examples/java/TestPersistentVolumeFramework.java (line 878)
https://reviews.apache.org/r/9/#comment150715
s/slave/serverId
use Javadoc instead of // comments - this way they show up in the docs
src/examples/java/TestPersistentVolumeFramework.java (lines 889 - 901)
https://reviews.apache.org/r/9/#comment150716
use javadoc
also - is default (package) visibility what you want here?
src/examples/java/TestPersistentVolumeFramework.java (line 912)
https://reviews.apache.org/r/9/#comment150718
I think I already commented on my strong aversion to DIY classes.
Apache has already a Flags class - please re-use that implementation:
simplifies your code; less bugs to worry about; makes life easier for folks who
can rely on documentation/examples in an established library.
No need to re-invent the wheel :)
src/examples/java/TestPersistentVolumeFramework.java (lines 1065 - 1066)
https://reviews.apache.org/r/9/#comment150719
you really don't need these.
If you do decide to keep them, please place them at the top of the class,
where all CONSTANTS are expected to be.
src/examples/java/TestPersistentVolumeFramework.java (line 1068)
https://reviews.apache.org/r/9/#comment150720
please place your `main()` in a `Main` class and make sure we can run this
via a simple:
```
java -jar PersistenceExampleFramework.jar Main
```
(or, even better, add the necessary flags in the pom.xml so that a MANIFEST
is added as required).
src/examples/java/TestPersistentVolumeFramework.java (lines 1099 - 1103)
https://reviews.apache.org/r/9/#comment150722
```
int status = driver.run() == Status.DRIVER_STOPPED ? EXIT_SUCCESS :
EXIT_FAILURE;
```
src/examples/java/TestPersistentVolumeFramework.java (line 1105)
https://reviews.apache.org/r/9/#comment150721
unnecessary; use
```
return status;
```
src/tests/examples_tests.cpp (line 43)
https://reviews.apache.org/r/9/#comment150723
Can we please NOT add yet another test framework to the unit tests? already
to run `make check` takes forever...
src/tests/java_persistent_volume_framework_test.sh (lines 5 - 15)
https://reviews.apache.org/r/9/#comment150726
Even Bash deserves some respect :)
Please follow https://google-styleguide.googlecode.com/svn/trunk/shell.xml
```
HAS_ENV=$(env | grep MESOS_SOURCE_DIR)
if [[ -z ${HAS_ENV} ]]; then
echo ...
exit 1
fi
```
same for BUILD_DIR
- Marco Massenzio
On June 21, 2015, 9:57 a.m., haosdent huang wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/
---
(Updated June 21, 2015, 9:57 a.m.)
Review request for mesos, Adam B, Jie Yu, and Marco Massenzio.
Bugs: MESOS-2610
https://issues.apache.org/jira/browse/MESOS-2610
Repository: mesos
Description
---
Add a Java example framework to test persistent volumes.
Diffs
-
configure.ac 563e9c529444b3e980db6d04173f0d016a737c74
src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
src/examples/java/TestPersistentVolumeFramework.java PRE-CREATION
src/examples/java/test-persistent-volume-framework.in PRE-CREATION
src/tests/examples_tests.cpp 2ff6e7a449cc5037f9a3c8d6938855c35e389cca
src/tests/java_persistent_volume_framework_test.sh PRE-CREATION
Diff: https://reviews.apache.org/r/9/diff/
Testing
---
make check
Thanks,
haosdent huang
Re: Review Request 37555: Included /usr/bin/sh in the test root filesystem.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37555/#review95633 --- Ship it! a couple of nits, but LGTM. src/tests/containerizer/rootfs.hpp (line 108) https://reviews.apache.org/r/37555/#comment150742 why the trailing comma? src/tests/containerizer/rootfs.hpp (line 119) https://reviews.apache.org/r/37555/#comment150743 this seems very 'ad-hoc'? not sure what `rootfs-add()` does, but could we just add `/usr/bin` to `directories` above, maybe? Not a big deal, though. - Marco Massenzio On Aug. 17, 2015, 8:47 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37555/ --- (Updated Aug. 17, 2015, 8:47 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Bugs: MESOS-3050 https://issues.apache.org/jira/browse/MESOS-3050 Repository: mesos Description --- Included /usr/bin/sh in the test root filesystem. Diffs - src/tests/containerizer/rootfs.hpp 55dd4964cfb1ca0e5f7b7616ccc6d5ad2be135d7 Diff: https://reviews.apache.org/r/37555/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 37336: [WIP] Added `wait()` method to process::Subprocess
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/ --- (Updated Aug. 14, 2015, 8:17 a.m.) Review request for mesos and Joris Van Remoortere. Changes --- Updated the interface of the `wait()` method, and refined some of the control flow. Summary (updated) - [WIP] Added `wait()` method to process::Subprocess Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description (updated) --- Jira: MESOS-3035 The original API for `process::Subprocess` still left a lot of legwork to do for the caller; we have now added a `wait(Duration timeout)` method that returns a `CommandResult` (also introduced with this patch) which contains useful information about the command invocation; the exit code; stdout and, optionally, stderr too. The `wait()` method will wait for both the process to terminate, and stdout/stderr to be available to read from; it also unpacks them into ready-to-use `string`s. This is still WIP as I'm seeing some unusual behavior and I'd like to discuss with someone more expert on libprocess. Diffs (updated) - 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 3rdparty/libprocess/src/subprocess.cpp d6ea62ed1c914d34e0e189395831c86fff8aac22 3rdparty/libprocess/src/tests/subprocess_tests.cpp ab7515325e5db0a4fd222bb982f51243d7b7e39d Diff: https://reviews.apache.org/r/37336/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 37336: [WIP] Added `wait()` method to process::Subprocess
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/ --- (Updated Aug. 14, 2015, 8:20 a.m.) Review request for mesos and Joris Van Remoortere. Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description --- Jira: MESOS-3035 The original API for `process::Subprocess` still left a lot of legwork to do for the caller; we have now added a `wait(Duration timeout)` method that returns a `CommandResult` (also introduced with this patch) which contains useful information about the command invocation; the exit code; stdout and, optionally, stderr too. The `wait()` method will wait for both the process to terminate, and stdout/stderr to be available to read from; it also unpacks them into ready-to-use `string`s. This is still WIP as I'm seeing some unusual behavior and I'd like to discuss with someone more expert on libprocess. Diffs (updated) - 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 3rdparty/libprocess/src/subprocess.cpp d6ea62ed1c914d34e0e189395831c86fff8aac22 3rdparty/libprocess/src/tests/subprocess_tests.cpp ab7515325e5db0a4fd222bb982f51243d7b7e39d Diff: https://reviews.apache.org/r/37336/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 33339: Add a Java example framework to test persistent volumes.
On Aug. 14, 2015, 10:28 p.m., Marco Massenzio wrote: [mmm turns out that it matters WHICH boxes you put your general comments in :) - copied here, as they make sense *before* the nitpicking that follows] Again, sorry it's taken so long to get round to doing this review and s many thanks for doing this! I've only got halfway through, I'll try my best to do more in the next few days, less craziness (here's to hoping, anyway!) I notice the one file runs for 1,000 lines - the Resource class is probably worth having in its own .java file, and probably Flags too - maybe you can refactor further other parts too. In general, I like Java class files to only rarely exceed the 300-400 lines - bigger than that, it usually signals design choices that are sub-optimal in separating concerns. As I mentioned, it's great that you're doing this: as someone who wants to learn more about persistence framework, I'm looking forward to having this committed and being able to also hack around with it :) Maybe, we may also get a blog entry out of it, as we expose persistent volumes to a wider public and show folks how to use them in a Java framework. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9/#review95473 --- On June 21, 2015, 9:57 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9/ --- (Updated June 21, 2015, 9:57 a.m.) Review request for mesos, Adam B, Jie Yu, and Marco Massenzio. Bugs: MESOS-2610 https://issues.apache.org/jira/browse/MESOS-2610 Repository: mesos Description --- Add a Java example framework to test persistent volumes. Diffs - configure.ac 563e9c529444b3e980db6d04173f0d016a737c74 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/examples/java/TestPersistentVolumeFramework.java PRE-CREATION src/examples/java/test-persistent-volume-framework.in PRE-CREATION src/tests/examples_tests.cpp 2ff6e7a449cc5037f9a3c8d6938855c35e389cca src/tests/java_persistent_volume_framework_test.sh PRE-CREATION Diff: https://reviews.apache.org/r/9/diff/ Testing --- make check Thanks, haosdent huang
