mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Klaus Ma <klaus1982...@hotmail.com>
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.

Thanks
Klaus

On Jul 14, 2016, at 02:08, Greg Mann <greg@mesosphere.io<mailto:greg@mesosphere.io>>
wrote:

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 <https://issues.apache.org/jira/browse/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'
files:

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

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

Cheers,
Greg


On Tue, Jul 12, 2016 at 11:14 PM, Klaus Ma <klaus1982.cn@gmail.com<mailto:klaus1982.cn@gmail.com>>
wrote:

Hi team,


When I updating the patch for MESOS-5123<
https://issues.apache.org/jira/browse/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:

```

 add(&Flags::executor_environment_variables,

     "executor_environment_variables",

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

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

     "flag is none. Users have to define executor environment
explicitly.\n"

     "Example:\n"

     "{\n"

     "  \"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 (!value.is<http://value.is><JSON::String>()) {

             return Error("`executor_environment_variables` must "

                          "only contain string values");

           }

         }

       }

       return None();

     });

```


Flags::add for required parameters:


```

 add(&Flags::work_dir,

     "work_dir",

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

     "Absolute directory path of the agent work directory. This is
where\n"

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

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

     "cleaned automatically are not suitable for the work directory
when\n"

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

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

     "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 | madaxa@cn.ibm.com<mailto:madaxa@cn.ibm.com> | http://k82.me

<http://k82.me/>


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