Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Gastón Kleiman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52556/#review152666
---



Superseded by the chain starting with https://reviews.apache.org/r/52878/

- Gastón Kleiman


On Oct. 11, 2016, 6:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 11, 2016, 6:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/default_executor.cpp 2454bd7df608254af72af01460782f5ab78a19c1 
>   src/launcher/executor.cpp 3e95d6029bea0ce6e0dfb39c24b795fe98d90d13 
>   src/slave/slave.cpp 119fb36c27b25739f2a86a55d48e964ca4a84ff7 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Jie Yu]

> On Oct. 10, 2016, 3:32 p.m., Jiang Yan Xu wrote:
> > src/launcher/executor.cpp, lines 903-910
> > 
> >
> > You didn't start this but I wonder why we can't consistently use the 
> > flags, which can be provided through environment variables, i.e., use 
> > `flags.load("MESOS_", , );` above.
> > 
> > That question aside, I don't think we need to be concerned about 
> > backwards-compatibility here: this is the command executor bundled with the 
> > agent so they are upgraded together. i.e., we are sure that the agent is 
> > setting the new variable.
> 
> Gastón Kleiman wrote:
> Wouldn't then the tasks started between the moment in which the binary is 
> overwritten and the moment in which the Mesos Agent is restarted fail?
> 
> Jiang Yan Xu wrote:
> OK I guess it'll be helpful in this regard. My main point is that this is 
> not a deprecation cycle concern.
> 
> Generally it's advisable to restart the agent right after upgrading it 
> but even though it's done this way with most Mesos toolings I have seen, I've 
> also seen tasks failing when launched mid-upgrade with libmesos and 
> executable mismatch. 
> 
> Therefore, fine with me to do this to aid N -> N+1 upgrade.
> 
> If we do this, the cleaniest approach may be flag aliases.
> 
> Alexander Rukletsov wrote:
> Technically, people may overwrite the mesos-executor binary.
> 
> Jiang Yan Xu wrote:
> As in, a custom executor but called via the agent <-> CommandExecutor 
> "protocol"? Is this an argument that the removal of `--launch_dir` flag 
> should go through a deprecation cycle? Could you elaborate?
> 
> Alexander Rukletsov wrote:
> Yes, this is what I meant.
> 
> Another reason is that we probably should maintain upgradability among 
> all 1.x version, i.e. N -> N + m upgrades, at least this is how I read the 
> versioning doc. With this in mind, we can't simply remove the flag.
> 
> Gastón Kleiman wrote:
> I'm dropping this issue, since it seems like we agree that keeping 
> backwards-compatibility is useful for N -> N+1 upgrades.
> 
> If someone wants to shepherd me, I volunteer to make `executor.cpp` fully 
> move to use env variables instead of a mix of flags and env variables.

> Wouldn't then the tasks started between the moment in which the binary is 
> overwritten and the moment in which the Mesos Agent is restarted fail?

I don't understand why env variable can address this issue? If the evn is 
optional, then, it won't fail (same as the flag is optional). If the env in not 
optional, will the task still fail?


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52556/#review151918
---


On Oct. 11, 2016, 6:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 11, 2016, 6:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/default_executor.cpp 2454bd7df608254af72af01460782f5ab78a19c1 
>   src/launcher/executor.cpp 3e95d6029bea0ce6e0dfb39c24b795fe98d90d13 
>   src/slave/slave.cpp 119fb36c27b25739f2a86a55d48e964ca4a84ff7 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52556/#review152526
---




docs/executor-http-api.md (line 352)


I agree with YanX that we probably should use flags in command executor and 
default executor. The benefit is that environment variables can be picked up by 
flags load as well (so that we can avoid os::getenv calls in the executor).

Also, I think we should call it launcher_dir because runtime_dir has 
different meanings in Mesos (especially mesos containerizer, see the new agent 
flag runtime_dir).


- Jie Yu


