mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Klaus Ma <>
Subject Re: Validate lamba function in `Flags::add` for required parameters
Date Thu, 14 Jul 2016 05:39:33 GMT
Agree with you clean up `FlagBase::add` :).

alexR/bbannier/neil is also working on enhancement to the `FlagBase` (MESOS-3335), I think
we can work together to make `FlagBase::add` more clear.


On Jul 14, 2016, at 02:08, Greg Mann <<>>

Thanks for bringing this up, Klaus - in this case, I think that extra
argument appears simply to match the desired function overload. Over time,
the overloads for `FlagsBase::add` have multiplied considerably; It looks
like we have about 20 now :-) I think it would be really nice to clean
these up somehow. I didn't see a JIRA issue for this improvement so I
created one: MESOS-5841 <>

One option would be to get rid of all the overloads except for
`FlagsBase::add(const Flag& flag)`, add a couple helper functions for
modifying `Flag` objects, and construct flag objects in the 'flags.cpp'

 Flag flag; = "work_dir"; = help_string;
 flag.set_storage(&Flags::work_dir); // New helper
 flag.set_validation(lambda_function); // New helper

I think this would make the 'flags.cpp' files more readable, and it would
clean up `FlagsBase` by getting rid of all those overloads.


On Tue, Jul 12, 2016 at 11:14 PM, Klaus Ma <<>>

Hi team,

When I updating the patch for MESOS-5123<>, I found the validate
lamba function for in `Flags::add` for required parameters is different
with optional parameters. Does any know why? The coding style is
inconsistent, it took times to find the suitable function  :).

Flags::add for optional parameters:




     "JSON object representing the environment variables that should be\n"

     "passed to the executor, and thus subsequently task(s). By default

     "flag is none. Users have to define executor environment



     "  \"PATH\": \"/bin:/usr/bin\",\n"

     "  \"LD_LIBRARY_PATH\": \"/usr/local/lib\"\n"


     [](const Option<JSON::Object>& object) -> Option<Error> {

       if (object.isSome()) {

         foreachvalue (const JSON::Value& value, object.get().values) {

           if (!<><JSON::String>()) {

             return Error("`executor_environment_variables` must "

                          "only contain string values");




       return None();



Flags::add for required parameters:




     None(),   // <============= Additional parameters to Flags::add

     "Absolute directory path of the agent work directory. This is

     "executor sandboxes will be placed, as well as the agent's

     "state in case of failover. Note that locations like `/tmp` which

     "cleaned automatically are not suitable for the work directory

     "running in production, since long-running agents could lose data

     "cleanup occurs; if launching docker tasks, the path must not

     "any disallowed symbols for docker volumes.\n"

     "(Example: `/var/lib/mesos/agent`)",

     static_cast<const string*>(0),

     [](const string& workDir) -> Option<Error> {

       if (!strings::startsWith(workDir, "/")) {

         return Error(

             "The required option `--work_dir` must be absolute path.");


       return None();




Da (Klaus), Ma (??), PMPĀ®| Software Architect
Platform DCOS Development & Support, STG, IBM GCG
+86-10-8245 4084 |<> |


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