> On Jan. 27, 2015, 5:58 p.m., Dominic Hamon wrote:
> > src/zookeeper/url.hpp, line 53
> > <https://reviews.apache.org/r/30195/diff/1/?file=830596#file830596line53>
> >
> > keep the * with the type. also, why is this a method?
> >
> > static const char prefix[] = "zk://"; will work fine.
>From a readability standpoint I want the prefix to be in the header where
>people can see what it is easily.
Giving the prefix in the header though gets me an error since we don't have
constexpr:
```
../../mesos/src/zookeeper/url.hpp: error: in-class initializer for static data
member of type 'const char [6]'
requires 'constexpr' specifier
static const char prefix[] = "zk://";
```
> On Jan. 27, 2015, 5:58 p.m., Dominic Hamon wrote:
> > docs/configuration.md, line 12
> > <https://reviews.apache.org/r/30195/diff/1/?file=830580#file830580line12>
> >
> > this is clumsy. maybe:
> >
> > "... where the file 'foo' must contain the value."
> >
> > note - is this a json format with key-value pairs? can i set a simple
> > string/int value with this?
> >
> > will this eventually allow us to move to a configuration file that has
> > all options as keys and values as values? :)
The format is exactly the same as what was accepted before, whether that is a
number, string, JSON, etc.
As far as for more general config file. My plan is not to implement a config
file, but rather what is the most "standard" args file I know of. There is some
value to keeping all the configuration for mesos in one place.
Args file documentation can be found at:
https://sourceware.org/binutils/docs/ld/Options.html#Options
https://gcc.gnu.org/onlinedocs/gcc/Overall-Options.html#Overall-Options
General syntax is '@filename', where that file is then opened and then it's
contents are fed in as arguments.
That said, maybe a JSON format (Since we use a lot of json args) would that
could be mapped fairly directly to what flags takes: `std::map<std::string,
Option<std::string>>`.
> On Jan. 27, 2015, 5:58 p.m., Dominic Hamon wrote:
> > docs/configuration.md, line 271
> > <https://reviews.apache.org/r/30195/diff/1/?file=830580#file830580line271>
> >
> > this is a really subtle api change and needs some thought. what if i
> > want to specify a protocol like hdfs? must this be a local file? can i pass
> > a relative path?
This only got half updated in one of the reworks of this patch... You can use
file:// still, you can't use '/path/to/file' anymore. We could definitely teach
the command line parser to use mesos-fetcher if the argument is a URI if we
felt like it as an extension to the file:// support.
I could teach credentials to handle the special case of starting with '/' since
in this case it is unambiguous so we accept the old /path/to/file, but that
seems like a lot of work for not much benefit. If you give
--credentials=/path/to/file you will get an error since that isn't valid
credentials format.
There is one which is a subtle change, --whitelist, where if you had used the
file://path/to/file format, it now means "read from path to file to find the
filename of the whitelist" rather than "path/to/file is the whitelist".
> On Jan. 27, 2015, 5:58 p.m., Dominic Hamon wrote:
> > src/credentials/credentials.hpp, line 68
> > <https://reviews.apache.org/r/30195/diff/1/?file=830581#file830581line68>
> >
> > don't use inline here. trust the compiler.
Unfortunately: "An explicit specialization of a function template is inline
only if it is declared with the inline specifier (or defined as deleted), it
doesn't matter if the primary template is inline."
(http://en.cppreference.com/w/cpp/language/template_specialization).
> On Jan. 27, 2015, 5:58 p.m., Dominic Hamon wrote:
> > src/credentials/credentials.hpp, line 62
> > <https://reviews.apache.org/r/30195/diff/1/?file=830581#file830581line62>
> >
> > can you please create jira tickets for these TODOs?
Done, MESOS-2280, MESOS-2281
> On Jan. 27, 2015, 5:58 p.m., Dominic Hamon wrote:
> > src/master/detector.cpp, line 222
> > <https://reviews.apache.org/r/30195/diff/1/?file=830584#file830584line222>
> >
> > this should be CHECK, i believe.
Updated these to be CHECK
- Cody
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30195/#review69835
-----------------------------------------------------------
On Jan. 27, 2015, 11:39 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30195/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2015, 11:39 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ken Sipe.
>
>
> Bugs: mesos-1806
> https://issues.apache.org/jira/browse/mesos-1806
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Mostly simplifies things. There are two places where there things get
> complicated
>
> 1. --whitelist: This code wants a file to watch for changes and update
> the whitelist from periodically. Now you have to specify that path
> without a 'file://' prefix since the file:// prefix would read the
> value out of a file.
> 2. --credential, --credentials: Custom parsing logic since the
> credentials can be given in two different formats. Once the old
> format is removed, either teaching <stout/flags> how to parse json
> to protobuf generally or taking the flag as JSON then converting it
> to protobuf later will make this clean.
>
>
> Diffs
> -----
>
> CHANGELOG f515bc937441a2b4cc7e33bb857cb48a21aedea0
> docs/configuration.md 22f9e3db7b0e1691018de5fe3dfea3cb908de4b9
> src/credentials/credentials.hpp 4cdadb1b6d5a607cee8caeb38f2cbf2e3ec5da7a
> src/examples/load_generator_framework.cpp
> 01a567b424140575ae0c369d026589300f173143
> src/master/contender.cpp c1bf82b621d6b46afe001acafe9ee53336726406
> src/master/detector.cpp 700eb9dde8e71648bacc00a82766634f77cf2d15
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/main.cpp e5e76ce16646eb0244227104038efeae5fbbbb2b
> src/master/master.cpp ab6d1d17367f199191b7c77bccec73ec3b112d4f
> src/slave/flags.hpp 0f6cc41d60a2e3bc2121cc438351135541ef99ba
> src/slave/slave.cpp fca83b3977b95ddda30f9830da10e124b5c605e6
> src/tests/credentials_tests.cpp 091b545c11cfa9deabf710366e15cfc0b966de0f
> src/tests/master_allocator_tests.cpp
> 2430622d09c7ef1e020e2eb8f97444e7efc7c8ea
> src/tests/master_contender_detector_tests.cpp
> d847a30d21b2a2980c6b7ceb62bbf61dc77487de
> src/tests/master_tests.cpp 678d27f41a2f246c714c77adb132263c0c2c61ed
> src/tests/mesos.cpp 5ed4df530cf1bf11eec3b29542641822e0f702b2
> src/watcher/whitelist_watcher.cpp b6e73d7f500931dec69f115e554f9cd7cb42c3ed
> src/zookeeper/url.hpp 16e711c5c0bc29b1967a20f0827238f8a7b0deaf
>
> Diff: https://reviews.apache.org/r/30195/diff/
>
>
> Testing
> -------
>
> make distcheck
> - Tests not hitting the file:// handling which were replaced with CHECK()
> statements.
>
>
> Thanks,
>
> Cody Maloney
>
>