On Oct. 11, 2016, 6:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 11, 2016, 6:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/default_executor.cpp 2454bd7df608254af72af01460782f5ab78a19c1 
>   src/launcher/executor.cpp 3e95d6029bea0ce6e0dfb39c24b795fe98d90d13 
>   src/slave/slave.cpp 119fb36c27b25739f2a86a55d48e964ca4a84ff7 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Gastón Kleiman]

> On Oct. 10, 2016, 3:32 p.m., Jiang Yan Xu wrote:
> > src/launcher/executor.cpp, lines 903-910
> > 
> >
> > You didn't start this but I wonder why we can't consistently use the 
> > flags, which can be provided through environment variables, i.e., use 
> > `flags.load("MESOS_", , );` above.
> > 
> > That question aside, I don't think we need to be concerned about 
> > backwards-compatibility here: this is the command executor bundled with the 
> > agent so they are upgraded together. i.e., we are sure that the agent is 
> > setting the new variable.
> 
> Gastón Kleiman wrote:
> Wouldn't then the tasks started between the moment in which the binary is 
> overwritten and the moment in which the Mesos Agent is restarted fail?
> 
> Jiang Yan Xu wrote:
> OK I guess it'll be helpful in this regard. My main point is that this is 
> not a deprecation cycle concern.
> 
> Generally it's advisable to restart the agent right after upgrading it 
> but even though it's done this way with most Mesos toolings I have seen, I've 
> also seen tasks failing when launched mid-upgrade with libmesos and 
> executable mismatch. 
> 
> Therefore, fine with me to do this to aid N -> N+1 upgrade.
> 
> If we do this, the cleaniest approach may be flag aliases.
> 
> Alexander Rukletsov wrote:
> Technically, people may overwrite the mesos-executor binary.
> 
> Jiang Yan Xu wrote:
> As in, a custom executor but called via the agent <-> CommandExecutor 
> "protocol"? Is this an argument that the removal of `--launch_dir` flag 
> should go through a deprecation cycle? Could you elaborate?
> 
> Alexander Rukletsov wrote:
> Yes, this is what I meant.
> 
> Another reason is that we probably should maintain upgradability among 
> all 1.x version, i.e. N -> N + m upgrades, at least this is how I read the 
> versioning doc. With this in mind, we can't simply remove the flag.

I'm dropping this issue, since it seems like we agree that keeping 
backwards-compatibility is useful for N -> N+1 upgrades.

If someone wants to shepherd me, I volunteer to make `executor.cpp` fully move 
to use env variables instead of a mix of flags and env variables.


- Gastón


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52556/#review151918
---


On Oct. 11, 2016, 6:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 11, 2016, 6:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/default_executor.cpp 2454bd7df608254af72af01460782f5ab78a19c1 
>   src/launcher/executor.cpp 3e95d6029bea0ce6e0dfb39c24b795fe98d90d13 
>   src/slave/slave.cpp 119fb36c27b25739f2a86a55d48e964ca4a84ff7 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Alexander Rukletsov]

> On Oct. 10, 2016, 3:32 p.m., Jiang Yan Xu wrote:
> > src/launcher/executor.cpp, lines 903-910
> > 
> >
> > You didn't start this but I wonder why we can't consistently use the 
> > flags, which can be provided through environment variables, i.e., use 
> > `flags.load("MESOS_", , );` above.
> > 
> > That question aside, I don't think we need to be concerned about 
> > backwards-compatibility here: this is the command executor bundled with the 
> > agent so they are upgraded together. i.e., we are sure that the agent is 
> > setting the new variable.
> 
> Gastón Kleiman wrote:
> Wouldn't then the tasks started between the moment in which the binary is 
> overwritten and the moment in which the Mesos Agent is restarted fail?
> 
> Jiang Yan Xu wrote:
> OK I guess it'll be helpful in this regard. My main point is that this is 
> not a deprecation cycle concern.
> 
> Generally it's advisable to restart the agent right after upgrading it 
> but even though it's done this way with most Mesos toolings I have seen, I've 
> also seen tasks failing when launched mid-upgrade with libmesos and 
> executable mismatch. 
> 
> Therefore, fine with me to do this to aid N -> N+1 upgrade.
> 
> If we do this, the cleaniest approach may be flag aliases.
> 
> Alexander Rukletsov wrote:
> Technically, people may overwrite the mesos-executor binary.
> 
> Jiang Yan Xu wrote:
> As in, a custom executor but called via the agent <-> CommandExecutor 
> "protocol"? Is this an argument that the removal of `--launch_dir` flag 
> should go through a deprecation cycle? Could you elaborate?

