mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Validate lamba function in `Flags::add` for required parameters
Date Wed, 13 Jul 2016 18:08:07 GMT
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> 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<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 | http://k82.me
>
> <http://k82.me/>
>

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