Re: Review Request 67616: Added a length validation for container IDs.

[wei xiao]

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

(Updated 六月 19, 2018, 5:45 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Summary (updated)
-

Added a length validation for container IDs.


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


Repository: mesos


Description (updated)
---

Added a length validation for container IDs.


Diffs (updated)
-

  src/slave/constants.hpp b97daf3d2eb04b796de5283d9adb0f515ca69f8c 
  src/slave/validation.cpp 09f1fc702a1b4550c04bc9c99d5ebd17974ebbb1 
  src/tests/slave_validation_tests.cpp d8bc142dd707f0888c29bf070135d5d0083ef421 


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

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


Testing
---


Thanks,

wei xiao

Re: Review Request 67616: Added the limitation of the container id length.

[wei xiao]

> On 六月 18, 2018, 11:37 p.m., Gastón Kleiman wrote:
> > src/slave/constants.hpp
> > Lines 89 (patched)
> > 
> >
> > We should add a comment saying how we got to this magic number.
> > 
> > Something like: `This value is set to AUFS' per-path-component limit`.
> > 
> > We should check whether AUFS' limit is inclusive or exclusive, i.e., 
> > whether it the check should be: `containerId.length() < 
> > MAX_NESTED_CONTAINER_ID_LENGTH` or ``containerId.length() <= 
> > MAX_NESTED_CONTAINER_ID_LENGTH`

Thank you. I tested the aufs filename limitation in my laptop, 242 is ok, 243 
failed.


- wei


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


On 六月 18, 2018, 10:56 p.m., wei xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67616/
> ---
> 
> (Updated 六月 18, 2018, 10:56 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7168
> https://issues.apache.org/jira/browse/MESOS-7168
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the limitation of the container id length.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp b97daf3d2eb04b796de5283d9adb0f515ca69f8c 
>   src/slave/validation.cpp 09f1fc702a1b4550c04bc9c99d5ebd17974ebbb1 
>   src/tests/slave_validation_tests.cpp 
> d8bc142dd707f0888c29bf070135d5d0083ef421 
> 
> 
> Diff: https://reviews.apache.org/r/67616/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> wei xiao
> 
>

Re: Review Request 65995: Ensured wanted offers in `RetryOperationStatusUpdate*` SLRP tests.

[Chun-Hung Hsiao]

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

(Updated June 19, 2018, 4:31 a.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, and Jie 
Yu.


Summary (updated)
-

Ensured wanted offers in `RetryOperationStatusUpdate*` SLRP tests.


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


Repository: mesos


Description (updated)
---

The two SLRP tests assume that SLRP will send out a RAW resource before
the test framework registers, and thus the framework would receive the
RAW resource in its first offer, but this assumption is not guaranteed.
This patch ensures the assumption by settling the clock in advance.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
1afe8a8e0413ef225e952cd9cd6376e5d82774e5 


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

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


Testing
---

sudo make check
Ran the two tests in repitition.


Thanks,

Chun-Hung Hsiao

Re: Review Request 67616: Added the limitation of the container id length.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67616]

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

- Mesos Reviewbot


On June 18, 2018, 3:56 p.m., wei xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67616/
> ---
> 
> (Updated June 18, 2018, 3:56 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7168
> https://issues.apache.org/jira/browse/MESOS-7168
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the limitation of the container id length.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp b97daf3d2eb04b796de5283d9adb0f515ca69f8c 
>   src/slave/validation.cpp 09f1fc702a1b4550c04bc9c99d5ebd17974ebbb1 
>   src/tests/slave_validation_tests.cpp 
> d8bc142dd707f0888c29bf070135d5d0083ef421 
> 
> 
> Diff: https://reviews.apache.org/r/67616/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> wei xiao
> 
>

Re: Review Request 67616: Added the limitation of the container id length.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67616 was successfully built and tested.

Reviews applied: `['67616']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67616

- Mesos Reviewbot Windows


On June 18, 2018, 10:56 p.m., wei xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67616/
> ---
> 
> (Updated June 18, 2018, 10:56 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7168
> https://issues.apache.org/jira/browse/MESOS-7168
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the limitation of the container id length.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp b97daf3d2eb04b796de5283d9adb0f515ca69f8c 
>   src/slave/validation.cpp 09f1fc702a1b4550c04bc9c99d5ebd17974ebbb1 
>   src/tests/slave_validation_tests.cpp 
> d8bc142dd707f0888c29bf070135d5d0083ef421 
> 
> 
> Diff: https://reviews.apache.org/r/67616/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> wei xiao
> 
>

Re: Review Request 67616: Added the limitation of the container id length.

[Gastón Kleiman]

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



Thank you for your contribution! The patch looks pretty good, I just opened a 
few minor issues.

I noticed that I was looking at an old revision, so I closed some of the ones 
that were already addresed.

Small suggestion for the patch summary: `Added a length validation for 
container IDs.`

- Gastón Kleiman


On June 18, 2018, 3:56 p.m., wei xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67616/
> ---
> 
> (Updated June 18, 2018, 3:56 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7168
> https://issues.apache.org/jira/browse/MESOS-7168
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the limitation of the container id length.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp b97daf3d2eb04b796de5283d9adb0f515ca69f8c 
>   src/slave/validation.cpp 09f1fc702a1b4550c04bc9c99d5ebd17974ebbb1 
>   src/tests/slave_validation_tests.cpp 
> d8bc142dd707f0888c29bf070135d5d0083ef421 
> 
> 
> Diff: https://reviews.apache.org/r/67616/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> wei xiao
> 
>

Re: Review Request 67616: Added the limitation of the container id length.

[Gastón Kleiman]

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




src/slave/validation.cpp
Lines 58-59 (patched)


We use stout's `stringify()` instead of `std::to_string()` in our codebase.

We also try to avoid having trailing spaces in a string literal, preferring 
having them at the beginning of a line.

I think we should also write "maximum" instead of "max".

So I'd change these lines to:

```
return Error("'ContainerID.value' '" + id + "' exceeds the maximum"
 " length (" + stringify(MAX_CONTAINER_ID_LENGTH) + ")");
```


- Gastón Kleiman


On June 18, 2018, 3:56 p.m., wei xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67616/
> ---
> 
> (Updated June 18, 2018, 3:56 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7168
> https://issues.apache.org/jira/browse/MESOS-7168
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the limitation of the container id length.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp b97daf3d2eb04b796de5283d9adb0f515ca69f8c 
>   src/slave/validation.cpp 09f1fc702a1b4550c04bc9c99d5ebd17974ebbb1 
>   src/tests/slave_validation_tests.cpp 
> d8bc142dd707f0888c29bf070135d5d0083ef421 
> 
> 
> Diff: https://reviews.apache.org/r/67616/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> wei xiao
> 
>

Re: Review Request 67616: Added the limitation of the container id length.

[Gastón Kleiman]

> On June 18, 2018, 4:37 p.m., Gastón Kleiman wrote:
> > src/slave/validation.cpp
> > Lines 75-77 (patched)
> > 
> >
> > Is there any reason why we shouldn't perform this validation for 
> > containers without a parent container? 
> > 
> > I think we should move this block outside the 
> > `containerId.has_parent()` check.

A similar issue was opened by Joseph and has been addressed, dropping this one.


> On June 18, 2018, 4:37 p.m., Gastón Kleiman wrote:
> > src/slave/validation.cpp
> > Lines 76 (patched)
> > 
> >
> > I believe that this is a user-facing error (it shows up in logs and 
> > might even be sent in an HTTP response), so it needs to be a bit friendlier.
> > 
> > I suggest something like:
> > 
> > ```
> > "'ContainerID.value' exceeds " + 
> > stringify(MAX_NESTED_CONTAINER_ID_LENGTH) +" characters".
> > ```

Dropping this one as it was already addresed.


- Gastón


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


On June 18, 2018, 3:56 p.m., wei xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67616/
> ---
> 
> (Updated June 18, 2018, 3:56 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7168
> https://issues.apache.org/jira/browse/MESOS-7168
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the limitation of the container id length.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp b97daf3d2eb04b796de5283d9adb0f515ca69f8c 
>   src/slave/validation.cpp 09f1fc702a1b4550c04bc9c99d5ebd17974ebbb1 
>   src/tests/slave_validation_tests.cpp 
> d8bc142dd707f0888c29bf070135d5d0083ef421 
> 
> 
> Diff: https://reviews.apache.org/r/67616/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> wei xiao
> 
>

Re: Review Request 67616: Added the limitation of the container id length.

[Gastón Kleiman]

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




src/slave/constants.hpp
Lines 89 (patched)


We should add a comment saying how we got to this magic number.

Something like: `This value is set to AUFS' per-path-component limit`.

We should check whether AUFS' limit is inclusive or exclusive, i.e., 
whether it the check should be: `containerId.length() < 
MAX_NESTED_CONTAINER_ID_LENGTH` or ``containerId.length() <= 
MAX_NESTED_CONTAINER_ID_LENGTH`



src/slave/validation.cpp
Lines 18 (patched)


We sort includes from the same component lexicographically, so this include 
should go before line #17.



src/slave/validation.cpp
Lines 75-77 (patched)


Is there any reason why we shouldn't perform this validation for containers 
without a parent container? 

I think we should move this block outside the `containerId.has_parent()` 
check.



src/slave/validation.cpp
Lines 76 (patched)


I believe that this is a user-facing error (it shows up in logs and might 
even be sent in an HTTP response), so it needs to be a bit friendlier.

I suggest something like:

```
"'ContainerID.value' exceeds " + stringify(MAX_NESTED_CONTAINER_ID_LENGTH) 
+" characters".
```



src/tests/slave_validation_tests.cpp
Lines 96 (patched)


s/id/ID/


- Gastón Kleiman


On June 18, 2018, 3:56 p.m., wei xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67616/
> ---
> 
> (Updated June 18, 2018, 3:56 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7168
> https://issues.apache.org/jira/browse/MESOS-7168
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the limitation of the container id length.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp b97daf3d2eb04b796de5283d9adb0f515ca69f8c 
>   src/slave/validation.cpp 09f1fc702a1b4550c04bc9c99d5ebd17974ebbb1 
>   src/tests/slave_validation_tests.cpp 
> d8bc142dd707f0888c29bf070135d5d0083ef421 
> 
> 
> Diff: https://reviews.apache.org/r/67616/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> wei xiao
> 
>

Re: Review Request 67616: Added the limitation of the container id length.

[wei xiao]

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

(Updated 六月 18, 2018, 10:56 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Summary (updated)
-

Added the limitation of the container id length.


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


Repository: mesos


Description (updated)
---

Added the limitation of the container id length.


Diffs (updated)
-

  src/slave/constants.hpp b97daf3d2eb04b796de5283d9adb0f515ca69f8c 
  src/slave/validation.cpp 09f1fc702a1b4550c04bc9c99d5ebd17974ebbb1 
  src/tests/slave_validation_tests.cpp d8bc142dd707f0888c29bf070135d5d0083ef421 


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

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


Testing
---


Thanks,

wei xiao

Re: Review Request 66875: Added an hourly timer for docker store pull latency.

[Jason Lai]

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


Ship it!




Ship It!

- Jason Lai


On May 21, 2018, 5:16 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66875/
> ---
> 
> (Updated May 21, 2018, 5:16 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6451
> https://issues.apache.org/jira/browse/MESOS-6451
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch added pull latency tracking for docker store, which can tell
> us both latency distribution of pull as well as number of pulls.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 8b3f07f5027cb90d4b4ed401960494709d3eda5f 
> 
> 
> Diff: https://reviews.apache.org/r/66875/diff/2/
> 
> 
> Testing
> ---
> 
> Ran agent in command line and trigger several launches through 
> `mesos-execute`, observed following metrics from agent endpoint:
> 
> ```
>   "containerizer/mesos/docker_store/pull_ms": 4084.254208,
>   "containerizer/mesos/docker_store/pull_ms/count": 2,
>   "containerizer/mesos/docker_store/pull_ms/max": 4084.254208,
>   "containerizer/mesos/docker_store/pull_ms/min": 3525.044736,
>   "containerizer/mesos/docker_store/pull_ms/p50": 3804.649472,
>   "containerizer/mesos/docker_store/pull_ms/p90": 4028.3332608,
>   "containerizer/mesos/docker_store/pull_ms/p95": 4056.2937344,
>   "containerizer/mesos/docker_store/pull_ms/p99": 4078.66211328,
>   "containerizer/mesos/docker_store/pull_ms/p999": 4083.694998528,
>   "containerizer/mesos/docker_store/pull_ms/p": 4084.1982870528,
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 53105: Added an hourly timer for `slave/docker_containerizer/pull`.

[Jason Lai]

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


Ship it!




Ship It!

- Jason Lai


On May 21, 2018, 5:16 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53105/
> ---
> 
> (Updated May 21, 2018, 5:16 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6451
> https://issues.apache.org/jira/browse/MESOS-6451
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an hourly timer for `slave/docker_containerizer/pull`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 6bdf4c7f60384473927de437a061fafe94170557 
>   src/slave/containerizer/docker.cpp a4c9c10e91e75f406329cbb2086f39b549cbeed0 
> 
> 
> Diff: https://reviews.apache.org/r/53105/diff/5/
> 
> 
> Testing
> ---
> 
> Ran `mesos-execute` with a docker image and docker containerizer, observed 
> the following metrics:
> ```
> curl -s localhost:5051/metrics/snapshot | jq . | grep pull
>   "containerizer/docker/pull_ms/p999": 26209.36173824,
>   "containerizer/docker/pull_ms/p90": 21036.405248,
>   "containerizer/docker/pull_ms/p": 26256.388615424,
>   "containerizer/docker/pull_ms/p50": 135.570944,
>   "containerizer/docker/pull_ms/max": 26261.613824,
>   "containerizer/docker/pull_ms/p95": 23649.009536,
>   "containerizer/docker/pull_ms/min": 103.215872,
>   "containerizer/docker/pull_ms/p99": 25739.0929664,
>   "containerizer/docker/pull_ms": 26261.613824,
>   "containerizer/docker/pull_ms/count": 3
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 67616: Added the limitation of the nested container id length.

[Joseph Wu]

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




src/slave/constants.hpp
Lines 88-89 (patched)


Can you add a sentence on why `242` was chosen?  (i.e. max entry length for 
directory names on AUFS)

Also, comments should end with a period.  The style checker should catch 
things like this if you set up the commit hooks properly.



src/slave/validation.cpp
Lines 75-77 (patched)


Let's move this check out of the nested case.  We should check it in all 
cases instead.

For normal container launches, the ContainerID is agent-generated, so we 
should never fail validation there.  But standalone containers are 
user-specified top-level containers.  So we need to enforce the length limit 
there too.



src/slave/validation.cpp
Lines 76 (patched)


Looks like this error message is missing a part after the "exceeds" (i.e. 
"exceeds the max length (242)").


- Joseph Wu


On June 15, 2018, 12:07 p.m., wei xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67616/
> ---
> 
> (Updated June 15, 2018, 12:07 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7168
> https://issues.apache.org/jira/browse/MESOS-7168
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the limitation of the nested container id length.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp b97daf3d2eb04b796de5283d9adb0f515ca69f8c 
>   src/slave/validation.cpp 09f1fc702a1b4550c04bc9c99d5ebd17974ebbb1 
>   src/tests/slave_validation_tests.cpp 
> d8bc142dd707f0888c29bf070135d5d0083ef421 
> 
> 
> Diff: https://reviews.apache.org/r/67616/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> wei xiao
> 
>

Re: Review Request 67603: Made sure all configured shell script outputs are executable.

[Andrew Schwartzmeyer]

> On June 18, 2018, 1:25 p.m., Andrew Schwartzmeyer wrote:
> > Didn't https://reviews.apache.org/r/67604 deprecate this review?
> 
> Benjamin Bannier wrote:
> Unfortunately not since `AC_CONFIG_FILES` does not preserve permissions.

Ew. :(


- Andrew


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


On June 14, 2018, 3:13 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67603/
> ---
> 
> (Updated June 14, 2018, 3:13 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made sure all configured shell script outputs are executable.
> 
> 
> Diffs
> -
> 
>   configure.ac f5a9d5bded40d2af6df7fe872395b076cbd37123 
> 
> 
> Diff: https://reviews.apache.org/r/67603/diff/1/
> 
> 
> Testing
> ---
> 
> * `../configure`; all shell scripts created in the build tree are executable
> * `make distcheck` for good measure, but likely would not be able to detect 
> issues here
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67603: Made sure all configured shell script outputs are executable.

[Benjamin Bannier]

> On June 18, 2018, 10:25 p.m., Andrew Schwartzmeyer wrote:
> > Didn't https://reviews.apache.org/r/67604 deprecate this review?

Unfortunately not since `AC_CONFIG_FILES` does not preserve permissions.


- Benjamin


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


On June 15, 2018, 12:13 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67603/
> ---
> 
> (Updated June 15, 2018, 12:13 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made sure all configured shell script outputs are executable.
> 
> 
> Diffs
> -
> 
>   configure.ac f5a9d5bded40d2af6df7fe872395b076cbd37123 
> 
> 
> Diff: https://reviews.apache.org/r/67603/diff/1/
> 
> 
> Testing
> ---
> 
> * `../configure`; all shell scripts created in the build tree are executable
> * `make distcheck` for good measure, but likely would not be able to detect 
> issues here
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67603: Made sure all configured shell script outputs are executable.

[Andrew Schwartzmeyer]

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



Didn't https://reviews.apache.org/r/67604 deprecate this review?

- Andrew Schwartzmeyer


On June 14, 2018, 3:13 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67603/
> ---
> 
> (Updated June 14, 2018, 3:13 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made sure all configured shell script outputs are executable.
> 
> 
> Diffs
> -
> 
>   configure.ac f5a9d5bded40d2af6df7fe872395b076cbd37123 
> 
> 
> Diff: https://reviews.apache.org/r/67603/diff/1/
> 
> 
> Testing
> ---
> 
> * `../configure`; all shell scripts created in the build tree are executable
> * `make distcheck` for good measure, but likely would not be able to detect 
> issues here
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67632: Added definition for PICOJSON_USE_INT64 into stout headers.

[Andrew Schwartzmeyer]

> On June 18, 2018, 1:23 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/Makefile.am
> > Lines 96-99 (original), 96-97 (patched)
> > 
> >
> > We should probably remove this from CMake too, yeah?
> > 
> > ```
> > 3rdparty/CMakeLists.txt
> > 424:  PICOJSON_USE_INT64
> > ```

That said, this part:

> Previously, we relied on the Mesos build system to set
that macro from the command line, but since we don't
generate a separate pkg-config file for stout there is
no way to convey this information to other users of stout.

Isn't true for CMake since it has a notion of interfaces. But still, if we're 
defining it in the file itself, it's unneeded.


- Andrew


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


On June 18, 2018, 8:53 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67632/
> ---
> 
> (Updated June 18, 2018, 8:53 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The macro PICOJSON_USE_INT64 must always be defined
> when attempting to use stout's JSON facilities, since
> they unconditionally depend on these features.
> 
> Previously, we relied on the Mesos build system to set
> that macro from the command line, but since we don't
> generate a separate pkg-config file for stout there is
> no way to convey this information to other users of stout.
> 
> Even for users trying to use stout as part of a libmesos
> installation, it might be surprising that it is required to
> read CPPFLAGS if only the header-only stout part is to be
> used.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 5b922af6cd58dc03c44aacf88637e89f6e289702 
>   3rdparty/stout/include/stout/json.hpp 
> 5e738cf6f72f32b852beb61a16787e48082dab14 
> 
> 
> Diff: https://reviews.apache.org/r/67632/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67632: Added definition for PICOJSON_USE_INT64 into stout headers.

[Andrew Schwartzmeyer]

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




3rdparty/stout/Makefile.am
Lines 96-99 (original), 96-97 (patched)


We should probably remove this from CMake too, yeah?

```
3rdparty/CMakeLists.txt
424:  PICOJSON_USE_INT64
```


- Andrew Schwartzmeyer


On June 18, 2018, 8:53 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67632/
> ---
> 
> (Updated June 18, 2018, 8:53 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The macro PICOJSON_USE_INT64 must always be defined
> when attempting to use stout's JSON facilities, since
> they unconditionally depend on these features.
> 
> Previously, we relied on the Mesos build system to set
> that macro from the command line, but since we don't
> generate a separate pkg-config file for stout there is
> no way to convey this information to other users of stout.
> 
> Even for users trying to use stout as part of a libmesos
> installation, it might be surprising that it is required to
> read CPPFLAGS if only the header-only stout part is to be
> used.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 5b922af6cd58dc03c44aacf88637e89f6e289702 
>   3rdparty/stout/include/stout/json.hpp 
> 5e738cf6f72f32b852beb61a16787e48082dab14 
> 
> 
> Diff: https://reviews.apache.org/r/67632/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 65995: Declined unwanted offers in `RetryOperationStatusUpdate*` SLRP tests.

[Benjamin Bannier]

> On June 18, 2018, 2:08 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 2561-2562 (patched)
> > 
> >
> > If there is a reason to `settle` before we `advance`, we should add a 
> > comment, otherwise I would expect a sequence of first `advance`, then 
> > `settle`.
> > 
> > We don't seem to be very disciplined to _always_ `settle` after and 
> > `advance` in this file, we could clean that up in a follow-up.
> 
> Chun-Hung Hsiao wrote:
> Yes there's a reason to do `settle` before `advance`: we need to ensure 
> that the allocater gets the RP resouces before doing any allocation. I'll add 
> a comment here.
> 
> However, it seems to me that it needs not to always `settle` after 
> `advance`. IMO `settle` enforces many synchronizations, and sometimes 
> enforces too many for our unit tests to reveal certain schedules that might 
> expose data races.
> Instead, I'd prefer more specific synchronizations after `advance`. For 
> example,
> ```
> Clock::advance(masterFlags.allocation_interval);
> 
> AWAIT_READY(offers);
> ```
> The above `advance` is for triggering an offer, and thus waiting for the 
> offer should be sufficient.
> 
> WDYT?
> 
> Chun-Hung Hsiao wrote:
> Hmm sorry I made a mistake on the previous `settle`. Actually there's 
> already a comment there to explain why we do the `settle` first.

Makes sense, dropping.


- Benjamin


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


On June 14, 2018, 2:02 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65995/
> ---
> 
> (Updated June 14, 2018, 2:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The two SLRP tests assume that SLRP will send out a RAW resource in its
> first `UPDATE_STATE` message, and expect that the test framework would
> receive an offer containing the RAW resource in its first offer. However
> this behavior is not guaranteed and should not be relied on. This patch
> makes the tests decline unwanted offers by default so they no longer
> rely on SLRP's internal behavior.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 1afe8a8e0413ef225e952cd9cd6376e5d82774e5 
> 
> 
> Diff: https://reviews.apache.org/r/65995/diff/7/
> 
> 
> Testing
> ---
> 
> sudo make check
> Ran the two tests in repitition.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65995: Declined unwanted offers in `RetryOperationStatusUpdate*` SLRP tests.

[Chun-Hung Hsiao]

> On June 18, 2018, 12:08 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 2561-2562 (patched)
> > 
> >
> > If there is a reason to `settle` before we `advance`, we should add a 
> > comment, otherwise I would expect a sequence of first `advance`, then 
> > `settle`.
> > 
> > We don't seem to be very disciplined to _always_ `settle` after and 
> > `advance` in this file, we could clean that up in a follow-up.
> 
> Chun-Hung Hsiao wrote:
> Yes there's a reason to do `settle` before `advance`: we need to ensure 
> that the allocater gets the RP resouces before doing any allocation. I'll add 
> a comment here.
> 
> However, it seems to me that it needs not to always `settle` after 
> `advance`. IMO `settle` enforces many synchronizations, and sometimes 
> enforces too many for our unit tests to reveal certain schedules that might 
> expose data races.
> Instead, I'd prefer more specific synchronizations after `advance`. For 
> example,
> ```
> Clock::advance(masterFlags.allocation_interval);
> 
> AWAIT_READY(offers);
> ```
> The above `advance` is for triggering an offer, and thus waiting for the 
> offer should be sufficient.
> 
> WDYT?

Hmm sorry I made a mistake on the previous `settle`. Actually there's already a 
comment there to explain why we do the `settle` first.


- Chun-Hung


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


On June 14, 2018, 12:02 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65995/
> ---
> 
> (Updated June 14, 2018, 12:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The two SLRP tests assume that SLRP will send out a RAW resource in its
> first `UPDATE_STATE` message, and expect that the test framework would
> receive an offer containing the RAW resource in its first offer. However
> this behavior is not guaranteed and should not be relied on. This patch
> makes the tests decline unwanted offers by default so they no longer
> rely on SLRP's internal behavior.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 1afe8a8e0413ef225e952cd9cd6376e5d82774e5 
> 
> 
> Diff: https://reviews.apache.org/r/65995/diff/7/
> 
> 
> Testing
> ---
> 
> sudo make check
> Ran the two tests in repitition.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65995: Declined unwanted offers in `RetryOperationStatusUpdate*` SLRP tests.

[Chun-Hung Hsiao]

> On June 18, 2018, 12:08 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 2561-2562 (patched)
> > 
> >
> > If there is a reason to `settle` before we `advance`, we should add a 
> > comment, otherwise I would expect a sequence of first `advance`, then 
> > `settle`.
> > 
> > We don't seem to be very disciplined to _always_ `settle` after and 
> > `advance` in this file, we could clean that up in a follow-up.

Yes there's a reason to do `settle` before `advance`: we need to ensure that 
the allocater gets the RP resouces before doing any allocation. I'll add a 
comment here.

However, it seems to me that it needs not to always `settle` after `advance`. 
IMO `settle` enforces many synchronizations, and sometimes enforces too many 
for our unit tests to reveal certain schedules that might expose data races.
Instead, I'd prefer more specific synchronizations after `advance`. For example,
```
Clock::advance(masterFlags.allocation_interval);

AWAIT_READY(offers);
```
The above `advance` is for triggering an offer, and thus waiting for the offer 
should be sufficient.

WDYT?


- Chun-Hung


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


On June 14, 2018, 12:02 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65995/
> ---
> 
> (Updated June 14, 2018, 12:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The two SLRP tests assume that SLRP will send out a RAW resource in its
> first `UPDATE_STATE` message, and expect that the test framework would
> receive an offer containing the RAW resource in its first offer. However
> this behavior is not guaranteed and should not be relied on. This patch
> makes the tests decline unwanted offers by default so they no longer
> rely on SLRP's internal behavior.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 1afe8a8e0413ef225e952cd9cd6376e5d82774e5 
> 
> 
> Diff: https://reviews.apache.org/r/65995/diff/7/
> 
> 
> Testing
> ---
> 
> sudo make check
> Ran the two tests in repitition.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65976: Allowed profiles to be missing from `DiskProfileAdaptor`.

[Chun-Hung Hsiao]

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

(Updated June 18, 2018, 6:31 p.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, and Joseph Wu.


Changes
---

Added the checks back as suggested by Benjamin.


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


Repository: mesos


Description
---

Allowed profiles to be missing from `DiskProfileAdaptor`.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
b90a4b81838fec410a97a10ce44a811bb81c87eb 
  src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
614590ef7d1c0cc1df99b8e57c7fbd496793b5a7 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 67631: Added non-const accessor for JSON values.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On June 18, 2018, 3:52 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67631/
> ---
> 
> (Updated June 18, 2018, 3:52 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a non-const version of Value::as(),
> to enable modifying json objects without
> having to resort to the underlying boost functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/json.hpp 
> 5e738cf6f72f32b852beb61a16787e48082dab14 
> 
> 
> Diff: https://reviews.apache.org/r/67631/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67575: Changed operator API to notify subscribers on every status change.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67575 was successfully built and tested.

Reviews applied: `['67575']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67575

- Mesos Reviewbot Windows


On June 15, 2018, 12:45 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67575/
> ---
> 
> (Updated June 15, 2018, 12:45 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Vinod Kone, 
> and Zhitao Li.
> 
> 
> Bugs: MESOS-9000
> https://issues.apache.org/jira/browse/MESOS-9000
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this change, the master would only send TaskUpdated messages
> to subscribers when the latest known task state on the agent changed.
> 
> This implied that schedulers could not reliably wait for the status
> information corresponding to specific state updates (i.e. TASK_RUNNING),
> since there is no guarantee that subscribers get notified during
> the time when this status update will be included in the status field.
> 
> After this change, TaskUpdate messages are sent whenever the latest
> acknowledged state of the task changes.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 54f84120728eea7995422b9c356ed67e5b054623 
>   include/mesos/v1/master/master.proto 
> 12f019d8d0902e212d4a3706fe2310b514d0d183 
>   src/master/master.cpp 5db5a8da85f02323a5654c93ac47ec4aa7e711d2 
> 
> 
> Diff: https://reviews.apache.org/r/67575/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67634: Removed PICOJSON_USE_INT64 from Mesos build system.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67634 was successfully built and tested.

Reviews applied: `['67632', '67633', '67634']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67634

- Mesos Reviewbot Windows


On June 18, 2018, 3:53 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67634/
> ---
> 
> (Updated June 18, 2018, 3:53 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is no longer necessary, since the macro is now defined
> directly within .
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 3c1dfcc7885693cd76331b515ca8e361bd1976d0 
>   src/Makefile.am 2bcee1e0e198e6be009174570cdaa1c8b8372a71 
> 
> 
> Diff: https://reviews.apache.org/r/67634/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67634: Removed PICOJSON_USE_INT64 from Mesos build system.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On June 18, 2018, 3:53 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67634/
> ---
> 
> (Updated June 18, 2018, 3:53 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is no longer necessary, since the macro is now defined
> directly within .
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 3c1dfcc7885693cd76331b515ca8e361bd1976d0 
>   src/Makefile.am 2bcee1e0e198e6be009174570cdaa1c8b8372a71 
> 
> 
> Diff: https://reviews.apache.org/r/67634/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67631: Added non-const accessor for JSON values.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['67631']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67631

Relevant logs:

- 
[stout-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67631/logs/stout-tests-cmake-stdout.log):

```
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(365): error 
C2768: 'JSON::Value::as': illegal use of explicit template arguments (compiling 
source file D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\socket_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(365): error 
C2244: 'JSON::Value::as': unable to match function definition to an existing 
declaration (compiling source file 
D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\socket_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(365): error 
C2988: unrecognizable template declaration/definition (compiling source file 
D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\socket_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(365): error 
C2059: syntax error: '' (compiling source file 
D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\socket_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(365): error 
C2143: syntax error: missing ';' before '{' (compiling source file 
D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\socket_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(365): error 
C2447: '{': missing function header (old-style formal list?) (compiling source 
file D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\socket_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(364): error 
C2988: unrecognizable template declaration/definition (compiling source file 
D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\strerror_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(364): error 
C2143: syntax error: missing ';' before '&' (compiling source file 
D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\strerror_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(365): error 
C4430: missing type specifier - int assumed. Note: C++ does not support 
default-int (compiling source file 
D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\strerror_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(365): error 
C2768: 'JSON::Value::as': illegal use of explicit template arguments (compiling 
source file D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\strerror_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(365): error 
C2244: 'JSON::Value::as': unable to match function definition to an existing 
declaration (compiling source file 
D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\strerror_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(365): error 
C2988: unrecognizable template declaration/definition (compiling source file 
D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\strerror_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(365): error 
C2059: syntax error: '' (compiling source file 
D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\strerror_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(365): error 
C2143: syntax error: missing ';' before '{' (compiling source file 
D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\strerror_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(365): error 
C2447: '{': missing function header (old-style formal list?) (compiling source 
file D:\DCOS\mesos\mesos\3rdparty\stout\tests\os\strerror_tests.cpp) 
[D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\json.hpp(364): error 
C2988: unrecognizable template 

Review Request 67633: Removed PICOJSON_USE_INT64 from libprocess build system.

[Benno Evers]

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

Review request for mesos.


Repository: mesos


Description
---

It is no longer necessary, since the macro is now set
directly within .


Diffs
-

  3rdparty/libprocess/Makefile.am 8910416ce4313a0d70721cf1bb1d1453aaf691f9 


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


Testing
---


Thanks,

Benno Evers

Review Request 67634: Removed PICOJSON_USE_INT64 from Mesos build system.

[Benno Evers]

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

Review request for mesos.


Repository: mesos


Description
---

It is no longer needed, since the macro is now directly
defined in json.hpp.


Diffs
-

  3rdparty/CMakeLists.txt 3c1dfcc7885693cd76331b515ca8e361bd1976d0 
  src/Makefile.am 2bcee1e0e198e6be009174570cdaa1c8b8372a71 


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


Testing
---


Thanks,

Benno Evers

Review Request 67632: Added definition for PICOJSON_USE_INT64 into stout headers.

[Benno Evers]

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

Review request for mesos.


Repository: mesos


Description
---

The macro PICOJSON_USE_INT64 must always be defined
when attempting to use stout's JSON facilities, since
they unconditionally depend on these features.

Previously, we relied on the Mesos build system to set
that macro from the command line, but since we don't
generate a separate pkg-config file for stout there is
no way to convey this information to other users of stout.

Even for users trying to use stout as part of a libmesos
installation, it might be surprising that it is required to
read CPPFLAGS if only the header-only stout part is to be
used.


Diffs
-

  3rdparty/stout/Makefile.am 5b922af6cd58dc03c44aacf88637e89f6e289702 
  3rdparty/stout/include/stout/json.hpp 
5e738cf6f72f32b852beb61a16787e48082dab14 


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


Testing
---


Thanks,

Benno Evers

Review Request 67631: Added non-const accessor for JSON values.

[Benno Evers]

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

Review request for mesos.


Repository: mesos


Description
---

Added a non-const version of Value::as(),
to enable modifying json objects without
having to resort to the underlying boost functions.


Diffs
-

  3rdparty/stout/include/stout/json.hpp 
5e738cf6f72f32b852beb61a16787e48082dab14 


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


Testing
---


Thanks,

Benno Evers

Re: Review Request 65640: Fixed a race condition in `UriDiskProfileAdaptorTests`.

[Benjamin Bannier]

> On June 18, 2018, 3:38 p.m., Benjamin Bannier wrote:
> > src/Makefile.am
> > Lines 2517 (patched)
> > 
> >
> > This looks unrelated, and AFAICT not needed.
> > 
> > Let's remove this from the patch.
> 
> Chun-Hung Hsiao wrote:
> This is required for the test to use `FUTURE_DISPATCH` on 
> `UriDiskProfileAdaptorProcess::_poll` without a link error.

The symbols from that file are already in `liburi_disk_profile_adaptor` and we 
should not add them directly here (if we'd link to the library in the future 
we'd have ODR violations). Instead we should link `libmesos` or `mesos_tests` 
with that library; the cmake build links `libmesos` with 
`liburi_disk_profile_adaptor`.


- Benjamin


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


On June 14, 2018, 2:20 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65640/
> ---
> 
> (Updated June 14, 2018, 2:20 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8567
> https://issues.apache.org/jira/browse/MESOS-8567
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There was a race between `Clock::advance()` in the `FetchFromHTTP` test
> and `delay()` in `UriDiskProfileAdaptorProcess::_poll`. This patch
> avoids the race by enforcing an order between the dispatch of the
> `__poll` function (previously `_poll`) and the clock manipulation
> in the test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2bcee1e0e198e6be009174570cdaa1c8b8372a71 
>   src/resource_provider/storage/uri_disk_profile_adaptor.hpp 
> 0484933b42d0bd66c689b06cb48f492eef7bc606 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
> 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 4485f1635f484ce6e1c7c532eedb277f5eee118b 
> 
> 
> Diff: https://reviews.apache.org/r/65640/diff/5/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65640: Fixed a race condition in `UriDiskProfileAdaptorTests`.

[Chun-Hung Hsiao]

> On June 18, 2018, 1:38 p.m., Benjamin Bannier wrote:
> > src/Makefile.am
> > Lines 2517 (patched)
> > 
> >
> > This looks unrelated, and AFAICT not needed.
> > 
> > Let's remove this from the patch.

This is required for the test to use `FUTURE_DISPATCH` on 
`UriDiskProfileAdaptorProcess::_poll` without a link error.


- Chun-Hung


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


On June 14, 2018, 12:20 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65640/
> ---
> 
> (Updated June 14, 2018, 12:20 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8567
> https://issues.apache.org/jira/browse/MESOS-8567
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There was a race between `Clock::advance()` in the `FetchFromHTTP` test
> and `delay()` in `UriDiskProfileAdaptorProcess::_poll`. This patch
> avoids the race by enforcing an order between the dispatch of the
> `__poll` function (previously `_poll`) and the clock manipulation
> in the test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2bcee1e0e198e6be009174570cdaa1c8b8372a71 
>   src/resource_provider/storage/uri_disk_profile_adaptor.hpp 
> 0484933b42d0bd66c689b06cb48f492eef7bc606 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
> 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 4485f1635f484ce6e1c7c532eedb277f5eee118b 
> 
> 
> Diff: https://reviews.apache.org/r/65640/diff/5/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65640: Fixed a race condition in `UriDiskProfileAdaptorTests`.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/Makefile.am
Lines 2517 (patched)


This looks unrelated, and AFAICT not needed.

Let's remove this from the patch.



src/resource_provider/storage/uri_disk_profile_adaptor.hpp
Line 233 (original), 235 (patched)


Nit: remove this line.


- Benjamin Bannier


On June 14, 2018, 2:20 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65640/
> ---
> 
> (Updated June 14, 2018, 2:20 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8567
> https://issues.apache.org/jira/browse/MESOS-8567
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There was a race between `Clock::advance()` in the `FetchFromHTTP` test
> and `delay()` in `UriDiskProfileAdaptorProcess::_poll`. This patch
> avoids the race by enforcing an order between the dispatch of the
> `__poll` function (previously `_poll`) and the clock manipulation
> in the test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2bcee1e0e198e6be009174570cdaa1c8b8372a71 
>   src/resource_provider/storage/uri_disk_profile_adaptor.hpp 
> 0484933b42d0bd66c689b06cb48f492eef7bc606 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
> 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 4485f1635f484ce6e1c7c532eedb277f5eee118b 
> 
> 
> Diff: https://reviews.apache.org/r/65640/diff/5/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65975: Sequentialize reconciliations of storage pools in SLRP.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/resource_provider/storage/provider.cpp
Lines 1312 (patched)


We could break after `return` for readability.



src/resource_provider/storage/provider.cpp
Line 2927 (original), 2974 (patched)


We can codify this comment,

CHECK(!protobuf::isSpeculativeOperation(operation.info())) << "Operation " 
<< operation << " is speculative";


- Benjamin Bannier


On June 14, 2018, 2:06 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65975/
> ---
> 
> (Updated June 14, 2018, 2:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The storage pools needs to be reconciled in the following two scenarios:
> 
> 1. When there is a change in the set of known profiles.
> 2. When a volume/block of an unknown profile is destroyed, because the
>disk space being freed up may belong to a known profile.
> 
> This patch adds a sequence to coordinate the reconciliations for the
> above two cases.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> b90a4b81838fec410a97a10ce44a811bb81c87eb 
> 
> 
> Diff: https://reviews.apache.org/r/65975/diff/5/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65995: Declined unwanted offers in `RetryOperationStatusUpdate*` SLRP tests.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/tests/storage_local_resource_provider_tests.cpp
Lines 2561-2562 (patched)


If there is a reason to `settle` before we `advance`, we should add a 
comment, otherwise I would expect a sequence of first `advance`, then `settle`.

We don't seem to be very disciplined to _always_ `settle` after and 
`advance` in this file, we could clean that up in a follow-up.



src/tests/storage_local_resource_provider_tests.cpp
Lines 2718-2719 (patched)


Ditto.


- Benjamin Bannier


On June 14, 2018, 2:02 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65995/
> ---
> 
> (Updated June 14, 2018, 2:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The two SLRP tests assume that SLRP will send out a RAW resource in its
> first `UPDATE_STATE` message, and expect that the test framework would
> receive an offer containing the RAW resource in its first offer. However
> this behavior is not guaranteed and should not be relied on. This patch
> makes the tests decline unwanted offers by default so they no longer
> rely on SLRP's internal behavior.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 1afe8a8e0413ef225e952cd9cd6376e5d82774e5 
> 
> 
> Diff: https://reviews.apache.org/r/65995/diff/7/
> 
> 
> Testing
> ---
> 
> sudo make check
> Ran the two tests in repitition.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67606: Allow for unbundled libevent cmake builds.

[Benjamin Bannier]

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


Fix it, then Ship it!




Thanks for this Till, LGTM.

I mention some of my grievances with `find_package_helper` below. It can e.g., 
lead to inconsistent results, or ignore arguments to `find_package` which users 
likely expect to change behavior.


3rdparty/cmake/Findlibevent.cmake
Lines 1 (patched)


This file should be called `3rdparty/cmake/FindLIBEVENT.cmake` (package 
name all UPPERCASE). This is the usual convention, see e.g., cmake upstream.



3rdparty/cmake/Findlibevent.cmake
Lines 41 (patched)


Note `find_package` interprets these paths only as `HINTS`, so that e.g., 
even if `LIBEVENT_ROOT_DIR` does not exist we might still find an installation 
somewhere else in the system.

This is tech debt in `find_package`, not an issue we should address here.



3rdparty/cmake/Findlibevent.cmake
Lines 51 (patched)


I was about to suggest to use `find_package_handle_standard_args` here, but 
`find_package_helper` implements part of that in suprising ways (e.g., due to 
the way `find_package_helper` is implemented, even when calling 
`find_package(LIBEVENT)` it becomes `REQUIRED`.

I created https://issues.apache.org/jira/browse/MESOS-9005 to track this.


- Benjamin Bannier


On June 17, 2018, 10:33 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67606/
> ---
> 
> (Updated June 17, 2018, 10:33 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Joseph Wu, 
> and Jan Schlicht.
> 
> 
> Bugs: MESOS-8998
> https://issues.apache.org/jira/browse/MESOS-8998
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow for unbundled libevent cmake builds.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 3c1dfcc7885693cd76331b515ca8e361bd1976d0 
>   3rdparty/cmake/Findlibevent.cmake PRE-CREATION 
>   cmake/CompilationConfigure.cmake 2f92acb3a140faa48e3639c7000be7f43020ad7d 
>   docs/configuration/cmake.md 74abe65507d251ffb9cbae31a6fa18eb0d76e79b 
> 
> 
> Diff: https://reviews.apache.org/r/67606/diff/4/
> 
> 
> Testing
> ---
> 
> ```
> $ cmake .. -DENABLE_LIBEVENT=TRUE -DENABLE_SSL=TRUE 
> -DOPENSSL_ROOT_DIR=/usr/local/opt/openssl
> $ cmake --build . --target tests -- -j6
> 
> $ otool -L 3rdparty/libprocess/src/libprocess.dylib
> libprocess.dylib:
>   @rpath/libprocess.dylib (compatibility version 0.0.0, current version 
> 0.0.0)
>   /usr/local/opt/openssl/lib/libssl.1.0.0.dylib (compatibility version 
> 1.0.0, current version 1.0.0)
>   /usr/local/opt/openssl/lib/libcrypto.1.0.0.dylib (compatibility version 
> 1.0.0, current version 1.0.0)
>   
> /Users/till/Development/mesos/build/3rdparty/libevent-2.1.5-beta/src/libevent-2.1.5-beta-build/lib/libevent.2.1.5.dylib
>  (compatibility version 2.1.5, current version 0.0.0)
>   /usr/local/opt/apr/libexec/lib/libapr-1.0.dylib (compatibility version 
> 7.0.0, current version 7.3.0)
>   /usr/lib/libcurl.4.dylib (compatibility version 7.0.0, current version 
> 9.0.0)
>   
> /Users/till/Development/mesos/build/3rdparty/glog-0.3.3/src/glog-0.3.3-build/lib/libglog.0.dylib
>  (compatibility version 1.0.0, current version 1.0.0)
>   
> /Users/till/Development/mesos/build/3rdparty/protobuf-3.5.0/src/protobuf-3.5.0-build/libprotobuf.dylib
>  (compatibility version 0.0.0, current version 0.0.0)
>   /usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 
> 1.2.11)
>   /usr/local/opt/subversion/lib/libsvn_delta-1.0.dylib (compatibility 
> version 1.0.0, current version 1.0.0)
>   /usr/local/opt/subversion/lib/libsvn_diff-1.0.dylib (compatibility 
> version 1.0.0, current version 1.0.0)
>   /usr/local/opt/subversion/lib/libsvn_subr-1.0.dylib (compatibility 
> version 1.0.0, current version 1.0.0)
>   /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 
> 400.9.3)
>   /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current 
> version 1252.200.5)
> ```
> 
> ```
> $ cmake .. -DENABLE_LIBEVENT=TRUE -DENABLE_SSL=TRUE 
> -DOPENSSL_ROOT_DIR=/usr/local/opt/openssl -DUNBUNDLED_LIBEVENT=TRUE
> $ cmake --build . --target tests -- -j6
> 
> $ otool -L 3rdparty/libprocess/src/libprocess.dylib
> libprocess.dylib:
>   @rpath/libprocess.dylib (compatibility version 0.0.0, current version 
> 0.0.0)
>   /usr/local/opt/openssl/lib/libssl.1.0.0.dylib (compatibility version 
> 1.0.0, current version 1.0.0)
>