Yes, this is what I meant.

Another reason is that we probably should maintain upgradability among all 1.x 
version, i.e. N -> N + m upgrades, at least this is how I read the versioning 
doc. With this in mind, we can't simply remove the flag.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52556/#review151918
---


On Oct. 11, 2016, 6:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 11, 2016, 6:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/default_executor.cpp 2454bd7df608254af72af01460782f5ab78a19c1 
>   src/launcher/executor.cpp 3e95d6029bea0ce6e0dfb39c24b795fe98d90d13 
>   src/slave/slave.cpp 119fb36c27b25739f2a86a55d48e964ca4a84ff7 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Gastón Kleiman]

> On Oct. 11, 2016, 4:22 p.m., Jie Yu wrote:
> > Can you explain why you want to replace `launcher_dir`? Looking at the 
> > attached ticket, seems like this is irrelevant?

I just updated the patch to also add support for the new env variable to the 
"default executor".

I'm adding this new env variable, because `launcher_dir` is passed as a flag, 
and the recently introduced "default executor" doesn't use flags at all.

The command executor uses a mix of flags and env variables. I would like to 
make it more consistent and make it use only env variables. @xujyan/@jieyu if I 
create a new patch to make the command executor use only env variables, would 
you be willing to shepherd it?


- Gastón


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52556/#review152145
---


On Oct. 11, 2016, 6:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 11, 2016, 6:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/default_executor.cpp 2454bd7df608254af72af01460782f5ab78a19c1 
>   src/launcher/executor.cpp 3e95d6029bea0ce6e0dfb39c24b795fe98d90d13 
>   src/slave/slave.cpp 119fb36c27b25739f2a86a55d48e964ca4a84ff7 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Gastón Kleiman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52556/
---

(Updated Oct. 11, 2016, 6:52 p.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
Jiang Yan Xu.


Changes
---

Addressed the feedback, including making the "default executor" read the env 
variable.


Bugs: MESOS-6288
https://issues.apache.org/jira/browse/MESOS-6288


Repository: mesos


Description
---

This environment variable points to the directory containing the
binaries used by the Mesos Agent. It will eventually replace the
`--launcher-dir` executor flag.


Diffs (updated)
-

  docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
  src/launcher/default_executor.cpp 2454bd7df608254af72af01460782f5ab78a19c1 
  src/launcher/executor.cpp 3e95d6029bea0ce6e0dfb39c24b795fe98d90d13 
  src/slave/slave.cpp 119fb36c27b25739f2a86a55d48e964ca4a84ff7 

Diff: https://reviews.apache.org/r/52556/diff/


Testing
---

`make check`


Thanks,

Gastón Kleiman

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Jiang Yan Xu]

> On Oct. 10, 2016, 8:32 a.m., Jiang Yan Xu wrote:
> > src/launcher/executor.cpp, lines 903-910
> > 
> >
> > You didn't start this but I wonder why we can't consistently use the 
> > flags, which can be provided through environment variables, i.e., use 
> > `flags.load("MESOS_", , );` above.
> > 
> > That question aside, I don't think we need to be concerned about 
> > backwards-compatibility here: this is the command executor bundled with the 
> > agent so they are upgraded together. i.e., we are sure that the agent is 
> > setting the new variable.
> 
> Gastón Kleiman wrote:
> Wouldn't then the tasks started between the moment in which the binary is 
> overwritten and the moment in which the Mesos Agent is restarted fail?
> 
> Jiang Yan Xu wrote:
> OK I guess it'll be helpful in this regard. My main point is that this is 
> not a deprecation cycle concern.
> 
> Generally it's advisable to restart the agent right after upgrading it 
> but even though it's done this way with most Mesos toolings I have seen, I've 
> also seen tasks failing when launched mid-upgrade with libmesos and 
> executable mismatch. 
> 
> Therefore, fine with me to do this to aid N -> N+1 upgrade.
> 
> If we do this, the cleaniest approach may be flag aliases.
> 
> Alexander Rukletsov wrote:
> Technically, people may overwrite the mesos-executor binary.

