----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44655/#review123731 -----------------------------------------------------------
Looking good! Just some minor logical tweaks and some updates to the documentation. include/mesos/executor/executor.proto (lines 41 - 55) <https://reviews.apache.org/r/44655/#comment185983> Ditto the comments I made on the v1 changes below. include/mesos/v1/executor/executor.proto (lines 47 - 48) <https://reviews.apache.org/r/44655/#comment185981> The "or" here is going to lead to some confusion, which takes precendence? I'd suggest we just clarify that the environment variable is how they find it, and they can configure it with the ExecutorInfo field. Thoughts? include/mesos/v1/executor/executor.proto (lines 53 - 55) <https://reviews.apache.org/r/44655/#comment185982> Now that the period within the Shutdown message was removed, we should probably have a TODO following this note for adding the period into the Shutdown message so that the agent can communicate when a shorter period has been alloted. src/exec/exec.cpp (line 235) <https://reviews.apache.org/r/44655/#comment185980> Ditto the other comment, could we update this to reflect that it's not a default anymore? Perhaps the logic here should be to set if not found in environment. src/exec/exec.cpp (lines 713 - 714) <https://reviews.apache.org/r/44655/#comment185978> Per the other suggestions, can you update this to reflect that it's not a default? src/exec/exec.cpp (lines 715 - 716) <https://reviews.apache.org/r/44655/#comment185979> Can you confirm the environment variable setting didn't make it in 0.28.0? src/executor/executor.cpp (lines 235 - 236) <https://reviews.apache.org/r/44655/#comment185975> Not sure if you grabbed these comments from my reviews, but I had been thinking about the environment variable only specifying the default (i.e. DEFAULT_MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD rather than MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD). Since MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD now contains the expected value, we should just document that it's available in both places (so long as the agent is new enough to be setting the environment variable). It would bge helpful to mention that it's in the environment so that we know the shutdown time before we register with the agent. I suppose the reasoning here is that for those executors that do not set `ExecutorInfo.shutdown_grace_period`, they still want to know how much time they have. Also executors may, for some reason that escapes me, want to know how much time they have if they have to shutdown before they register. This latter use case seems a bit strange. Anyway, could you update these comments to reflect that the environment variable isn't a "default" anymore? src/executor/executor.cpp (line 703) <https://reviews.apache.org/r/44655/#comment185977> I'm thinking now that we should trust the value in the environment if it's set, rather than overwriting, since the agent could set a smaller one. This would only happen if, rather than rejecting too large of timeouts, we bounded them down implicitly. There's also the case of the agent deciding it has less time, but that case would likely come at the time of the SHUTDOWN Event. Perhaps this logic should be: set if not found in environment. And we should document that the agent will use the ExecutorInfo to set the environment value. src/slave/slave.cpp (lines 4214 - 4215) <https://reviews.apache.org/r/44655/#comment185973> Would you mind wrapping this a bit more cleanly? Like the one above for example: ``` // If the executor specifies shutdown grace period, // pass it instead of the default. ``` - Ben Mahler On March 15, 2016, 3:49 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44655/ > ----------------------------------------------------------- > > (Updated March 15, 2016, 3:49 p.m.) > > > Review request for mesos, Ben Mahler and Gilbert Song. > > > Bugs: MESOS-4949 > https://issues.apache.org/jira/browse/MESOS-4949 > > > Repository: mesos > > > Description > ------- > > If `ExecutorInfo.shutdown_grace_period` is set, the executor > driver uses it, otherwise it falls back to the environment > variable `MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD`. > > > Diffs > ----- > > docs/configuration.md 739d4ff9aeeb1ba70ce11033168d63d37b6ef56b > include/mesos/executor/executor.proto > ae211194a44e0bf2fadc79e833881e45ea3eb2c2 > include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b > include/mesos/v1/executor/executor.proto > 36a2b3f9bc3aaa524f655b9e686a6d33512e6aaa > include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b > src/exec/exec.cpp 741786132f3a8cc43f5b9ced262429038832a946 > src/executor/executor.cpp 87db4e02cbaa778aab0173741bfe066fdee9a48d > src/slave/flags.cpp 4d10818105627738e258116647ccada374e3d7b9 > src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 > > Diff: https://reviews.apache.org/r/44655/diff/ > > > Testing > ------- > > The complete chain was tested. See https://reviews.apache.org/r/44662/. > > > Thanks, > > Alexander Rukletsov > >
