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 <[email protected]<mailto:[email protected]>> 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 <[email protected]<mailto:[email protected]>> 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 | [email protected]<mailto:[email protected]> | http://k82.me <http://k82.me/>