As in, a custom executor but called via the agent <-> CommandExecutor 
"protocol"? Is this an argument that the removal of `--launch_dir` flag should 
go through a deprecation cycle? Could you elaborate?


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52556/#review151918
---


On Oct. 10, 2016, 5:35 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 10, 2016, 5:35 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/executor.cpp 8a1051b886a0c95d19ff370e5c77d9c4033c8b61 
>   src/slave/slave.cpp d30001bc0d1798311ba3966f67aadc6be2c92306 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Alexander Rukletsov]

> On Oct. 10, 2016, 3:32 p.m., Jiang Yan Xu wrote:
> > src/launcher/executor.cpp, lines 903-910
> > 
> >
> > You didn't start this but I wonder why we can't consistently use the 
> > flags, which can be provided through environment variables, i.e., use 
> > `flags.load("MESOS_", , );` above.
> > 
> > That question aside, I don't think we need to be concerned about 
> > backwards-compatibility here: this is the command executor bundled with the 
> > agent so they are upgraded together. i.e., we are sure that the agent is 
> > setting the new variable.
> 
> Gastón Kleiman wrote:
> Wouldn't then the tasks started between the moment in which the binary is 
> overwritten and the moment in which the Mesos Agent is restarted fail?
> 
> Jiang Yan Xu wrote:
> OK I guess it'll be helpful in this regard. My main point is that this is 
> not a deprecation cycle concern.
> 
> Generally it's advisable to restart the agent right after upgrading it 
> but even though it's done this way with most Mesos toolings I have seen, I've 
> also seen tasks failing when launched mid-upgrade with libmesos and 
> executable mismatch. 
> 
> Therefore, fine with me to do this to aid N -> N+1 upgrade.
> 
> If we do this, the cleaniest approach may be flag aliases.

Technically, people may overwrite the mesos-executor binary.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52556/#review151918
---


On Oct. 10, 2016, 12:35 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 10, 2016, 12:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/executor.cpp 8a1051b886a0c95d19ff370e5c77d9c4033c8b61 
>   src/slave/slave.cpp d30001bc0d1798311ba3966f67aadc6be2c92306 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Jiang Yan Xu]

> On Oct. 10, 2016, 8:32 a.m., Jiang Yan Xu wrote:
> > src/launcher/executor.cpp, lines 903-910
> > 
> >
> > You didn't start this but I wonder why we can't consistently use the 
> > flags, which can be provided through environment variables, i.e., use 
> > `flags.load("MESOS_", , );` above.
> > 
> > That question aside, I don't think we need to be concerned about 
> > backwards-compatibility here: this is the command executor bundled with the 
> > agent so they are upgraded together. i.e., we are sure that the agent is 
> > setting the new variable.
> 
> Gastón Kleiman wrote:
> Wouldn't then the tasks started between the moment in which the binary is 
> overwritten and the moment in which the Mesos Agent is restarted fail?

OK I guess it'll be helpful in this regard. My main point is that this is not a 
deprecation cycle concern.

Generally it's advisable to restart the agent right after upgrading it but even 
though it's done this way with most Mesos toolings I have seen, I've also seen 
tasks failing when launched mid-upgrade with libmesos and executable mismatch. 

Therefore, fine with me to do this to aid N -> N+1 upgrade.

If we do this, the cleaniest approach may be flag aliases.


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52556/#review151918
---


