Re: Review Request 58907: Refactored Docker containerizer launch path per interface changes.

[Joseph Wu]

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

(Updated May 19, 2017, 3:15 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebase and un-delete a validation check.


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


Repository: mesos


Description
---

This continues the containerizer interface change that replaced
most "Infos" (Task, Executor, Command, Container) with a single
`ContainerConfig` and combined the nested/non-nested container
launch paths.

Notably, this commit also changes the fields stored in the Docker
containerizer to match the new interface.  The somewhat ambiguously
named `directory` field has been renamed to `containerWorkDir`.
And the copy of `Slave::flags` in each container has been removed
because it is not used.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 44efa44586455df416fc9cdc51c92bd9027f24c3 
  src/slave/containerizer/docker.cpp 2480f595bee032ea399d642653d618472db0b7a7 


Diff: https://reviews.apache.org/r/58907/diff/3/

Changes: https://reviews.apache.org/r/58907/diff/2-3/


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Re: Review Request 58905: Changed naming of Docker containers to the pre-0.23 scheme.

[Joseph Wu]

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

(Updated May 19, 2017, 3:14 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Change recovery path to save the name of the container for use in later calls.


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


Repository: mesos


Description
---

The choice of naming scheme is cosmetic.  All versions of the Docker
containerizer will consider containers starting with "mesos-"
as containers started by Mesos.  The Docker containerizer does not
consider the `SlaveID` at all when recovering.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 44efa44586455df416fc9cdc51c92bd9027f24c3 
  src/slave/containerizer/docker.cpp 2480f595bee032ea399d642653d618472db0b7a7 


Diff: https://reviews.apache.org/r/58905/diff/2/

Changes: https://reviews.apache.org/r/58905/diff/1-2/


Testing (updated)
---

See last patch in chain.

TODO: Add a backwards compatibility test .


Thanks,

Joseph Wu

Re: Review Request 58904: Combined launch paths in ComposingContainerizer.

[Joseph Wu]

> On May 11, 2017, 5:50 a.m., Jie Yu wrote:
> > src/slave/containerizer/composing.cpp
> > Lines 439 (patched)
> > 
> >
> > YOu can use 'auto' here.

Personally, I don't really like to use `auto` unless the type can be inferred 
from the line itself :)

Also, the original line didn't use `auto`, and I didn't have a strong reason to 
change it.


- Joseph


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


On May 1, 2017, 7:05 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58904/
> ---
> 
> (Updated May 1, 2017, 7:05 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7449
> https://issues.apache.org/jira/browse/MESOS-7449
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This eliminates one of the entrypoints for launching a container
> in the `ComposingContainerizer`.  Nested and non-nested containers
> are still launched the same way, where nested containers get launched
> using their root container's containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.hpp 
> 8e04bfeb76607e87ee336f4a7f519eac08b7d44c 
>   src/slave/containerizer/composing.cpp 
> 0b6c76b3d081d86df81a6062ae7a191ba8dadfde 
> 
> 
> Diff: https://reviews.apache.org/r/58904/diff/2/
> 
> 
> Testing
> ---
> 
> See last patch in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 58903: Combined Mesos containerizer's launch methods.

[Joseph Wu]

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

(Updated May 19, 2017, 3:10 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

This simplifies the container launch path by removing combining
the nested and non-nested container code paths into one.

The Mesos containerizer was originally translating the two
`containerizer->launch` entrypoints into a common method (also
called `launch`).  The previous commits moved this translation
logic into the caller (i.e. the Agent).

The end result has some slight changes:
  * It is now possible for the Agent to specify some more combinations
of `ContainerConfig`.  For example, specifying a TaskInfo with
a DEBUG-class container.  Or a nested container with Resources.
We may need to add extra validation around this.
  * The `bool checkpoint` argument was replaced with a `Option`
which optionally contains an absolute path.  This allows us to
remove the `SlaveID` field.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
04ab997454534b8e5e821b53b83e166e5018e11c 
  src/slave/containerizer/mesos/containerizer.cpp 
50a63b58b4960729316b0b5685793ce18ee5ce93 


Diff: https://reviews.apache.org/r/58903/diff/2/

Changes: https://reviews.apache.org/r/58903/diff/1-2/


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Review Request 59420: Added some comments to ContainerConfig protobuf.

[Joseph Wu]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

These comments clarify some of the expectations of each field
of the `ContainerConfig` protobuf, especially where fields
are duplicated based on other fields.


Diffs
-

  include/mesos/slave/containerizer.proto 
0f96334c236027780db1d88807e09685ccda4562 


Diff: https://reviews.apache.org/r/59420/diff/1/


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Review Request 59419: Updated mesos-local work directory structure.

[Joseph Wu]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

This combines the `--work_dir` and `--runtime_dir` flags for the
mesos-local helper (**not** the Agent flags, which have the same name).
Only the `--work_dir` will remain.

The new directory structure will be the following:

  work_dir (mesos-local flag)
  |-- agents
  |   |-- 0
  |   |   |-- fetch (Agent --fetcher_cache_dir flag)
  |   |   |-- run   (Agent --runtime_dir flag)
  |   |   |-- work  (Agent --work_dir flag)
  |   |-- 1
  |   |   ...
  |-- master


Diffs
-

  src/local/flags.hpp 0cef5ac730d19b00f78869e987993a71e464ca05 
  src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3 


Diff: https://reviews.apache.org/r/59419/diff/1/


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Re: Review Request 58900: Changed the default fetcher cache directory.

[Joseph Wu]

> On May 11, 2017, 1:53 a.m., Jie Yu wrote:
> > src/local/local.cpp
> > Lines 372-374 (patched)
> > 
> >
> > I'd suggest we put fetcher dir under work_dir as well for local mode so 
> > that all directories related to this local run have a common parent 
> > directory.
> > 
> > ```
> > propagatedFlags["work_dir"] =
> >   path::join(flags.work_dir, "agents", stringify(i), "work_dir");
> >   
> > propagatedFlags["runtime_dir"] =
> >   path::join(flags.work_dir, "agents", stringify(i), "runtime_dir");
> >   
> > propagatedFlags["fetcher_cache_dir"] =
> >   path::join(flags.work_dir, "agents", stringify(i), 
> > "fetcher_cache_dir");
> > ```
> > 
> > No need for the `runtime_dir` in local flags.

This is logically a separate change (with some flag documentation updates), so 
I'll open a separate review with this change.


- Joseph


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


On May 19, 2017, 3:05 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58900/
> ---
> 
> (Updated May 19, 2017, 3:05 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7304
> https://issues.apache.org/jira/browse/MESOS-7304
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher cache directory was historically located (by default)
> in `/tmp/mesos/fetch`.  The agent flag `--fetcher_cache_dir` could
> be used to change this value.
> 
> The fetcher would create a subdirectory underneath `/tmp/mesos/fetch`
> for each `SlaveID`.  This was done because multiple agents can run on
> the same node.  If all the agents use the same default fetcher cache
> directory, they will collide and cause unpredictable results.
> As a result, the `SlaveID` needed to be passed into the fetcher
> after the agent recovers and/or registers with the master, because
> that is when the `SlaveID` is determined.
> 
> This changes the default fetcher cache directory to
> `/tmp/mesos/fetch`.  The `SlaveID` subdirectory has been removed.
> 
> This change, while techically a breaking change, is safe because of
> how the fetcher uses this directory.  Upon starting up, the fetcher
> "recovers" by clearing this directory.  Although the subdirectory
> has been removed, the fetcher still clears the fetcher cache
> on startup.
> 
> This change will only cause breakages if multiple agents are run
> with the same `--fetcher_cache_dir`.  In this case, each agent
> will delete the fetcher caches of all the other agents.
> 
> ---
> 
> With the removal of the `SlaveID` field in the fetcher's methods,
> it is no longer necessary to pass in the `SlaveID` or agent Flags
> at agent recovery time.  Instead, the flags can be passed in during
> the fetcher's construction.
> 
> Similarly, the fetcher's "recovery" (clearing the fetcher cache)
> can be done immediately upon construction, which simplifies the
> code slightly.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3 
>   src/slave/containerizer/fetcher.hpp 
> 9e3018dc087ed55c61b2824d0105bc5339b83043 
>   src/slave/containerizer/fetcher.cpp 
> a910fea5a5556afb376524c5bb2ff98d7d84e611 
>   src/slave/flags.cpp e172aa526cfd38f4f30efda22ef011146e5c1a7d 
>   src/slave/main.cpp 507d59996a90f51c7cd1e0f836a34c2e9c484791 
>   src/slave/slave.cpp 14de72fa4a8746504a251d735bf56041b934df40 
> 
> 
> Diff: https://reviews.apache.org/r/58900/diff/6/
> 
> 
> Testing
> ---
> 
> See last patch in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 58900: Changed the default fetcher cache directory.

[Joseph Wu]

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

(Updated May 19, 2017, 3:05 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Removed some refactoring artifacts.


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


Repository: mesos


Description
---

The fetcher cache directory was historically located (by default)
in `/tmp/mesos/fetch`.  The agent flag `--fetcher_cache_dir` could
be used to change this value.

The fetcher would create a subdirectory underneath `/tmp/mesos/fetch`
for each `SlaveID`.  This was done because multiple agents can run on
the same node.  If all the agents use the same default fetcher cache
directory, they will collide and cause unpredictable results.
As a result, the `SlaveID` needed to be passed into the fetcher
after the agent recovers and/or registers with the master, because
that is when the `SlaveID` is determined.

This changes the default fetcher cache directory to
`/tmp/mesos/fetch`.  The `SlaveID` subdirectory has been removed.

This change, while techically a breaking change, is safe because of
how the fetcher uses this directory.  Upon starting up, the fetcher
"recovers" by clearing this directory.  Although the subdirectory
has been removed, the fetcher still clears the fetcher cache
on startup.

This change will only cause breakages if multiple agents are run
with the same `--fetcher_cache_dir`.  In this case, each agent
will delete the fetcher caches of all the other agents.

---

With the removal of the `SlaveID` field in the fetcher's methods,
it is no longer necessary to pass in the `SlaveID` or agent Flags
at agent recovery time.  Instead, the flags can be passed in during
the fetcher's construction.

Similarly, the fetcher's "recovery" (clearing the fetcher cache)
can be done immediately upon construction, which simplifies the
code slightly.


Diffs (updated)
-

  src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3 
  src/slave/containerizer/fetcher.hpp 9e3018dc087ed55c61b2824d0105bc5339b83043 
  src/slave/containerizer/fetcher.cpp a910fea5a5556afb376524c5bb2ff98d7d84e611 
  src/slave/flags.cpp e172aa526cfd38f4f30efda22ef011146e5c1a7d 
  src/slave/main.cpp 507d59996a90f51c7cd1e0f836a34c2e9c484791 
  src/slave/slave.cpp 14de72fa4a8746504a251d735bf56041b934df40 


Diff: https://reviews.apache.org/r/58900/diff/6/

Changes: https://reviews.apache.org/r/58900/diff/5-6/


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Re: Review Request 59375: Added REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason.

[Andrei Budnik]

> On May 19, 2017, 10:04 a.m., Alexander Rukletsov wrote:
> > src/launcher/default_executor.cpp
> > Lines 795-797 (original), 795-799 (patched)
> > 
> >
> > Why do you think the reason here should be updated task health? This 
> > update is sent for the finished task, so the reason is more "task 
> > finished". We do include health information here, but I'm not sure we 
> > should modify the reason.

After the discussion with AlexR, we've decided to consider specifying 
appropriate status update reason, meaning that the task was killed due to a 
failing health check. Something like 
TaskStatus::REASON_TASK_KILLED_AFTER_FAILING_HEALTH_CHECK.


- Andrei


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


On May 19, 2017, 9:07 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59375/
> ---
> 
> (Updated May 19, 2017, 9:07 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-6905
> https://issues.apache.org/jira/browse/MESOS-6905
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This gives clear indication about the reason of task status updates
> caused by task health update.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e138e2aedb1369acdde82facf9dfea9780411154 
>   include/mesos/v1/mesos.proto ab617bfd2bc46e44f7d71cefecf5eac3630d22b3 
>   src/docker/executor.cpp 82ae9bdc94565acb746724c2e6ab6432aa6bd751 
>   src/launcher/default_executor.cpp 5c31f948b5ba470e7c007c39e1c0c903451477e2 
>   src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
>   src/tests/check_tests.cpp 3607a690c963a84acfb2d3adc6c4a4f148b456d6 
>   src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad 
> 
> 
> Diff: https://reviews.apache.org/r/59375/diff/3/
> 
> 
> Testing
> ---
> 
> Added checks for task status updates caused by task health update.
> 
> make check (fedora 25, mac os 10.12)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 58048: Added id to Resource.DiskInfo.

[Benjamin Bannier]

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

(Updated May 19, 2017, 11:16 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Addressed issue raised by Qian.


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


Repository: mesos


Description
---

Ids will allow to create distinguishable resources, e.g., of RAW or
BLOCK type.


Diffs (updated)
-

  include/mesos/mesos.proto e138e2aedb1369acdde82facf9dfea9780411154 
  include/mesos/v1/mesos.proto ab617bfd2bc46e44f7d71cefecf5eac3630d22b3 
  src/common/resources.cpp 1d07f1a049ba3bb3f5d78f05031f33ba97e07e8c 
  src/tests/mesos.hpp 3c57f25ff0bec55582b8a7decef53be61a9ec6f4 
  src/tests/resources_tests.cpp 5dcbce2dd4944194b59790551929d6d75b805ba3 
  src/v1/resources.cpp 1bb5d0741c9f9ea59f34e92159a335661019444d 


Diff: https://reviews.apache.org/r/58048/diff/4/

Changes: https://reviews.apache.org/r/58048/diff/3-4/


Testing
---

make check (OS X, Fedora25)


Thanks,

Benjamin Bannier

Re: Review Request 58048: Added id to Resource.DiskInfo.

[Benjamin Bannier]

> On May 17, 2017, 5:12 p.m., Qian Zhang wrote:
> > src/tests/resources_tests.cpp
> > Lines 2190 (patched)
> > 
> >
> > Unused variable?

Thanks, also fixed the other sites.


- Benjamin


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


On May 19, 2017, 11:16 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58048/
> ---
> 
> (Updated May 19, 2017, 11:16 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7312
> https://issues.apache.org/jira/browse/MESOS-7312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ids will allow to create distinguishable resources, e.g., of RAW or
> BLOCK type.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e138e2aedb1369acdde82facf9dfea9780411154 
>   include/mesos/v1/mesos.proto ab617bfd2bc46e44f7d71cefecf5eac3630d22b3 
>   src/common/resources.cpp 1d07f1a049ba3bb3f5d78f05031f33ba97e07e8c 
>   src/tests/mesos.hpp 3c57f25ff0bec55582b8a7decef53be61a9ec6f4 
>   src/tests/resources_tests.cpp 5dcbce2dd4944194b59790551929d6d75b805ba3 
>   src/v1/resources.cpp 1bb5d0741c9f9ea59f34e92159a335661019444d 
> 
> 
> Diff: https://reviews.apache.org/r/58048/diff/4/
> 
> 
> Testing
> ---
> 
> make check (OS X, Fedora25)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 58048: Added id to Resource.DiskInfo.

[Benjamin Bannier]

> On April 17, 2017, 11:49 a.m., Jie Yu wrote:
> > src/tests/resources_tests.cpp
> > Lines 2255 (patched)
> > 
> >
> > instead of relying on 'count', let's use `size()` instead.
> 
> Qian Zhang wrote:
> I think here the intention is to count how many `disks` are in `(r1 + 
> r2)`, so `count()` should be the right method to call.

Yes, exactly. Note that we could use `size` and explicit value checks here, but 
this would introduce dependencies to the exact disk values which in my opinion 
makes the code harder to understand.


- Benjamin


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


On May 19, 2017, 11:16 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58048/
> ---
> 
> (Updated May 19, 2017, 11:16 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7312
> https://issues.apache.org/jira/browse/MESOS-7312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ids will allow to create distinguishable resources, e.g., of RAW or
> BLOCK type.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e138e2aedb1369acdde82facf9dfea9780411154 
>   include/mesos/v1/mesos.proto ab617bfd2bc46e44f7d71cefecf5eac3630d22b3 
>   src/common/resources.cpp 1d07f1a049ba3bb3f5d78f05031f33ba97e07e8c 
>   src/tests/mesos.hpp 3c57f25ff0bec55582b8a7decef53be61a9ec6f4 
>   src/tests/resources_tests.cpp 5dcbce2dd4944194b59790551929d6d75b805ba3 
>   src/v1/resources.cpp 1bb5d0741c9f9ea59f34e92159a335661019444d 
> 
> 
> Diff: https://reviews.apache.org/r/58048/diff/4/
> 
> 
> Testing
> ---
> 
> make check (OS X, Fedora25)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 59375: Added REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason.

[Andrei Budnik]

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

(Updated May 19, 2017, 9:07 p.m.)


Review request for mesos and Alexander Rukletsov.


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


Repository: mesos


Description
---

This gives clear indication about the reason of task status updates
caused by task health update.


Diffs (updated)
-

  include/mesos/mesos.proto e138e2aedb1369acdde82facf9dfea9780411154 
  include/mesos/v1/mesos.proto ab617bfd2bc46e44f7d71cefecf5eac3630d22b3 
  src/docker/executor.cpp 82ae9bdc94565acb746724c2e6ab6432aa6bd751 
  src/launcher/default_executor.cpp 5c31f948b5ba470e7c007c39e1c0c903451477e2 
  src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
  src/tests/check_tests.cpp 3607a690c963a84acfb2d3adc6c4a4f148b456d6 
  src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad 


Diff: https://reviews.apache.org/r/59375/diff/3/

Changes: https://reviews.apache.org/r/59375/diff/2-3/


Testing
---

Added checks for task status updates caused by task health update.

make check (fedora 25, mac os 10.12)


Thanks,

Andrei Budnik

Re: Review Request 59413: Fix constexpr compilation failure with GCC 7.1.

[Aaron Wood via Review Board]

> On May 19, 2017, 6:51 p.m., Neil Conway wrote:
> > `stout`, `libprocess`, and `mesos` are nominally distinct projects, so a 
> > single RR should touch no more than one of them. i.e., please split the 
> > `stout` changes into a separate review.
> > 
> > Might also be worth waiting to see if GCC upstream is going to fix the 
> > problem promptly. If GCC 7.2 fixes the problem, we don't necessarily need 
> > to workaround a bug that occurs in just a single minor release of GCC.
> 
> Aaron Wood wrote:
> Sure, I can hold on this and see what they say. If it's something that 
> still needs to be fixed I'll break out this RR into separate ones.
> 
> Benjamin Bannier wrote:
> I am not a big fan of the derived classes of `Bytes` since they are 
> practically designed to be used with slicing (`Bytes b = Kilobytes(42)`) 
> which is nasty by itself. I believe the intention here was to provide easy to 
> use factory functions, and don't believe introducing dedicated types for 
> multiples is a very useful but instead confusing (think e.g., number of 
> implicit conversions and overload resolution). This fix makes sense 
> regardless of whether GCC fixes this particular regression.

I think it is much cleaner as well. If everyone is in agreement I can move 
forward with this patch regardless of the GCC issue.


> On May 19, 2017, 6:51 p.m., Neil Conway wrote:
> > src/master/constants.hpp
> > Line 49 (original), 49 (patched)
> > 
> >
> > This change (and all the similar changes) seems unfortunate. Can't we 
> > play a similar trick to the change you made to `Bytes`?
> 
> Aaron Wood wrote:
> I had wanted to but I didn't see a quick and easy way to do this without 
> making some major changes. The extended classes in `duration.hpp` mostly look 
> something like this:
> ```
> class Nanoseconds : public Duration
> {
> public:
>   explicit constexpr Nanoseconds(int64_t nanoseconds)
> : Duration(nanoseconds, NANOSECONDS) {}
> 
>   constexpr Nanoseconds(const Duration& d) : Duration(d) {}
> 
>   double value() const { return static_cast(this->ns()); }
> 
>   static std::string units() { return "ns"; }
> };
> ```
> I'm open to ideas, I just figured if I changed these clases a lot I'd 
> have even more code to change around Mesos.
> 
> Benjamin Bannier wrote:
> It looks like the only custom method here is `units` which seems to be 
> used by just the lengthy stringification function below. I believe getting 
> rid of the derived classes here makes just as much sense as it did for 
> `Bytes` and friends.

That's good to know, I didn't dig too deeply to see what was using some of this 
stuff. I can rework this file a bit so that we could keep the `Duration` base 
type everywhere like it was.


- Aaron


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


On May 19, 2017, 7 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59413/
> ---
> 
> (Updated May 19, 2017, 7 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James Peach, Michael Park, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-7520
> https://issues.apache.org/jira/browse/MESOS-7520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Many of the `constexpr` variables fail to compile with errors such as `error: 
> 'Megabytes(32).Megabytes::' is not a constant expression because 
> it refers to an incompletely initialized variable`. 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80829
> 
> This patch changes around the `Bytes` class a bit by getting rid of the small 
> classes that extend `Bytes`. Additionally, any usage of the `Duration` class 
> has been adjusted to not instantiate using the base type (which triggers the 
> issue along with `constexpr`).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/bytes.hpp 98223db50 
>   src/master/constants.hpp 725680b1e 
>   src/sched/constants.hpp 9edb25b38 
>   src/sched/sched.cpp ef73c1dcc 
>   src/scheduler/constants.hpp e3da12646 
>   src/slave/constants.hpp 9c1d7245c 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 929b7e0b7 
>   src/slave/slave.cpp 14de72fa4 
>   src/slave/status_update_manager.cpp 0cd88ac3a 
> 
> 
> Diff: https://reviews.apache.org/r/59413/diff/3/
> 
> 
> Testing
> ---
> 
> `./bootstrap && mkdir build && cd build && ../configure --disable-python 
> --disable-java --disable-optimize --disable-hardening &&  make -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 59413: Fix constexpr compilation failure with GCC 7.1.

[Benjamin Bannier]

> On May 19, 2017, 8:51 p.m., Neil Conway wrote:
> > `stout`, `libprocess`, and `mesos` are nominally distinct projects, so a 
> > single RR should touch no more than one of them. i.e., please split the 
> > `stout` changes into a separate review.
> > 
> > Might also be worth waiting to see if GCC upstream is going to fix the 
> > problem promptly. If GCC 7.2 fixes the problem, we don't necessarily need 
> > to workaround a bug that occurs in just a single minor release of GCC.
> 
> Aaron Wood wrote:
> Sure, I can hold on this and see what they say. If it's something that 
> still needs to be fixed I'll break out this RR into separate ones.

I am not a big fan of the derived classes of `Bytes` since they are practically 
designed to be used with slicing (`Bytes b = Kilobytes(42)`) which is nasty by 
itself. I believe the intention here was to provide easy to use factory 
functions, and don't believe introducing dedicated types for multiples is a 
very useful but instead confusing (think e.g., number of implicit conversions 
and overload resolution). This fix makes sense regardless of whether GCC fixes 
this particular regression.


> On May 19, 2017, 8:51 p.m., Neil Conway wrote:
> > src/master/constants.hpp
> > Line 49 (original), 49 (patched)
> > 
> >
> > This change (and all the similar changes) seems unfortunate. Can't we 
> > play a similar trick to the change you made to `Bytes`?
> 
> Aaron Wood wrote:
> I had wanted to but I didn't see a quick and easy way to do this without 
> making some major changes. The extended classes in `duration.hpp` mostly look 
> something like this:
> ```
> class Nanoseconds : public Duration
> {
> public:
>   explicit constexpr Nanoseconds(int64_t nanoseconds)
> : Duration(nanoseconds, NANOSECONDS) {}
> 
>   constexpr Nanoseconds(const Duration& d) : Duration(d) {}
> 
>   double value() const { return static_cast(this->ns()); }
> 
>   static std::string units() { return "ns"; }
> };
> ```
> I'm open to ideas, I just figured if I changed these clases a lot I'd 
> have even more code to change around Mesos.

It looks like the only custom method here is `units` which seems to be used by 
just the lengthy stringification function below. I believe getting rid of the 
derived classes here makes just as much sense as it did for `Bytes` and friends.


- Benjamin


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


On May 19, 2017, 9 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59413/
> ---
> 
> (Updated May 19, 2017, 9 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James Peach, Michael Park, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-7520
> https://issues.apache.org/jira/browse/MESOS-7520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Many of the `constexpr` variables fail to compile with errors such as `error: 
> 'Megabytes(32).Megabytes::' is not a constant expression because 
> it refers to an incompletely initialized variable`. 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80829
> 
> This patch changes around the `Bytes` class a bit by getting rid of the small 
> classes that extend `Bytes`. Additionally, any usage of the `Duration` class 
> has been adjusted to not instantiate using the base type (which triggers the 
> issue along with `constexpr`).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/bytes.hpp 98223db50 
>   src/master/constants.hpp 725680b1e 
>   src/sched/constants.hpp 9edb25b38 
>   src/sched/sched.cpp ef73c1dcc 
>   src/scheduler/constants.hpp e3da12646 
>   src/slave/constants.hpp 9c1d7245c 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 929b7e0b7 
>   src/slave/slave.cpp 14de72fa4 
>   src/slave/status_update_manager.cpp 0cd88ac3a 
> 
> 
> Diff: https://reviews.apache.org/r/59413/diff/3/
> 
> 
> Testing
> ---
> 
> `./bootstrap && mkdir build && cd build && ../configure --disable-python 
> --disable-java --disable-optimize --disable-hardening &&  make -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 59413: Fix constexpr compilation failure with GCC 7.1.

[Aaron Wood via Review Board]

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

(Updated May 19, 2017, 7 p.m.)


Review request for mesos, Benjamin Bannier, James Peach, Michael Park, and Neil 
Conway.


Changes
---

Reference filed GCC bug.


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


Repository: mesos


Description (updated)
---

Many of the `constexpr` variables fail to compile with errors such as `error: 
'Megabytes(32).Megabytes::' is not a constant expression because it 
refers to an incompletely initialized variable`. 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80829

This patch changes around the `Bytes` class a bit by getting rid of the small 
classes that extend `Bytes`. Additionally, any usage of the `Duration` class 
has been adjusted to not instantiate using the base type (which triggers the 
issue along with `constexpr`).


Diffs
-

  3rdparty/stout/include/stout/bytes.hpp 98223db50 
  src/master/constants.hpp 725680b1e 
  src/sched/constants.hpp 9edb25b38 
  src/sched/sched.cpp ef73c1dcc 
  src/scheduler/constants.hpp e3da12646 
  src/slave/constants.hpp 9c1d7245c 
  src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 929b7e0b7 
  src/slave/slave.cpp 14de72fa4 
  src/slave/status_update_manager.cpp 0cd88ac3a 


Diff: https://reviews.apache.org/r/59413/diff/3/


Testing
---

`./bootstrap && mkdir build && cd build && ../configure --disable-python 
--disable-java --disable-optimize --disable-hardening &&  make -j$(nproc)`


Thanks,

Aaron Wood

Re: Review Request 59413: Fix constexpr compilation failure with GCC 7.1.

[Aaron Wood via Review Board]

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

(Updated May 19, 2017, 6:58 p.m.)


Review request for mesos, Benjamin Bannier, James Peach, Michael Park, and Neil 
Conway.


Changes
---

Clean base for patch to be applied.


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


Repository: mesos


Description
---

Many of the `constexpr` variables fail to compile with errors such as `error: 
'Megabytes(32).Megabytes::' is not a constant expression because it 
refers to an incompletely initialized variable`.

This patch changes around the `Bytes` class a bit by getting rid of the small 
classes that extend `Bytes`. Additionally, any usage of the `Duration` class 
has been adjusted to not instantiate using the base type (which triggers the 
issue along with `constexpr`).


Diffs (updated)
-

  3rdparty/stout/include/stout/bytes.hpp 98223db50 
  src/master/constants.hpp 725680b1e 
  src/sched/constants.hpp 9edb25b38 
  src/sched/sched.cpp ef73c1dcc 
  src/scheduler/constants.hpp e3da12646 
  src/slave/constants.hpp 9c1d7245c 
  src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 929b7e0b7 
  src/slave/slave.cpp 14de72fa4 
  src/slave/status_update_manager.cpp 0cd88ac3a 


Diff: https://reviews.apache.org/r/59413/diff/3/

Changes: https://reviews.apache.org/r/59413/diff/2-3/


Testing
---

`./bootstrap && mkdir build && cd build && ../configure --disable-python 
--disable-java --disable-optimize --disable-hardening &&  make -j$(nproc)`


Thanks,

Aaron Wood

Re: Review Request 59413: Fix constexpr compilation failure with GCC 7.1.

[Aaron Wood via Review Board]

> On May 19, 2017, 6:51 p.m., Neil Conway wrote:
> > `stout`, `libprocess`, and `mesos` are nominally distinct projects, so a 
> > single RR should touch no more than one of them. i.e., please split the 
> > `stout` changes into a separate review.
> > 
> > Might also be worth waiting to see if GCC upstream is going to fix the 
> > problem promptly. If GCC 7.2 fixes the problem, we don't necessarily need 
> > to workaround a bug that occurs in just a single minor release of GCC.

Sure, I can hold on this and see what they say. If it's something that still 
needs to be fixed I'll break out this RR into separate ones.


> On May 19, 2017, 6:51 p.m., Neil Conway wrote:
> > src/master/constants.hpp
> > Line 49 (original), 49 (patched)
> > 
> >
> > This change (and all the similar changes) seems unfortunate. Can't we 
> > play a similar trick to the change you made to `Bytes`?

I had wanted to but I didn't see a quick and easy way to do this without making 
some major changes. The extended classes in `duration.hpp` mostly look 
something like this:
```
class Nanoseconds : public Duration
{
public:
  explicit constexpr Nanoseconds(int64_t nanoseconds)
: Duration(nanoseconds, NANOSECONDS) {}

  constexpr Nanoseconds(const Duration& d) : Duration(d) {}

  double value() const { return static_cast(this->ns()); }

  static std::string units() { return "ns"; }
};
```
I'm open to ideas, I just figured if I changed these clases a lot I'd have even 
more code to change around Mesos.


- Aaron


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


On May 19, 2017, 6:51 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59413/
> ---
> 
> (Updated May 19, 2017, 6:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James Peach, Michael Park, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-7520
> https://issues.apache.org/jira/browse/MESOS-7520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Many of the `constexpr` variables fail to compile with errors such as `error: 
> 'Megabytes(32).Megabytes::' is not a constant expression because 
> it refers to an incompletely initialized variable`.
> 
> This patch changes around the `Bytes` class a bit by getting rid of the small 
> classes that extend `Bytes`. Additionally, any usage of the `Duration` class 
> has been adjusted to not instantiate using the base type (which triggers the 
> issue along with `constexpr`).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/bytes.hpp 98223db50 
>   src/master/constants.hpp 7edf9f65f 
>   src/sched/constants.hpp 9edb25b38 
>   src/sched/sched.cpp ef73c1dcc 
>   src/scheduler/constants.hpp e3da12646 
>   src/slave/constants.hpp 1159ac3b1 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp cf0466c62 
>   src/slave/slave.cpp 7564e8d39 
>   src/slave/status_update_manager.cpp df63a708c 
> 
> 
> Diff: https://reviews.apache.org/r/59413/diff/2/
> 
> 
> Testing
> ---
> 
> `./bootstrap && mkdir build && cd build && ../configure --disable-python 
> --disable-java --disable-optimize --disable-hardening &&  make -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 59413: Fix constexpr compilation failure with GCC 7.1.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [59413]

Failed command: python support/apply-reviews.py -n -r 59413

Error:
error: patch failed: src/slave/constants.hpp:99
error: src/slave/constants.hpp: patch does not apply
error: patch failed: 
src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp:45
error: src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp: patch 
does not apply

Full log: NotAvailableYet\console

- Mesos Reviewbot Windows


On May 19, 2017, 6:51 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59413/
> ---
> 
> (Updated May 19, 2017, 6:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James Peach, Michael Park, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-7520
> https://issues.apache.org/jira/browse/MESOS-7520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Many of the `constexpr` variables fail to compile with errors such as `error: 
> 'Megabytes(32).Megabytes::' is not a constant expression because 
> it refers to an incompletely initialized variable`.
> 
> This patch changes around the `Bytes` class a bit by getting rid of the small 
> classes that extend `Bytes`. Additionally, any usage of the `Duration` class 
> has been adjusted to not instantiate using the base type (which triggers the 
> issue along with `constexpr`).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/bytes.hpp 98223db50 
>   src/master/constants.hpp 7edf9f65f 
>   src/sched/constants.hpp 9edb25b38 
>   src/sched/sched.cpp ef73c1dcc 
>   src/scheduler/constants.hpp e3da12646 
>   src/slave/constants.hpp 1159ac3b1 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp cf0466c62 
>   src/slave/slave.cpp 7564e8d39 
>   src/slave/status_update_manager.cpp df63a708c 
> 
> 
> Diff: https://reviews.apache.org/r/59413/diff/2/
> 
> 
> Testing
> ---
> 
> `./bootstrap && mkdir build && cd build && ../configure --disable-python 
> --disable-java --disable-optimize --disable-hardening &&  make -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 59413: Fix constexpr compilation failure with GCC 7.1.

[Aaron Wood via Review Board]

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

(Updated May 19, 2017, 6:51 p.m.)


Review request for mesos, Benjamin Bannier, James Peach, Michael Park, and Neil 
Conway.


Changes
---

Rebased on the latest HEAD.


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


Repository: mesos


Description
---

Many of the `constexpr` variables fail to compile with errors such as `error: 
'Megabytes(32).Megabytes::' is not a constant expression because it 
refers to an incompletely initialized variable`.

This patch changes around the `Bytes` class a bit by getting rid of the small 
classes that extend `Bytes`. Additionally, any usage of the `Duration` class 
has been adjusted to not instantiate using the base type (which triggers the 
issue along with `constexpr`).


Diffs (updated)
-

  3rdparty/stout/include/stout/bytes.hpp 98223db50 
  src/master/constants.hpp 7edf9f65f 
  src/sched/constants.hpp 9edb25b38 
  src/sched/sched.cpp ef73c1dcc 
  src/scheduler/constants.hpp e3da12646 
  src/slave/constants.hpp 1159ac3b1 
  src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp cf0466c62 
  src/slave/slave.cpp 7564e8d39 
  src/slave/status_update_manager.cpp df63a708c 


Diff: https://reviews.apache.org/r/59413/diff/2/

Changes: https://reviews.apache.org/r/59413/diff/1-2/


Testing
---

`./bootstrap && mkdir build && cd build && ../configure --disable-python 
--disable-java --disable-optimize --disable-hardening &&  make -j$(nproc)`


Thanks,

Aaron Wood

Re: Review Request 59413: Fix constexpr compilation failure with GCC 7.1.

[Neil Conway]

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



`stout`, `libprocess`, and `mesos` are nominally distinct projects, so a single 
RR should touch no more than one of them. i.e., please split the `stout` 
changes into a separate review.

Might also be worth waiting to see if GCC upstream is going to fix the problem 
promptly. If GCC 7.2 fixes the problem, we don't necessarily need to workaround 
a bug that occurs in just a single minor release of GCC.


src/master/constants.hpp
Line 49 (original), 49 (patched)


This change (and all the similar changes) seems unfortunate. Can't we play 
a similar trick to the change you made to `Bytes`?


- Neil Conway


On May 19, 2017, 6:45 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59413/
> ---
> 
> (Updated May 19, 2017, 6:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James Peach, Michael Park, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-7520
> https://issues.apache.org/jira/browse/MESOS-7520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Many of the `constexpr` variables fail to compile with errors such as `error: 
> 'Megabytes(32).Megabytes::' is not a constant expression because 
> it refers to an incompletely initialized variable`.
> 
> This patch changes around the `Bytes` class a bit by getting rid of the small 
> classes that extend `Bytes`. Additionally, any usage of the `Duration` class 
> has been adjusted to not instantiate using the base type (which triggers the 
> issue along with `constexpr`).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/bytes.hpp 98223db50 
>   src/master/constants.hpp 7edf9f65f 
>   src/sched/constants.hpp 9edb25b38 
>   src/sched/sched.cpp ef73c1dcc 
>   src/scheduler/constants.hpp e3da12646 
>   src/slave/constants.hpp 1159ac3b1 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp cf0466c62 
>   src/slave/slave.cpp 7564e8d39 
>   src/slave/status_update_manager.cpp df63a708c 
> 
> 
> Diff: https://reviews.apache.org/r/59413/diff/1/
> 
> 
> Testing
> ---
> 
> `./bootstrap && mkdir build && cd build && ../configure --disable-python 
> --disable-java --disable-optimize --disable-hardening &&  make -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 59413: Fix constexpr compilation failure with GCC 7.1.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [59413]

Failed command: python support/apply-reviews.py -n -r 59413

Error:
error: patch failed: src/slave/constants.hpp:99
error: src/slave/constants.hpp: patch does not apply
error: patch failed: 
src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp:45
error: src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp: patch 
does not apply

Full log: NotAvailableYet\console

- Mesos Reviewbot Windows


On May 19, 2017, 6:45 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59413/
> ---
> 
> (Updated May 19, 2017, 6:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James Peach, Michael Park, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-7520
> https://issues.apache.org/jira/browse/MESOS-7520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Many of the `constexpr` variables fail to compile with errors such as `error: 
> 'Megabytes(32).Megabytes::' is not a constant expression because 
> it refers to an incompletely initialized variable`.
> 
> This patch changes around the `Bytes` class a bit by getting rid of the small 
> classes that extend `Bytes`. Additionally, any usage of the `Duration` class 
> has been adjusted to not instantiate using the base type (which triggers the 
> issue along with `constexpr`).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/bytes.hpp 98223db50 
>   src/master/constants.hpp 7edf9f65f 
>   src/sched/constants.hpp 9edb25b38 
>   src/sched/sched.cpp ef73c1dcc 
>   src/scheduler/constants.hpp e3da12646 
>   src/slave/constants.hpp 1159ac3b1 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp cf0466c62 
>   src/slave/slave.cpp 7564e8d39 
>   src/slave/status_update_manager.cpp df63a708c 
> 
> 
> Diff: https://reviews.apache.org/r/59413/diff/1/
> 
> 
> Testing
> ---
> 
> `./bootstrap && mkdir build && cd build && ../configure --disable-python 
> --disable-java --disable-optimize --disable-hardening &&  make -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Review Request 59413: Fix constexpr compilation failure with GCC 7.1.

[Aaron Wood via Review Board]

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

Review request for mesos, Benjamin Bannier, James Peach, Michael Park, and Neil 
Conway.


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


Repository: mesos


Description
---

Many of the `constexpr` variables fail to compile with errors such as `error: 
'Megabytes(32).Megabytes::' is not a constant expression because it 
refers to an incompletely initialized variable`.

This patch changes around the `Bytes` class a bit by getting rid of the small 
classes that extend `Bytes`. Additionally, any usage of the `Duration` class 
has been adjusted to not instantiate using the base type (which triggers the 
issue along with `constexpr`).


Diffs
-

  3rdparty/stout/include/stout/bytes.hpp 98223db50 
  src/master/constants.hpp 7edf9f65f 
  src/sched/constants.hpp 9edb25b38 
  src/sched/sched.cpp ef73c1dcc 
  src/scheduler/constants.hpp e3da12646 
  src/slave/constants.hpp 1159ac3b1 
  src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp cf0466c62 
  src/slave/slave.cpp 7564e8d39 
  src/slave/status_update_manager.cpp df63a708c 


Diff: https://reviews.apache.org/r/59413/diff/1/


Testing
---

`./bootstrap && mkdir build && cd build && ../configure --disable-python 
--disable-java --disable-optimize --disable-hardening &&  make -j$(nproc)`


Thanks,

Aaron Wood

Re: Review Request 58358: Update vendored protobuf tar.gz to 3.3.0.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On May 19, 2017, 5:52 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58358/
> ---
> 
> (Updated May 19, 2017, 5:52 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Bugs: MESOS-7228
> https://issues.apache.org/jira/browse/MESOS-7228
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The content of `3rdparty/protobuf-3.2.0.tar.gz` is generated by:
> - On a Mac OS, download and untar protobuf v3.3.0 source at
>   https://github.com/google/protobuf/archive/v3.3.0.tar.gz;
> - Run `./autogen.sh`;
> - Recompress and tar by `tar -czvf`.
> 
> 
> Diffs
> -
> 
>   3rdparty/protobuf-2.6.1.tar.gz e1c2d541c1eb806e837ff5d1e9d3fdb678b40e06 
>   3rdparty/protobuf-3.3.0.tar.gz PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/58358/diff/6/
> 
> 
> Testing
> ---
> 
> 1. make check;
> 2. used the modified `test-upgrade.py` in r/58357 to test upgrade/downgrade 
> between this chain and Mesos 1.2;
> 3. build this branch in Apache Aurora's vagrant based box, and verifies that 
> Aurora scheduler (using java) and executor (using python) still works with 
> `libmesos.so` built from this branch.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 58514: Update related tests in stout to support protobuf 3.3.0.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On May 19, 2017, 5:54 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58514/
> ---
> 
> (Updated May 19, 2017, 5:54 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Bugs: MESOS-7228
> https://issues.apache.org/jira/browse/MESOS-7228
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Command to regenerate:
> ```
> ./build/3rdparty/protobuf-3.3.0//src/protoc \
>   -I 3rdparty/stout/tests \
>   --cpp_out=3rdparty/stout/tests/ \
>   3rdparty/stout/tests/protobuf_tests.proto
> ```
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/README.md f4e41aaacee4f88c708413e954553011229c75e4 
>   3rdparty/stout/tests/protobuf_tests.pb.h 
> 18870cde806c7bcb0a204de2a40555739adf777b 
>   3rdparty/stout/tests/protobuf_tests.pb.cc 
> b2458f0edcf679b2390f87ec8c4bee2b16e71d70 
> 
> 
> Diff: https://reviews.apache.org/r/58514/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 58361: Updated LICENSE information for protobuf 3.3.0.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On May 19, 2017, 5:58 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58361/
> ---
> 
> (Updated May 19, 2017, 5:58 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Bugs: MESOS-7228
> https://issues.apache.org/jira/browse/MESOS-7228
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Content copied from https://github.com/google/protobuf/blob/v3.3.0/LICENSE.
> 
> 
> Diffs
> -
> 
>   LICENSE 512b089c5dd2540f9f83afb559e5d0998d97ea22 
> 
> 
> Diff: https://reviews.apache.org/r/58361/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 58359: Update Mesos build library to use protobuf 3.3.0.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On May 19, 2017, 5:55 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58359/
> ---
> 
> (Updated May 19, 2017, 5:55 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Bugs: MESOS-7228
> https://issues.apache.org/jira/browse/MESOS-7228
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update Mesos build library to use protobuf 3.3.0.
> 
> 
> Diffs
> -
> 
>   configure.ac fe5b20b53ea007dd8f2e3349e9904f6c4f43a14b 
>   src/java/mesos.pom.in 080767f011782f048ab0244f93daea189326587c 
>   src/python/interface/setup.py.in 8e38f3feb705f08737cff93e0ed442a1f48dcbf9 
>   src/python/native_common/ext_modules.py.in 
> e0bb335e5ae663f398cea904a0e53e3c5fb210d7 
>   src/python/protocol/setup.py.in b7de10a2a099d21d6eba551ba071b6c31a1460be 
>   support/mesos-tidy/entrypoint.sh 5dbaa6086621934a7313dfa5c8008b6a6f5496f1 
> 
> 
> Diff: https://reviews.apache.org/r/58359/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 57815: Added `suppressed_roles` field in `SUBSCRIBE`.

[Anindya Sinha]

> On April 17, 2017, 6:28 p.m., Vinod Kone wrote:
> > include/mesos/scheduler/scheduler.proto
> > Lines 249-250 (original), 249-250 (patched)
> > 
> >
> > Hmm, I was hoping that we could re-use the `Call::Suppress` message 
> > here instead of re-defining it. That way any changes to suppress would be 
> > in sync.
> 
> Anindya Sinha wrote:
> Currently, `Call::Suppress` is:
> ```
>   message Suppress {
> optional string role = 1;
>   }
> ```
> 
> So, the `SUPPRESS` call currently allows suppressing offers for a 
> framework on a per-role basis (or all roles if `role` is not set). Since we 
> now support multiple roles for a frameework, and if we want to suppress 
> offers for a subset of roles at the time of `SUBSCRIBE`, using 
> `Call::Suppress` would not satisfy that use case. It would only satisfy 
> suppressing for all roles (i.e. `Suppress::role` is not set) or for a 
> specific role.
> 
> Is there any plans of extending `SUPPRESS` to suppress offers for a 
> subset of roles (>0, and < all roles) in a single call? Do you think we 
> should consider that also?
> 
> One such approach could be to have the following:
> ```
>   message Suppress {
> repeated string roles = 1;
>   }
> 
>   optional Suppress suppress;
> ```
> 
> If suppress is not set, then we do not suppress for any roles. Otherwise, 
> we suppress for the set of roles specified in `suppress.roles`. If `suppress` 
> is set but `suppress.roles` is empty, we suppress for all roles of the 
> framework.
> 
> Thoughts?
> 
> Anindya Sinha wrote:
> So based on discussion in the Slack channel, this modified review chain 
> does the following:
> 
> 1. Add `repeated string deactivated_roles` to `FrameworkInfo` which 
> represents a subset of roles which are deactivated. Offers pertaining to the 
> deactivated roles shall not be sent out.
> 2. `SUPPRESS` and `REVIVE` calls will toggle the (de)activation mode of 
> the roles.
> 3. Allocator's `activateFramework()` call will activate all roles of the 
> framework which are not in `FrameworkInfo::deactivated+roles`. Similarly, in 
> `deactivateFramework()` call will deactivate all roles that are not 
> deactivated already.

Based on discussion on Slack, this is what was decided:

1. Add `repeated string suppressed_roles` to `Call::Subscribe` which represents 
a subset of roles which are suppressed. Offers pertaining to the suppressed 
roles shall not be sent out.
2. `SUPPRESS` and `REVIVE` calls will toggle the (de)activation mode of the 
roles.
3. Allocator's `activateFramework()` call will activate all roles of the 
framework which are not in suppressed_roles. Similarly, in 
`deactivateFramework()` call will deactivate all roles that are not suppressed 
already.

Note that this functionality shall not be added for scheduler driver in this 
review chain (for MESOS-7015). I opened 
https://issues.apache.org/jira/browse/MESOS-7526 to address that.


- Anindya


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


On May 19, 2017, 6:26 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57815/
> ---
> 
> (Updated May 19, 2017, 6:26 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This field is a subset of roles the framework registered as for which
> the framework does not want any resources offere to.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> f83b2ce7e88e83abc4e523b06333c448a3dcfd01 
>   include/mesos/v1/scheduler/scheduler.proto 
> d923cb9dc4205e4b601feebba84e3b30091ea3e0 
> 
> 
> Diff: https://reviews.apache.org/r/57815/diff/6/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.

[Anindya Sinha]

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

(Updated May 19, 2017, 6:26 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Updates based on agreed approach.


Summary (updated)
-

Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.


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


Repository: mesos


Description (updated)
---

If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
framework (re)registration, the allocator does not offer resources for
those roles to such frameworks. Note that this functionality is added
for `v1::SUBSCRIBE` only (and not for scheduler driver API).

In addition, the master validates the suppressed roles to be a subset
of all the roles being (re)registered by the framework.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 
  src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 
  src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
  src/master/allocator/mesos/allocator.hpp 
119b461136123229f85ed3c3cfd41137974a6b9b 
  src/master/allocator/mesos/hierarchical.hpp 
123f97cf495bff0f822838e09df0d88818f04da6 
  src/master/allocator/mesos/hierarchical.cpp 
b75ed9a20a9a42f958cebbacd91e5e15b0d21394 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/master/master.cpp 02affe2d6dc76ef91363df04d8d8cbed3beaf34f 
  src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 
  src/tests/hierarchical_allocator_tests.cpp 
6dee2296d5a14185dbf7eee17968b20148839bfd 
  src/tests/master_allocator_tests.cpp d05ee441a5120144aff42d78c095e1ce5051a6ac 
  src/tests/persistent_volume_endpoints_tests.cpp 
8c54372b7f6d94f0335561086f2a8cb90373e285 
  src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad 
  src/tests/resource_offers_tests.cpp f0bca1d9e03013ce35215b0ffa6b50b38972dc0c 
  src/tests/slave_recovery_tests.cpp 52e78b6b6280a159233b402ce2849448204d4f11 


Diff: https://reviews.apache.org/r/57817/diff/8/

Changes: https://reviews.apache.org/r/57817/diff/7-8/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.

[Anindya Sinha]

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

(Updated May 19, 2017, 6:26 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Updates based on agreed approach.


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


Repository: mesos


Description
---

Added unit tests to verify offers are suppressed based on registration.


Diffs (updated)
-

  src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 


Diff: https://reviews.apache.org/r/57818/diff/8/

Changes: https://reviews.apache.org/r/57818/diff/7-8/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57815: Added `suppressed_roles` field in `SUBSCRIBE`.

[Anindya Sinha]

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

(Updated May 19, 2017, 6:26 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Updates based on agreed approach.


Summary (updated)
-

Added `suppressed_roles` field in `SUBSCRIBE`.


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


Repository: mesos


Description
---

This field is a subset of roles the framework registered as for which
the framework does not want any resources offere to.


Diffs (updated)
-

  include/mesos/scheduler/scheduler.proto 
f83b2ce7e88e83abc4e523b06333c448a3dcfd01 
  include/mesos/v1/scheduler/scheduler.proto 
d923cb9dc4205e4b601feebba84e3b30091ea3e0 


Diff: https://reviews.apache.org/r/57815/diff/6/

Changes: https://reviews.apache.org/r/57815/diff/5-6/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 58361: Updated LICENSE information for protobuf 3.3.0.

[Zhitao Li]

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

(Updated May 19, 2017, 5:58 p.m.)


Review request for mesos, Anand Mazumdar and Joseph Wu.


Summary (updated)
-

Updated LICENSE information for protobuf 3.3.0.


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


Repository: mesos


Description (updated)
---

Content copied from https://github.com/google/protobuf/blob/v3.3.0/LICENSE.


Diffs (updated)
-

  LICENSE 512b089c5dd2540f9f83afb559e5d0998d97ea22 


Diff: https://reviews.apache.org/r/58361/diff/9/

Changes: https://reviews.apache.org/r/58361/diff/8-9/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 58360: Added a test for evolving large protobuf message.

[Zhitao Li]

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

(Updated May 19, 2017, 5:57 p.m.)


Review request for mesos and Anand Mazumdar.


Bugs: MESOS-6644 and MESOS-7228
https://issues.apache.org/jira/browse/MESOS-6644
https://issues.apache.org/jira/browse/MESOS-7228


Repository: mesos


Description (updated)
---

Before protobuf 3.3.0, this test would fail because the 64MB limit
imposed by older version of protobuf.


Diffs (updated)
-

  src/tests/protobuf_utils_tests.cpp 5239182812835b93a28e85146b2df2b20ae77328 


Diff: https://reviews.apache.org/r/58360/diff/6/

Changes: https://reviews.apache.org/r/58360/diff/5-6/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 58359: Update Mesos build library to use protobuf 3.3.0.

[Zhitao Li]

> On April 19, 2017, 5:04 p.m., Anand Mazumdar wrote:
> > src/java/mesos.pom.in
> > Line 86 (original), 86 (patched)
> > 
> >
> > hmm, does this mean frameworks would be forced to move to proto3 and 
> > not keep using proto2?

Yes, I'd like to enforce an upgrade from framework side.


> On April 19, 2017, 5:04 p.m., Anand Mazumdar wrote:
> > src/python/interface/setup.py.in
> > Line 29 (original), 29 (patched)
> > 
> >
> > hmm, this seems to suggest that any framework using the python bindings 
> > would be forced to use protobuf 3.2.0 and not use proto2?

Yes, ditto as above.


> On April 19, 2017, 5:04 p.m., Anand Mazumdar wrote:
> > src/python/protocol/setup.py.in
> > Line 29 (original), 29 (patched)
> > 
> >
> > Ditto as above.

Yes, ditto as above.


- Zhitao


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


On May 19, 2017, 5:55 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58359/
> ---
> 
> (Updated May 19, 2017, 5:55 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Bugs: MESOS-7228
> https://issues.apache.org/jira/browse/MESOS-7228
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update Mesos build library to use protobuf 3.3.0.
> 
> 
> Diffs
> -
> 
>   configure.ac fe5b20b53ea007dd8f2e3349e9904f6c4f43a14b 
>   src/java/mesos.pom.in 080767f011782f048ab0244f93daea189326587c 
>   src/python/interface/setup.py.in 8e38f3feb705f08737cff93e0ed442a1f48dcbf9 
>   src/python/native_common/ext_modules.py.in 
> e0bb335e5ae663f398cea904a0e53e3c5fb210d7 
>   src/python/protocol/setup.py.in b7de10a2a099d21d6eba551ba071b6c31a1460be 
>   support/mesos-tidy/entrypoint.sh 5dbaa6086621934a7313dfa5c8008b6a6f5496f1 
> 
> 
> Diff: https://reviews.apache.org/r/58359/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 58359: Update Mesos build library to use protobuf 3.3.0.

[Zhitao Li]

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

(Updated May 19, 2017, 5:55 p.m.)


Review request for mesos, Anand Mazumdar and Joseph Wu.


Changes
---

use 3.3.0 instead.


Summary (updated)
-

Update Mesos build library to use protobuf 3.3.0.


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


Repository: mesos


Description (updated)
---

Update Mesos build library to use protobuf 3.3.0.


Diffs (updated)
-

  configure.ac fe5b20b53ea007dd8f2e3349e9904f6c4f43a14b 
  src/java/mesos.pom.in 080767f011782f048ab0244f93daea189326587c 
  src/python/interface/setup.py.in 8e38f3feb705f08737cff93e0ed442a1f48dcbf9 
  src/python/native_common/ext_modules.py.in 
e0bb335e5ae663f398cea904a0e53e3c5fb210d7 
  src/python/protocol/setup.py.in b7de10a2a099d21d6eba551ba071b6c31a1460be 
  support/mesos-tidy/entrypoint.sh 5dbaa6086621934a7313dfa5c8008b6a6f5496f1 


Diff: https://reviews.apache.org/r/58359/diff/5/

Changes: https://reviews.apache.org/r/58359/diff/4-5/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 58515: Update 3rdparty build to support protobuf 3.3.0.

[Zhitao Li]

> On May 17, 2017, 9:57 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/cmake/Versions.cmake
> > Line 31 (original), 31 (patched)
> > 
> >
> > If 3.2.0 compiles on Windows then we can resolve MESOS-3453.
> 
> Chun-Hung Hsiao wrote:
> If we want to address this then `3rdparty/CMakeList.txt` needs 
> corresponding changes. Or maybe address this in another RR?

I'd like to address this in another follow up.


- Zhitao


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


On May 19, 2017, 5:54 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58515/
> ---
> 
> (Updated May 19, 2017, 5:54 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Bugs: MESOS-7228
> https://issues.apache.org/jira/browse/MESOS-7228
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update 3rdparty build to support protobuf 3.3.0.
> 
> 
> Diffs
> -
> 
>   3rdparty/cmake/Versions.cmake 728f88fe57aef4dd7be1d40c7c0227ad2db5015e 
>   3rdparty/versions.am b8144702adca9dd41199f69cb6fb0c0c4b490dbd 
> 
> 
> Diff: https://reviews.apache.org/r/58515/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 58515: Update 3rdparty build to support protobuf 3.3.0.

[Zhitao Li]

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

(Updated May 19, 2017, 5:54 p.m.)


Review request for mesos, Anand Mazumdar and Joseph Wu.


Changes
---

Use 3.3.0 instead.


Summary (updated)
-

Update 3rdparty build to support protobuf 3.3.0.


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


Repository: mesos


Description (updated)
---

Update 3rdparty build to support protobuf 3.3.0.


Diffs (updated)
-

  3rdparty/cmake/Versions.cmake 728f88fe57aef4dd7be1d40c7c0227ad2db5015e 
  3rdparty/versions.am b8144702adca9dd41199f69cb6fb0c0c4b490dbd 


Diff: https://reviews.apache.org/r/58515/diff/4/

Changes: https://reviews.apache.org/r/58515/diff/3-4/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 58514: Update related tests in stout to support protobuf 3.3.0.

[Zhitao Li]

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

(Updated May 19, 2017, 5:54 p.m.)


Review request for mesos, Anand Mazumdar and Joseph Wu.


Changes
---

use 3.3.0 instead.


Summary (updated)
-

Update related tests in stout to support protobuf 3.3.0.


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


Repository: mesos


Description (updated)
---

Command to regenerate:
```
./build/3rdparty/protobuf-3.3.0//src/protoc \
  -I 3rdparty/stout/tests \
  --cpp_out=3rdparty/stout/tests/ \
  3rdparty/stout/tests/protobuf_tests.proto
```


Diffs (updated)
-

  3rdparty/stout/README.md f4e41aaacee4f88c708413e954553011229c75e4 
  3rdparty/stout/tests/protobuf_tests.pb.h 
18870cde806c7bcb0a204de2a40555739adf777b 
  3rdparty/stout/tests/protobuf_tests.pb.cc 
b2458f0edcf679b2390f87ec8c4bee2b16e71d70 


Diff: https://reviews.apache.org/r/58514/diff/3/

Changes: https://reviews.apache.org/r/58514/diff/2-3/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 58513: Remove unnecessary patch after protobuf upgrade to 3.3.0.

[Zhitao Li]

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

(Updated May 19, 2017, 5:53 p.m.)


Review request for mesos, Anand Mazumdar and Joseph Wu.


Changes
---

use 3.3.0 instead.


Summary (updated)
-

Remove unnecessary patch after protobuf upgrade to 3.3.0.


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


Repository: mesos


Description
---

The patch should already be included in protobuf 3.0.0, so
this is not necessary anymore.


Diffs (updated)
-

  3rdparty/CMakeLists.txt cb118f6c454c3bb36990e292a31703e4a3f99483 
  3rdparty/Makefile.am 61d832b2d83cdeaf95341c062e8493ab72d0724e 
  3rdparty/protobuf-2.6.1.patch bfd3bb2c600383ef05bdb4ea9df0188b4a560315 


Diff: https://reviews.apache.org/r/58513/diff/4/

Changes: https://reviews.apache.org/r/58513/diff/3-4/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 58358: Update vendored protobuf tar.gz to 3.3.0.

[Zhitao Li]

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

(Updated May 19, 2017, 5:52 p.m.)


Review request for mesos, Anand Mazumdar and Joseph Wu.


Changes
---

Use 3.3.0 instead.


Summary (updated)
-

Update vendored protobuf tar.gz to 3.3.0.


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


Repository: mesos


Description (updated)
---

The content of `3rdparty/protobuf-3.2.0.tar.gz` is generated by:
- On a Mac OS, download and untar protobuf v3.3.0 source at
  https://github.com/google/protobuf/archive/v3.3.0.tar.gz;
- Run `./autogen.sh`;
- Recompress and tar by `tar -czvf`.


Diffs (updated)
-

  3rdparty/protobuf-2.6.1.tar.gz e1c2d541c1eb806e837ff5d1e9d3fdb678b40e06 
  3rdparty/protobuf-3.3.0.tar.gz PRE-CREATION 


Diff: https://reviews.apache.org/r/58358/diff/6/

Changes: https://reviews.apache.org/r/58358/diff/5-6/


Testing
---

1. make check;
2. used the modified `test-upgrade.py` in r/58357 to test upgrade/downgrade 
between this chain and Mesos 1.2;
3. build this branch in Apache Aurora's vagrant based box, and verifies that 
Aurora scheduler (using java) and executor (using python) still works with 
`libmesos.so` built from this branch.


Thanks,

Zhitao Li

Re: Review Request 59375: Added REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [59375]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On May 19, 2017, 2:54 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59375/
> ---
> 
> (Updated May 19, 2017, 2:54 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-6905
> https://issues.apache.org/jira/browse/MESOS-6905
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This gives clear indication about the reason of task status updates
> caused by task health update.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e138e2aedb1369acdde82facf9dfea9780411154 
>   include/mesos/v1/mesos.proto ab617bfd2bc46e44f7d71cefecf5eac3630d22b3 
>   src/docker/executor.cpp 82ae9bdc94565acb746724c2e6ab6432aa6bd751 
>   src/launcher/default_executor.cpp 5c31f948b5ba470e7c007c39e1c0c903451477e2 
>   src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
>   src/tests/check_tests.cpp 3607a690c963a84acfb2d3adc6c4a4f148b456d6 
>   src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad 
> 
> 
> Diff: https://reviews.apache.org/r/59375/diff/2/
> 
> 
> Testing
> ---
> 
> Added checks for task status updates caused by task health update.
> 
> make check (fedora 25, mac os 10.12)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 59375: Added REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason.

[Andrei Budnik]

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

(Updated May 19, 2017, 2:54 p.m.)


Review request for mesos and Alexander Rukletsov.


Summary (updated)
-

Added REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason.


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


Repository: mesos


Description
---

This gives clear indication about the reason of task status updates
caused by task health update.


Diffs (updated)
-

  include/mesos/mesos.proto e138e2aedb1369acdde82facf9dfea9780411154 
  include/mesos/v1/mesos.proto ab617bfd2bc46e44f7d71cefecf5eac3630d22b3 
  src/docker/executor.cpp 82ae9bdc94565acb746724c2e6ab6432aa6bd751 
  src/launcher/default_executor.cpp 5c31f948b5ba470e7c007c39e1c0c903451477e2 
  src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
  src/tests/check_tests.cpp 3607a690c963a84acfb2d3adc6c4a4f148b456d6 
  src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad 


Diff: https://reviews.apache.org/r/59375/diff/2/

Changes: https://reviews.apache.org/r/59375/diff/1-2/


Testing
---

Added checks for task status updates caused by task health update.

make check (fedora 25, mac os 10.12)


Thanks,

Andrei Budnik

Re: Review Request 59375: Added REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason. (WIP)

[Andrei Budnik]

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

(Updated May 19, 2017, 2:30 p.m.)


Review request for mesos and Alexander Rukletsov.


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


Repository: mesos


Description
---

This gives clear indication about the reason of task status updates
caused by task health update.


Diffs
-

  include/mesos/mesos.proto e138e2aedb1369acdde82facf9dfea9780411154 
  src/docker/executor.cpp 82ae9bdc94565acb746724c2e6ab6432aa6bd751 
  src/launcher/default_executor.cpp 5c31f948b5ba470e7c007c39e1c0c903451477e2 
  src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
  src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad 


Diff: https://reviews.apache.org/r/59375/diff/1/


Testing (updated)
---

Added checks for task status updates caused by task health update.

make check (fedora 25, mac os 10.12)


Thanks,

Andrei Budnik

Re: Review Request 58964: Added authorization support for operator endpoints.

[Alexander Rojas]

> On May 17, 2017, 11:57 a.m., Adam B wrote:
> > include/mesos/authorizer/acls.proto
> > Lines 364 (patched)
> > 
> >
> > Why do we think `machines` is the entity we want to authorize on? What 
> > if we decide we want to authorize on `schedules` in the future? This 
> > required field isn't very flexible.
> > Also, why not `agents` like in `RegisterAgent` above. Is there a 
> > distinction between agents and machines?

Schedules could be an interesting way to authorize, but also they would define 
a rather complex object which is not easy to specify by an entity. Moreover, a 
schedule is a beginning time, a duration and a set of machines. How you do 
define equality on them? does it make sense to say that someone is authorized 
to create a schedule in certain times and not in others. Likewise, machine 
could contain multiple agents. So what does it mean to be able to authorize one 
agent but not another in the same machine? that is why I decided machines made 
much more sense. 

Moreover, the request to set a maintenance schedule comes with a set of 
`machines_id`, which makes authorization much more easier and intuitive than 
using `agent_id` or any other.


> On May 17, 2017, 11:57 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.proto
> > Lines 58 (patched)
> > 
> >
> > Unused?!?

sorry, original I was planning to have the request use the machine ID to be 
authorized. I still think it makes sense to give the machine ID, which the 
authorizer could ignore. Let's decide on that.


- Alexander


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


On May 12, 2017, 2:51 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58964/
> ---
> 
> (Updated May 12, 2017, 2:51 p.m.)
> 
> 
> Review request for mesos, Adam B and Greg Mann.
> 
> 
> Bugs: MESOS-7415
> https://issues.apache.org/jira/browse/MESOS-7415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the actions `UPDATE_MAINTENANCE_SCHEDULE`,
> `GET_MAINTENANCE_SCHEDULE`, `START_MAINTENANCE`, `STOP_MAINTENANCE`
> and `GET_MAINTENANCE_STATUS` to the authorizer API as well as the
> necesary code to handle these new actions.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> ae0b1ea2e6417d186b1606542d75f3a20e0811db 
>   include/mesos/authorizer/authorizer.proto 
> c9184d151befa4cea9bdebb36a315c760e6424b2 
>   src/authorizer/local/authorizer.cpp 
> 89aaf4b712d337d519445c922606789c334e5101 
>   src/tests/authorization_tests.cpp 32aa6ac4db7854507127ea2fb88b3e92daa277c0 
> 
> 
> Diff: https://reviews.apache.org/r/58964/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 58964: Added authorization support for operator endpoints.

[Alexander Rojas]

> On May 17, 2017, 11:57 a.m., Adam B wrote:
> > include/mesos/authorizer/acls.proto
> > Line 354 (original), 354 (patched)
> > 
> >
> > Should this be `agents` plural?

Probably, but if we want to change it it should be a blocker for 1.3.0. Once it 
lands we will have to go through a six months deprecation cycle.


- Alexander


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


On May 12, 2017, 2:51 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58964/
> ---
> 
> (Updated May 12, 2017, 2:51 p.m.)
> 
> 
> Review request for mesos, Adam B and Greg Mann.
> 
> 
> Bugs: MESOS-7415
> https://issues.apache.org/jira/browse/MESOS-7415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the actions `UPDATE_MAINTENANCE_SCHEDULE`,
> `GET_MAINTENANCE_SCHEDULE`, `START_MAINTENANCE`, `STOP_MAINTENANCE`
> and `GET_MAINTENANCE_STATUS` to the authorizer API as well as the
> necesary code to handle these new actions.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> ae0b1ea2e6417d186b1606542d75f3a20e0811db 
>   include/mesos/authorizer/authorizer.proto 
> c9184d151befa4cea9bdebb36a315c760e6424b2 
>   src/authorizer/local/authorizer.cpp 
> 89aaf4b712d337d519445c922606789c334e5101 
>   src/tests/authorization_tests.cpp 32aa6ac4db7854507127ea2fb88b3e92daa277c0 
> 
> 
> Diff: https://reviews.apache.org/r/58964/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 59375: Added REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason. (WIP)

[Andrei Budnik]

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

(Updated May 19, 2017, 10:14 a.m.)


Review request for mesos and Alexander Rukletsov.


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


Repository: mesos


Description
---

This gives clear indication about the reason of task status updates
caused by task health update.


Diffs
-

  include/mesos/mesos.proto e138e2aedb1369acdde82facf9dfea9780411154 
  src/docker/executor.cpp 82ae9bdc94565acb746724c2e6ab6432aa6bd751 
  src/launcher/default_executor.cpp 5c31f948b5ba470e7c007c39e1c0c903451477e2 
  src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
  src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad 


Diff: https://reviews.apache.org/r/59375/diff/1/


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 59375: Added REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason. (WIP)

[Alexander Rukletsov]

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



Thanks for the patch, Andrei! First of all, let's address some process issues. 
We reference the JIRA ticket in the review, please do that. We also mention a 
review in the ticket, so that those who watch the ticket are given the 
opportunity to have a look at your patch. I'm sure you did some testing around 
your patch, could you please mention it in the appropriate section?

I see that you modify only unversioned API. Is it intentional?

There are two more tests using health status updates: 
`CommandCheckAndHealthCheckNoShadowing`. Could you please update those as well?


include/mesos/mesos.proto
Lines 1870 (patched)


Any reason you do not update v1 API as well?



src/docker/executor.cpp
Lines 486-487 (patched)


See my comment above.



src/launcher/default_executor.cpp
Lines 795-797 (original), 795-799 (patched)


Why do you think the reason here should be updated task health? This update 
is sent for the finished task, so the reason is more "task finished". We do 
include health information here, but I'm not sure we should modify the reason.



src/launcher/executor.cpp
Lines 917-920 (original), 917-921 (patched)


See my comment above.



src/tests/health_check_tests.cpp
Lines 479-481 (patched)


Why do you use asserts here and everywhere?


- Alexander Rukletsov


On May 18, 2017, 8 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59375/
> ---
> 
> (Updated May 18, 2017, 8 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This gives clear indication about the reason of task status updates
> caused by task health update.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e138e2aedb1369acdde82facf9dfea9780411154 
>   src/docker/executor.cpp 82ae9bdc94565acb746724c2e6ab6432aa6bd751 
>   src/launcher/default_executor.cpp 5c31f948b5ba470e7c007c39e1c0c903451477e2 
>   src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
>   src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad 
> 
> 
> Diff: https://reviews.apache.org/r/59375/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>