You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Klaus Ma <kl...@gmail.com> on 2016/07/13 06:14:41 UTC
Validate lamba function in `Flags::add` for required parameters
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/>
Re: Validate lamba function in `Flags::add` for required parameters
Posted by Michael Park <mp...@apache.org>.
I'm shepherding https://issues.apache.org/jira/browse/MESOS-3335,
and I'm hoping to we don't run into too much conflicts here in terms of
rebasing and such.
Do you guys have anything concrete at this point and a shepherd who will
help you guys with this stuff?
If not, I'm up to shepherding it, but after MESOS-3335.
Thanks,
MPark
On 14 July 2016 at 07:39, Klaus Ma <kl...@hotmail.com> wrote:
> 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<ma...@cn.ibm.com> |
> http://k82.me
>
> <http://k82.me/>
>
>
Re: Validate lamba function in `Flags::add` for required parameters
Posted by Klaus Ma <kl...@hotmail.com>.
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 <gr...@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 <kl...@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<ma...@cn.ibm.com> | http://k82.me
<http://k82.me/>
Re: Validate lamba function in `Flags::add` for required parameters
Posted by Greg Mann <gr...@mesosphere.io>.
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 <kl...@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/>
>