On Oct. 10, 2016, 5:35 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 10, 2016, 5:35 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/executor.cpp 8a1051b886a0c95d19ff370e5c77d9c4033c8b61 
>   src/slave/slave.cpp d30001bc0d1798311ba3966f67aadc6be2c92306 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Gastón Kleiman]

> On Oct. 10, 2016, 3:32 p.m., Jiang Yan Xu wrote:
> > src/launcher/executor.cpp, lines 903-910
> > 
> >
> > You didn't start this but I wonder why we can't consistently use the 
> > flags, which can be provided through environment variables, i.e., use 
> > `flags.load("MESOS_", , );` above.
> > 
> > That question aside, I don't think we need to be concerned about 
> > backwards-compatibility here: this is the command executor bundled with the 
> > agent so they are upgraded together. i.e., we are sure that the agent is 
> > setting the new variable.

Wouldn't then the tasks started between the moment in which the binary is 
overwritten and the moment in which the Mesos Agent is restarted fail?


- Gastón


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52556/#review151918
---


On Oct. 10, 2016, 12:35 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 10, 2016, 12:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/executor.cpp 8a1051b886a0c95d19ff370e5c77d9c4033c8b61 
>   src/slave/slave.cpp d30001bc0d1798311ba3966f67aadc6be2c92306 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52556/#review151918
---




src/launcher/executor.cpp (lines 903 - 910)


You didn't start this but I wonder why we can't consistently use the flags, 
which can be provided through environment variables, i.e., use 
`flags.load("MESOS_", , );` above.

That question aside, I don't think we need to be concerned about 
backwards-compatibility here: this is the command executor bundled with the 
agent so they are upgraded together. i.e., we are sure that the agent is 
setting the new variable.



src/slave/slave.cpp (lines 4328 - 4331)


Same here. Since the logic here is specific for the bundled command 
executor, I don't think there's backwards-compatibility concerns here. We can 
just change it.



src/slave/slave.cpp (line 6847)


I asked about this on slack then started discussing it on the dev list. I'd 
say we don't comment on the deprecation until we reach conclusion on that 
thread.


- Jiang Yan Xu


On Oct. 10, 2016, 5:35 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 10, 2016, 5:35 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/executor.cpp 8a1051b886a0c95d19ff370e5c77d9c4033c8b61 
>   src/slave/slave.cpp d30001bc0d1798311ba3966f67aadc6be2c92306 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Alexander Rukletsov]

> On Oct. 6, 2016, 3:40 p.m., Alexander Rukletsov wrote:
> > Please add support for
> > * docker executor
> > * pod (default) executor
> 
> Gastón Kleiman wrote:
> The default executor has no use for this env variable yet. Should I add 
> it as an unused attribute in the `DefaultExecutor` class?

We will need it for MESOS-6119, hence it's fine to introduce an yet unused 
variable.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52556/#review151657
---


On Oct. 10, 2016, 12:35 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 10, 2016, 12:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/executor.cpp 8a1051b886a0c95d19ff370e5c77d9c4033c8b61 
>   src/slave/slave.cpp d30001bc0d1798311ba3966f67aadc6be2c92306 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Gastón Kleiman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52556/
---

(Updated Oct. 6, 2016, 4:28 p.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, and Jiang Yan Xu.


Changes
---

Addressed AlexR's comments.


Bugs: MESOS-6288
https://issues.apache.org/jira/browse/MESOS-6288


Repository: mesos


Description (updated)
---

This environment variable points to the directory containing the
binaries used by the Mesos Agent. It will eventually replace the
`--launcher-dir` executor flag.


Diffs (updated)
-

  docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
  src/launcher/executor.cpp 8a1051b886a0c95d19ff370e5c77d9c4033c8b61 
  src/slave/slave.cpp d30001bc0d1798311ba3966f67aadc6be2c92306 

Diff: https://reviews.apache.org/r/52556/diff/


Testing
---

`make check`


Thanks,

Gastón Kleiman