Re: Review Request 66293: Tested default executor support of `max_completion_time`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66481, 66258, 66591, 66259, 66260, 66283, 66284, 66291, 66293]

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 April 8, 2018, 12:54 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66293/
> ---
> 
> (Updated April 8, 2018, 12:54 a.m.)
> 
> 
> Review request for mesos, Jason Lai and James Peach.
> 
> 
> Bugs: MESOS-8725
> https://issues.apache.org/jira/browse/MESOS-8725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tested default executor support of `max_completion_time`.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 293dd20d882447401572835bd31e197faf76861b 
> 
> 
> Diff: https://reviews.apache.org/r/66293/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66230: Added test for adding/removing framework roles.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66230 was successfully built and tested.

Reviews applied: `['66228', '66229', '66230']`

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

- Mesos Reviewbot Windows


On April 13, 2018, 1:38 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66230/
> ---
> 
> (Updated April 13, 2018, 1:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for adding/removing framework roles.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_tests.cpp 749420a23bc1a3873bd4d5aee56e78cff79bb1af 
> 
> 
> Diff: https://reviews.apache.org/r/66230/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.

[Kapil Arya]

> On April 12, 2018, 8:19 p.m., Till Toenshoff wrote:
> > src/master/validation.cpp
> > Lines 532 (patched)
> > 
> >
> > s/is/are/
> > 
> > How do we make sure the role is "valid" - isn't that implicit when it 
> > was contained in the framework roles?

Updated the comment to reflect that we are validating "suppressed" roles.


- Kapil


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


On April 12, 2018, 9:38 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66229/
> ---
> 
> (Updated April 12, 2018, 9:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
>   src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 
>   src/master/metrics.cpp 894c04173d218ffd891d203254035fe4756cc8bb 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 
> 
> 
> Diff: https://reviews.apache.org/r/66229/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.

[Kapil Arya]

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

(Updated April 12, 2018, 9:38 p.m.)


Review request for mesos, Benjamin Mahler and Till Toenshoff.


Changes
---

Addressed Till's comments.


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


Repository: mesos


Description
---

Implemented UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
  src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
  src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
  src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 
  src/master/metrics.cpp 894c04173d218ffd891d203254035fe4756cc8bb 
  src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
  src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 


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

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


Testing
---


Thanks,

Kapil Arya

Re: Review Request 66230: Added test for adding/removing framework roles.

[Kapil Arya]

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

(Updated April 12, 2018, 9:38 p.m.)


Review request for mesos, Benjamin Mahler and Till Toenshoff.


Changes
---

Addressed Till's comments.


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


Repository: mesos


Description
---

Added test for adding/removing framework roles.


Diffs (updated)
-

  src/tests/scheduler_tests.cpp 749420a23bc1a3873bd4d5aee56e78cff79bb1af 


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

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


Testing
---


Thanks,

Kapil Arya

Re: Review Request 66577: Enabled CSI proto compilation by default.

[Chun-Hung Hsiao]

> On April 12, 2018, 4:44 p.m., Andrew Schwartzmeyer wrote:
> > src/CMakeLists.txt
> > Lines 32-35 (patched)
> > 
> >
> > Nit: can this be moved down so it goes "no options", "grpc option", 
> > "java option", "internal option."

I put it in this order because this is from the 3rdparty bundle and (although 
there is no public Mesos proto that uses CSI for now) this opens the possiblity 
to let a public Mesos proto to use CSI.


- Chun-Hung


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


On April 12, 2018, 1:39 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66577/
> ---
> 
> (Updated April 12, 2018, 1:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.
> 
> 
> Bugs: MESOS-8724
> https://issues.apache.org/jira/browse/MESOS-8724
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CSI proto compilation is disabled due to MESOS-8724, which is resolved
> by bumping CSI to v0.2. This patch enables the compilation again.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
>   src/Makefile.am 257ff0e8d21e5240c27a123ed0cd563214e24fce 
>   src/slave/flags.cpp bdfc49a3903899b2741bb60c7e9e89f0196492e4 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66577/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66418: Handled failed publish and unpublish CSI calls properly.

[Chun-Hung Hsiao]

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

(Updated April 13, 2018, 1:24 a.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

Previously, when a `ControllerPublishVolume` or a `NodePublishVolume`
fails, SLRP assumes the volume states remain unchanged, which are not
suggested by the CSI spec. This patch handles such failures properly.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
40544e08ab512041640f0f9036f841dbc55e78ca 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Chun-Hung Hsiao]

> On April 13, 2018, 12:27 a.m., Greg Mann wrote:
> > include/mesos/mesos.proto
> > Lines 1975 (patched)
> > 
> >
> > Hmm I wonder if we should just use a `double` here? Does the 
> > `Value.Scalar` type provide some benefit?

>From the API perspective, `double` is easier to use, but `Value.Scalar` is 
>more consistent with how we represesnt a disk resource.

>From the implementation perspective, I prefer `Value.Scalar` because it's 
>arithmetic operators will do fixed-point conversions for me so I won't forget 
>that.


- Chun-Hung


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


On March 28, 2018, 6:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 6:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Chun-Hung Hsiao]

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




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


Let's remove the two blank lines. Ditto in `v1/mesos.proto`.


- Chun-Hung Hsiao


On March 28, 2018, 6:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 6:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 53267: Added a gauge to track active subscribers.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [53267]

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 April 12, 2018, 9:33 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53267/
> ---
> 
> (Updated April 12, 2018, 9:33 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jason Lai, and James Peach.
> 
> 
> Bugs: MESOS-6499
> https://issues.apache.org/jira/browse/MESOS-6499
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a gauge to track active subscribers.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 
>   src/master/metrics.cpp 894c04173d218ffd891d203254035fe4756cc8bb 
> 
> 
> Diff: https://reviews.apache.org/r/53267/diff/2/
> 
> 
> Testing
> ---
> 
> Ran this on master with a client keeps subscribing and disconnecting. 
> Observes that gauge gets updated and new log line is printed.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66050: Implemented grow and shrink of persistent volumes.

[Greg Mann]

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




src/master/master.cpp
Lines 4949 (patched)


Nit:
s/in operator API/in the operator API/

Here and elsewhere.



src/master/master.cpp
Lines 4986-4994 (patched)


What will happen if these operations are sent to a 1.5 agent which has the 
RESOURCE_PROVIDER capability, but does not support the operation?



src/master/master.cpp
Lines 5029 (patched)


Suggestion:
s/with subtract/subtracting/



src/master/master.cpp
Lines 5331-5334 (original), 5467-5472 (patched)


This change doesn't really belong in this patch. Also, if we're going to 
update this location for consistency, then we should update other similar 
occurrences in this function as well (like DESTROY_VOLUME below).

I'd recommend either following up with a separate cleanup patch, or leaving 
this as-is.



src/master/validation.cpp
Lines 2344 (patched)


Are you sure this will definitely result in a scalar value of zero? I think 
protobuf will default-initialize the double in the `value` field, which leaves 
it in an indeterminate state?

I think we should do `zero.set_value(0);` before the comparison is 
performed.

Ditto for the shrink_volume validator below.



src/master/validation.cpp
Lines 2346 (patched)


Suggestion:
"The size of the 'addition' volume in 'grow_volume' must be greater than 
zero"

If you end up enclosing 'grow_volume' in quotes here, then I would do it in 
other error messages in these functions as well.



src/master/validation.cpp
Lines 2355 (patched)


s/Not a/Invalid/

Here and below.



src/master/validation.cpp
Lines 2359 (patched)


Nit:
s/Growing/Growing a/



src/master/validation.cpp
Lines 2392 (patched)


s/cannot be zero/must be greater than zero/



src/master/validation.cpp
Lines 2397 (patched)


Suggestion:
"The value of 'subtract' in 'shrink_volume' must be smaller than the 
persistent volume's size"

If you end up enclosing 'shrink_volume' in quotes here, then I would do it 
in other error messages as well.



src/master/validation.cpp
Lines 2412 (patched)


Nit:
s/Shrinking/Shrinking a/


- Greg Mann


On April 10, 2018, 4:18 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated April 10, 2018, 4:18 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new offer operations are implemented as speculative operations, but
> we will use validation to make them non-speculative on API level so that
> we can transition later without a breaking change.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 
>   src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
>   src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 
> 
> 
> Diff: https://reviews.apache.org/r/66050/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Greg Mann]

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




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


Hmm I wonder if we should just use a `double` here? Does the `Value.Scalar` 
type provide some benefit?


- Greg Mann


On March 28, 2018, 6:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 6:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.

[Till Toenshoff]

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



Looks great Kapil - just some minor comments on logging and style.


src/master/master.hpp
Lines 584 (patched)


Great, I can actually use this right away with some of my work as well - 
thanks!



src/master/master.cpp
Lines 3440 (patched)


Which overload are we clashing with here? Or what is the reasoning?

Do you really need the `Master` namespace reference here?



src/master/master.cpp
Lines 3441 (patched)


Leave the parameter name out for that one, just like the others.



src/master/master.cpp
Lines 3481 (patched)


If you used `*framework` instead of `frameworkInfo.name()`, the resulting 
logging could be even more helpful.



src/master/master.cpp
Lines 3490 (patched)


See above and remove the leading tick here.



src/master/validation.hpp
Lines 117 (patched)


s/Valides/Validates/



src/master/validation.cpp
Lines 509 (patched)


s/frameworkInfos/'FrameworkInfo's/ ?



src/master/validation.cpp
Lines 526 (patched)


s/newFrameworkInfo.name()/*framework/



src/master/validation.cpp
Lines 532 (patched)


s/is/are/

How do we make sure the role is "valid" - isn't that implicit when it was 
contained in the framework roles?



src/master/validation.cpp
Lines 538 (patched)


s/suppression/Suppression/



src/master/validation.cpp
Line 538 (original), 597 (patched)


Thanks, that's better.



src/master/validation.cpp
Lines 712 (patched)


I really like this new, early specific validation.


- Till Toenshoff


On April 11, 2018, 5:28 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66229/
> ---
> 
> (Updated April 11, 2018, 5:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
>   src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 
>   src/master/metrics.cpp 894c04173d218ffd891d203254035fe4756cc8bb 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 
> 
> 
> Diff: https://reviews.apache.org/r/66229/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 65974: Added comments and made some renaming in SLRP.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On April 12, 2018, 3:33 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65974/
> ---
> 
> (Updated April 12, 2018, 3:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added comments and made some renaming in SLRP.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> a07620d1c4bf618f669575b3e183bf598da905a6 
> 
> 
> Diff: https://reviews.apache.org/r/65974/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66576: Added `STAGE_UNSTAGE_VOLUME` capability to the test CSI plugin.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On April 12, 2018, 1:36 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66576/
> ---
> 
> (Updated April 12, 2018, 1:36 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8777
> https://issues.apache.org/jira/browse/MESOS-8777
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now it is required to call `NodeStageVolume` before `NodePublishVolume`
> for the test CSI plugin. `NodeStageVolume` would bind-mount the volume
> directory to the specified staging path, and then `NodePublishVolume`
> would bind-mount the staging path to the target path.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp aed22623b6dadb791eeaac2ecde4c31236a5fc19 
> 
> 
> Diff: https://reviews.apache.org/r/66576/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66575: Supported `STAGE_UNSTAGE_VOLUME` CSI node capability in SLRP.

[Jie Yu]

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


Fix it, then Ship it!





src/resource_provider/storage/provider.cpp
Lines 2237-2244 (patched)


Ditto. Let's move to the caller.


- Jie Yu


On April 12, 2018, 1:35 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66575/
> ---
> 
> (Updated April 12, 2018, 1:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8777
> https://issues.apache.org/jira/browse/MESOS-8777
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The storage local resource provider now properly calls `NodeStageVolume`
> or `NodeUnstageVolume` when publishing or deleting volumes for CSI
> plugins support the `STAGE_UNSTAGE_VOLUME` capability.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 0373c8ad217998a54b06d483f94052bcf4293677 
>   src/resource_provider/storage/provider.cpp 
> a07620d1c4bf618f669575b3e183bf598da905a6 
> 
> 
> Diff: https://reviews.apache.org/r/66575/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> `STAGE_UNSTAGE_VOLUME` is tested later in the chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 53267: Added a gauge to track active subscribers.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 53267 was successfully built and tested.

Reviews applied: `['53267']`

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

- Mesos Reviewbot Windows


On April 12, 2018, 9:33 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53267/
> ---
> 
> (Updated April 12, 2018, 9:33 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jason Lai, and James Peach.
> 
> 
> Bugs: MESOS-6499
> https://issues.apache.org/jira/browse/MESOS-6499
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a gauge to track active subscribers.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 
>   src/master/metrics.cpp 894c04173d218ffd891d203254035fe4756cc8bb 
> 
> 
> Diff: https://reviews.apache.org/r/53267/diff/2/
> 
> 
> Testing
> ---
> 
> Ran this on master with a client keeps subscribing and disconnecting. 
> Observes that gauge gets updated and new log line is printed.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66574: Updated filesystem layout for staging and mounting CSI volumes.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On April 12, 2018, 1:33 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66574/
> ---
> 
> (Updated April 12, 2018, 1:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8777
> https://issues.apache.org/jira/browse/MESOS-8777
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To support the `STAGE_UNSTAGE_VOLUME` in CSI v0.2, a CSI volume is now
> staged at `/csi///mounts//staging` and
> mounted at `/csi///mounts//target`.
> 
> The `DiskInfo.Source.Path.root` and `DiskInfo.Source.Mount.root` fields
> now stores `./csi///mounts` for PATH and MOUNT disk
> resources respectively, and the actual mount point is carved out from
> the `root` and `id` fields. In the future, the `SharedInfo` might also
> be used to construct the mount point for shared volumes.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
>   src/Makefile.am 257ff0e8d21e5240c27a123ed0cd563214e24fce 
>   src/csi/paths.hpp 7892e5ed8d333be5e61e2148860789786db31925 
>   src/csi/paths.cpp 0d42db249c8c27b1089e91e8d068fb7fa81a46b3 
>   src/resource_provider/storage/provider.cpp 
> a07620d1c4bf618f669575b3e183bf598da905a6 
>   src/slave/paths.cpp 8a7e162ad41f5e7cc73eda660c6adc365a73edba 
> 
> 
> Diff: https://reviews.apache.org/r/66574/diff/1/
> 
> 
> Testing
> ---
> 
> This patch cannot be compiled standalone. Tests done later in the chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66418: Handled failed publish and unpublish CSI calls properly.

[Jie Yu]

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


Fix it, then Ship it!





src/resource_provider/storage/provider.cpp
Line 2076 (original), 2076-2083 (patched)


Discussed offline. Let's move this to the caller and make this function a 
pure helper doing controller publish.



src/resource_provider/storage/provider.cpp
Lines 2194-2196 (original), 2190-2197 (patched)


Ditto.


- Jie Yu


On April 11, 2018, 2:37 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66418/
> ---
> 
> (Updated April 11, 2018, 2:37 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8717
> https://issues.apache.org/jira/browse/MESOS-8717
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, when a `ControllerPublishVolume` or a `NodePublishVolume`
> fails, SLRP assumes the volume states remain unchanged, which are not
> suggested by the CSI spec. This patch handles such failures properly.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> a07620d1c4bf618f669575b3e183bf598da905a6 
> 
> 
> Diff: https://reviews.apache.org/r/66418/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66293: Tested default executor support of `max_completion_time`.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['66258', '66591', '66259', '66260', '66283', '66284', 
'66291', '66293']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3500):
 warning C4996: 'inet_ntoa': Use inet_ntop() or InetNtop() instead or define 
_WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3501):
 warning C4996: 'sprintf': This function or variable may be unsafe. Consider 
using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3479):
 warning C4101: 'addrstr': unreferenced local variable 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\recordio.c(170):
 warning C4267: '=': conversion from 'size_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496):
 warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256):
 warning C4090: 'function': different 'const' qualifiers 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

> On March 29, 2018, 5:07 a.m., Benjamin Bannier wrote:
> > include/mesos/mesos.proto
> > Lines 1920-1921 (patched)
> > 
> >
> > These enum values are not handled in a number of switch statements in 
> > the code so that this patch cannot be used in isolation (fatal compiler 
> > warnings). It might be enough to enumerate the new cases explicitly and 
> > mark them `UNIMPLEMENTED`.
> > 
> > Similar for v1 enum below.
> 
> Zhitao Li wrote:
> My plan was to commit all patches leading to proper full implementation 
> in one shot so code on master is still compilable (which seems previous norm 
> to me?)
> 
> If you think marking them as `UNIMPLEMENTED` then gradually implement 
> each in later patches is better, I can happily do that.
> 
> Greg Mann wrote:
> We generally try to make each patch self-contained, so that the codebase 
> will still compile and tests will still pass if that patch were applied 
> alone. We do sometimes break from this when it's very difficult to 
> accomplish. I think that Benjamin's comment here is valid; I'll leave it up 
> to you what you want to do with this patch, but in the future, try to make 
> patches self-contained when possible.

That's fair. I like your proposal to keep patches self-contained if possible 
(just weren't clear on this practice).

I'll add `UNIMPLEMENTED();` to broken enum checks in this patch.


- Zhitao


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


On March 28, 2018, 11:24 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 11:24 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Chun-Hung Hsiao]

> On March 29, 2018, 12:07 p.m., Benjamin Bannier wrote:
> > include/mesos/mesos.proto
> > Lines 1920-1921 (patched)
> > 
> >
> > These enum values are not handled in a number of switch statements in 
> > the code so that this patch cannot be used in isolation (fatal compiler 
> > warnings). It might be enough to enumerate the new cases explicitly and 
> > mark them `UNIMPLEMENTED`.
> > 
> > Similar for v1 enum below.
> 
> Zhitao Li wrote:
> My plan was to commit all patches leading to proper full implementation 
> in one shot so code on master is still compilable (which seems previous norm 
> to me?)
> 
> If you think marking them as `UNIMPLEMENTED` then gradually implement 
> each in later patches is better, I can happily do that.
> 
> Greg Mann wrote:
> We generally try to make each patch self-contained, so that the codebase 
> will still compile and tests will still pass if that patch were applied 
> alone. We do sometimes break from this when it's very difficult to 
> accomplish. I think that Benjamin's comment here is valid; I'll leave it up 
> to you what you want to do with this patch, but in the future, try to make 
> patches self-contained when possible.
> 
> Zhitao Li wrote:
> That's fair. I like your proposal to keep patches self-contained if 
> possible (just weren't clear on this practice).
> 
> I'll add `UNIMPLEMENTED();` to broken enum checks in this patch.

If this patch cannot be built standalone, please leave a note in the "Testing 
Done" text box to remind us to push this commit along with the following 
patches :)


- Chun-Hung


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


On March 28, 2018, 6:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 6:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 53267: Added a gauge to track active subscribers.

[Zhitao Li]

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

(Updated April 12, 2018, 2:32 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar.


Changes
---

Rebase and updated summary.


Summary (updated)
-

Added a gauge to track active subscribers.


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


Repository: mesos


Description (updated)
---

Added a gauge to track active subscribers.


Diffs (updated)
-

  src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
  src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 
  src/master/metrics.cpp 894c04173d218ffd891d203254035fe4756cc8bb 


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

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


Testing
---

Ran this on master with a client keeps subscribing and disconnecting. Observes 
that gauge gets updated and new log line is printed.


Thanks,

Zhitao Li

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Greg Mann]

> On March 29, 2018, 12:07 p.m., Benjamin Bannier wrote:
> > include/mesos/mesos.proto
> > Lines 1920-1921 (patched)
> > 
> >
> > These enum values are not handled in a number of switch statements in 
> > the code so that this patch cannot be used in isolation (fatal compiler 
> > warnings). It might be enough to enumerate the new cases explicitly and 
> > mark them `UNIMPLEMENTED`.
> > 
> > Similar for v1 enum below.
> 
> Zhitao Li wrote:
> My plan was to commit all patches leading to proper full implementation 
> in one shot so code on master is still compilable (which seems previous norm 
> to me?)
> 
> If you think marking them as `UNIMPLEMENTED` then gradually implement 
> each in later patches is better, I can happily do that.

We generally try to make each patch self-contained, so that the codebase will 
still compile and tests will still pass if that patch were applied alone. We do 
sometimes break from this when it's very difficult to accomplish. I think that 
Benjamin's comment here is valid; I'll leave it up to you what you want to do 
with this patch, but in the future, try to make patches self-contained when 
possible.


- Greg


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


On March 28, 2018, 6:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 6:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Review Request 66591: Added a validation that `max_completion_time` must be non-negative.

[Zhitao Li]

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

Review request for mesos, Jason Lai and James Peach.


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


Repository: mesos


Description
---

Added a validation that `max_completion_time` must be non-negative.


Diffs
-

  src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
  src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 


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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66218: Ensured that agent does not delete volume upon grow or shrink.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On March 28, 2018, 6:25 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66218/
> ---
> 
> (Updated March 28, 2018, 6:25 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `slave::syncCheckpointedResources` implementation will
> delete a persistent volume using `Resources::contains` check, which
> could cause a resized volume being deleted. The function was rewritten
> to compare `set_difference` between old and new paths for all persistent
> volumes and perform creation/deletion accordingly.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66218/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 65666: Added a unit test for SLRP operation state metrics.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [65666, 65665, 65640, 65976, 65975, 65594, 65974, 65995, 
66163, 61118, 61096, 66577, 66576, 66575, 66574, 66418, 66411, 66410, 66409, 
66408, 66407, 66398]

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

Error:
2018-04-12 20:45:19 URL:https://reviews.apache.org/r/66398/diff/raw/ 
[1889/1889] -> "66398.patch" [1]
error: missing binary patch data for '3rdparty/csi-0.2.0.tar.gz'
error: binary patch does not apply to '3rdparty/csi-0.2.0.tar.gz'
error: 3rdparty/csi-0.2.0.tar.gz: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/22172/console

- Mesos Reviewbot


On March 20, 2018, 3:27 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65666/
> ---
> 
> (Updated March 20, 2018, 3:27 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8383
> https://issues.apache.org/jira/browse/MESOS-8383
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the `ROOT_OperationStateMetrics` test that issues a
> `CREATE_VOLUME` followed by two `DESTROY_VOLUME`s. The first one will
> fail due to an out-of-band deletion of the actual volume, and the second
> one will fail due to modifying the resource version.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 264d42b57fe1a8ea04c1de0a10112878c7b45d39 
> 
> 
> Diff: https://reviews.apache.org/r/65666/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66493: Made FreeBSD default to non-GNU ld.

[Benjamin Bannier]

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


Fix it, then Ship it!





cmake/CompilationConfigure.cmake
Lines 308 (patched)


Drop _on_.


- Benjamin Bannier


On April 12, 2018, 7:13 a.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66493/
> ---
> 
> (Updated April 12, 2018, 7:13 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
> Bugs: MESOS-8761
> https://issues.apache.org/jira/browse/MESOS-8761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made FreeBSD default to non-GNU ld.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 
> 
> 
> Diff: https://reviews.apache.org/r/66493/diff/3/
> 
> 
> Testing
> ---
> 
> make on FreeBSD, with both lld and gold.
> 
> 
> Thanks,
> 
> David Forsythe
> 
>

Re: Review Request 66410: Adapted storage local resource provider to use CSI v0.2.

[Chun-Hung Hsiao]

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

(Updated April 12, 2018, 7:07 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed Jie's comment.


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


Repository: mesos


Description
---

This patch contains necessary changes for the storage local resource
provider to use CSI v0.2. Support for the `STAGE_UNSTAGE_VOLUME` CSI
node service capability is not implemented in this patch yet.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
a07620d1c4bf618f669575b3e183bf598da905a6 


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

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


Testing
---

This patch cannot be compiled standalone. Tests done later in the chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 66408: Updated CSI helpers for v0.2.

[Chun-Hung Hsiao]

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

(Updated April 12, 2018, 6:54 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed Jie's comment.


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


Repository: mesos


Description
---

This patch adds new helper classes for CSI plugin and node capabilities,
removed helpers for the removed csi.Version proto message, and makes
the VolumeState proto message uses csi::v0::VolumeCapability.

NOTE: This is not future-proof if there is a breaking change in
VolumeCapability, we might need to change VolumeState to support
multiple CSI versions in the future.


Diffs (updated)
-

  src/csi/state.hpp dcbc7aba84e0e66dd0e6caf7a64fb95aae69de0a 
  src/csi/state.proto 0373c8ad217998a54b06d483f94052bcf4293677 
  src/csi/utils.hpp 58b071d4f1512e4cf10d077bd1e03e66626f6dff 
  src/csi/utils.cpp 9e0435762a16f2995bc5a04ec342af6ea231054a 


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

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


Testing
---

This patch cannot be compiled standalone. Tests done later in the chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 65665: Added operation state metrics in SLRP.

[Chun-Hung Hsiao]

> On March 21, 2018, 1:12 p.m., Jan Schlicht wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 2713 (patched)
> > 
> >
> > `statusUpdateManager.update` below might still fail to drop the 
> > message. In that case we would count the operation as dropped in metrics 
> > when we increment here. Increment in an `onReady` handler instead.
> 
> Chun-Hung Hsiao wrote:
> If the `update()` call fails, the SLRP will immediately die anyway, so at 
> least there will be no double counting. But your suggestion makes sense so 
> I'll do the change, and also change where we increment the 
> `operations_finished` or `operations_failed` metrics.

Got a question. We update the latest status of the operations from PENDING to 
FINISHED/FAILED/DROPPED and checkpoint it before passing the status to the 
status update manager. If we increment the metrics after the status update 
manager returns ready, then if an operation that becomes terminal but the 
status update manager fails to send the update, this operation won't be counted 
in any of the metrics.

So I prefer making the metrics reflect the view of SLRP not the status update 
manager. Thoughts?


- Chun-Hung


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


On April 12, 2018, 5:15 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65665/
> ---
> 
> (Updated April 12, 2018, 5:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8383
> https://issues.apache.org/jira/browse/MESOS-8383
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds `operations_pending`, `operations_finished`,
> `operations_failed`, `operations_error` (currently unused), and
> `operations_dropped` metrics to count the occurances of these operation
> states.
> 
> Additionally, An error log in `_applyOperation()` is removed because the
> error is already logged at the call site.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> a07620d1c4bf618f669575b3e183bf598da905a6 
> 
> 
> Diff: https://reviews.apache.org/r/65665/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> A unit test is added in the next patch in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65666: Added a unit test for SLRP operation state metrics.

[Chun-Hung Hsiao]

> On March 26, 2018, 2:27 p.m., Jan Schlicht wrote:
> > This is missing a test for `operations_pending`. You could wait with a 
> > `DROP_PROTOBUF(UpdateOperationStatusMessage)` after applying an operation, 
> > then check that `operations_pending` is "1", then (manually) send the 
> > `UpdateOperationStatusMessage` and check the `operation_pending` is back to 
> > "0".

Hmm seems I'll have to stop the clock for this test.


- Chun-Hung


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


On March 20, 2018, 3:27 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65666/
> ---
> 
> (Updated March 20, 2018, 3:27 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8383
> https://issues.apache.org/jira/browse/MESOS-8383
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the `ROOT_OperationStateMetrics` test that issues a
> `CREATE_VOLUME` followed by two `DESTROY_VOLUME`s. The first one will
> fail due to an out-of-band deletion of the actual volume, and the second
> one will fail due to modifying the resource version.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 264d42b57fe1a8ea04c1de0a10112878c7b45d39 
> 
> 
> Diff: https://reviews.apache.org/r/65666/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65665: Added operation state metrics in SLRP.

[Benjamin Bannier]

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



Haven't managed to make a full review of the chain, yet, one small technicality 
noted below.


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


Since this is an internal message, we should be able to enumerate the full 
set of possible values.

With that we should remove this `default` case, turning the runtime into a 
compile time error.


- Benjamin Bannier


On April 12, 2018, 7:15 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65665/
> ---
> 
> (Updated April 12, 2018, 7:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8383
> https://issues.apache.org/jira/browse/MESOS-8383
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds `operations_pending`, `operations_finished`,
> `operations_failed`, `operations_error` (currently unused), and
> `operations_dropped` metrics to count the occurances of these operation
> states.
> 
> Additionally, An error log in `_applyOperation()` is removed because the
> error is already logged at the call site.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> a07620d1c4bf618f669575b3e183bf598da905a6 
> 
> 
> Diff: https://reviews.apache.org/r/65665/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> A unit test is added in the next patch in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66408: Updated CSI helpers for v0.2.

[Chun-Hung Hsiao]

> On April 12, 2018, 5:47 p.m., Jie Yu wrote:
> > src/csi/state.proto
> > Line 45 (original), 45 (patched)
> > 
> >
> > Please s/VolumeInfo/Volume/ here
> > 
> > ALso, a git grep shows some other references to VolumeInfo:
> > ```
> > Jies-MacBook-Pro:mesos jie$ git grep VolumeInfo
> > src/csi/state.proto:  // match the attributes of the `VolumeInfo` 
> > returned by
> > src/examples/test_csi_plugin.cpp:csi::VolumeInfo* info = 
> > response->add_entries()->mutable_volume_info();
> > src/resource_provider/storage/provider.cpp:  const 
> > csi::VolumeInfo& volumeInfo = response.volume_info();
> > src/tests/master_validation_tests.cpp:TEST_F(ResourceValidationTest, 
> > PersistentVolumeWithoutVolumeInfo)
> > ```

Will fix this comment. The `VolumeInfo` in the other files are fixed down in 
the chain.


- Chun-Hung


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


On April 4, 2018, 3:59 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66408/
> ---
> 
> (Updated April 4, 2018, 3:59 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8717
> https://issues.apache.org/jira/browse/MESOS-8717
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds new helper classes for CSI plugin and node capabilities,
> removed helpers for the removed csi.Version proto message, and makes
> the VolumeState proto message uses csi::v0::VolumeCapability.
> 
> NOTE: This is not future-proof if there is a breaking change in
> VolumeCapability, we might need to change VolumeState to support
> multiple CSI versions in the future.
> 
> 
> Diffs
> -
> 
>   src/csi/state.hpp dcbc7aba84e0e66dd0e6caf7a64fb95aae69de0a 
>   src/csi/state.proto 0373c8ad217998a54b06d483f94052bcf4293677 
>   src/csi/utils.hpp 58b071d4f1512e4cf10d077bd1e03e66626f6dff 
>   src/csi/utils.cpp 9e0435762a16f2995bc5a04ec342af6ea231054a 
> 
> 
> Diff: https://reviews.apache.org/r/66408/diff/2/
> 
> 
> Testing
> ---
> 
> This patch cannot be compiled standalone. Tests done later in the chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66411: Made the test CSI plugin compatible to CSI v0.2.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On April 11, 2018, 2:36 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66411/
> ---
> 
> (Updated April 11, 2018, 2:36 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8717
> https://issues.apache.org/jira/browse/MESOS-8717
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch contains necessary changes for the test CSI plugin to support
> CSI v0.2. The `STAGE_UNSTAGE_VOLUME` node service capability is not
> implemented in this patch yet.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp aed22623b6dadb791eeaac2ecde4c31236a5fc19 
> 
> 
> Diff: https://reviews.apache.org/r/66411/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65666: Added a unit test for SLRP operation state metrics.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 66398.

Failed command: `python.exe .\support\apply-reviews.py -n -r 66398`

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

Relevant logs:

- 
[apply-review-66398-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65666/logs/apply-review-66398-stdout.log):

```
error: missing binary patch data for '3rdparty/csi-0.2.0.tar.gz'
error: binary patch does not apply to '3rdparty/csi-0.2.0.tar.gz'
error: 3rdparty/csi-0.2.0.tar.gz: patch does not apply
```

- Mesos Reviewbot Windows


On March 20, 2018, 3:27 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65666/
> ---
> 
> (Updated March 20, 2018, 3:27 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8383
> https://issues.apache.org/jira/browse/MESOS-8383
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the `ROOT_OperationStateMetrics` test that issues a
> `CREATE_VOLUME` followed by two `DESTROY_VOLUME`s. The first one will
> fail due to an out-of-band deletion of the actual volume, and the second
> one will fail due to modifying the resource version.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 264d42b57fe1a8ea04c1de0a10112878c7b45d39 
> 
> 
> Diff: https://reviews.apache.org/r/65666/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66408: Updated CSI helpers for v0.2.

[Jie Yu]

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




src/csi/state.proto
Line 45 (original), 45 (patched)


Please s/VolumeInfo/Volume/ here

ALso, a git grep shows some other references to VolumeInfo:
```
Jies-MacBook-Pro:mesos jie$ git grep VolumeInfo
src/csi/state.proto:  // match the attributes of the `VolumeInfo` returned 
by
src/examples/test_csi_plugin.cpp:csi::VolumeInfo* info = 
response->add_entries()->mutable_volume_info();
src/resource_provider/storage/provider.cpp:  const csi::VolumeInfo& 
volumeInfo = response.volume_info();
src/tests/master_validation_tests.cpp:TEST_F(ResourceValidationTest, 
PersistentVolumeWithoutVolumeInfo)
```


- Jie Yu


On April 4, 2018, 3:59 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66408/
> ---
> 
> (Updated April 4, 2018, 3:59 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8717
> https://issues.apache.org/jira/browse/MESOS-8717
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds new helper classes for CSI plugin and node capabilities,
> removed helpers for the removed csi.Version proto message, and makes
> the VolumeState proto message uses csi::v0::VolumeCapability.
> 
> NOTE: This is not future-proof if there is a breaking change in
> VolumeCapability, we might need to change VolumeState to support
> multiple CSI versions in the future.
> 
> 
> Diffs
> -
> 
>   src/csi/state.hpp dcbc7aba84e0e66dd0e6caf7a64fb95aae69de0a 
>   src/csi/state.proto 0373c8ad217998a54b06d483f94052bcf4293677 
>   src/csi/utils.hpp 58b071d4f1512e4cf10d077bd1e03e66626f6dff 
>   src/csi/utils.cpp 9e0435762a16f2995bc5a04ec342af6ea231054a 
> 
> 
> Diff: https://reviews.apache.org/r/66408/diff/2/
> 
> 
> Testing
> ---
> 
> This patch cannot be compiled standalone. Tests done later in the chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65666: Added a unit test for SLRP operation state metrics.

[Chun-Hung Hsiao]

> On March 26, 2018, 2:27 p.m., Jan Schlicht wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 3127 (patched)
> > 
> >
> > We should wait for an `UpdateSlaveMessage` here. Otherwise we can't be 
> > sure that an offer will contain resource provider resources.
> > I see that you're using the `OffersHaveAnyResources` matcher to work 
> > around this. The test could be much stricter when using 
> > `UpdateSlaveMessage` instead as we wouldn't expect offers without RP 
> > resources after receiving this message, probably even run the test with a 
> > stopped clock.

Yeah that's true. However if we wait for the `UpdateSlaveMessage` or even 
stopping the clock, the we'll need to have logic like the following: 
https://github.com/apache/mesos/blob/master/src/tests/storage_local_resource_provider_tests.cpp#L296-L327
which reveals implementation details about SLRP sending multiple 
`UpdateSlaveMessage`s and their orders. Since these details are not very 
relevent to the test, I'd prefer using the current style which mimics how a 
framework would use SLRP, unless it is necessary to control the clock.


- Chun-Hung


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


On March 20, 2018, 3:27 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65666/
> ---
> 
> (Updated March 20, 2018, 3:27 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8383
> https://issues.apache.org/jira/browse/MESOS-8383
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the `ROOT_OperationStateMetrics` test that issues a
> `CREATE_VOLUME` followed by two `DESTROY_VOLUME`s. The first one will
> fail due to an out-of-band deletion of the actual volume, and the second
> one will fail due to modifying the resource version.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 264d42b57fe1a8ea04c1de0a10112878c7b45d39 
> 
> 
> Diff: https://reviews.apache.org/r/65666/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66410: Adapted storage local resource provider to use CSI v0.2.

[Jie Yu]

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


Fix it, then Ship it!





src/resource_provider/storage/provider.cpp
Line 726 (original), 724 (patched)


Using `ContainerID()` here is a bit weird.

Can you do the following? `Option` defines `==` for `T` as the argument.
```
if (nodeContainerId == containerId ||
controllerContainerId == containerId)
```


- Jie Yu


On April 11, 2018, 2:34 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66410/
> ---
> 
> (Updated April 11, 2018, 2:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8717
> https://issues.apache.org/jira/browse/MESOS-8717
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch contains necessary changes for the storage local resource
> provider to use CSI v0.2. Support for the `STAGE_UNSTAGE_VOLUME` CSI
> node service capability is not implemented in this patch yet.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> a07620d1c4bf618f669575b3e183bf598da905a6 
> 
> 
> Diff: https://reviews.apache.org/r/66410/diff/3/
> 
> 
> Testing
> ---
> 
> This patch cannot be compiled standalone. Tests done later in the chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66163: Built storage local resource provider with CMake.

[Andrew Schwartzmeyer]

> On April 12, 2018, 10:06 a.m., Andrew Schwartzmeyer wrote:
> > src/examples/CMakeLists.txt
> > Lines 55-59 (patched)
> > 
> >
> > What about non-Linux but not Windows platforms, e.g. MacOS and FreeBSD?

Saw in prior revision that it explicitly is only Linux because it includes 
`linux/fs.hpp`, maybe change the note that it's not non-Windows, but explicitly 
Linux specific.


- Andrew


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


On April 11, 2018, 10:15 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66163/
> ---
> 
> (Updated April 11, 2018, 10:15 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8698
> https://issues.apache.org/jira/browse/MESOS-8698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds CMake rules for compiling necessary source code for
> building storage local resource provider and related tests.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 
>   src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
>   src/examples/CMakeLists.txt e85eed607715d2b3ee596c3c0b51f16682491900 
>   src/resource_provider/storage/CMakeLists.txt PRE-CREATION 
>   src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 
> 
> 
> Diff: https://reviews.apache.org/r/66163/diff/3/
> 
> 
> Testing
> ---
> 
> `sudo make check` with CMake
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66163: Built storage local resource provider with CMake.

[Andrew Schwartzmeyer]

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




cmake/CompilationConfigure.cmake
Lines 387-390 (patched)


Do you know which targets actually require `ENABLE_GRPC` set as a 
pre-processor definition? Since it's new code being added, we should be able to 
resovle this TODO before committing.



src/CMakeLists.txt
Lines 387-388 (patched)


:(



src/examples/CMakeLists.txt
Lines 55-59 (patched)


What about non-Linux but not Windows platforms, e.g. MacOS and FreeBSD?



src/examples/CMakeLists.txt
Lines 93-98 (patched)


Ditto.



src/examples/CMakeLists.txt
Lines 97 (patched)


Nit: since it's already out of alignment due to being in a conditional, 
ther's no need to add the spaces to try to align `PRIVATE mesos grpc`.



src/resource_provider/storage/CMakeLists.txt
Lines 18 (patched)


Nit: I've been aligning the `###`... but I know it's that important.



src/tests/CMakeLists.txt
Line 237 (original), 250-254 (patched)


So `uri_disk_profile_adaptor` is, what, a module? that depends on (and 
links in `mesos`), but then `mesos-tess-interface` (which links in `mesos`) 
also links in `uri_disk_profile_adaptor`. So if things are being linked 
statically, `mesos-tests` would then then have `libmesos` twice? Sorry, I'm 
just trying to sort out the dependency graph in my head. Can you elaborate?



src/tests/CMakeLists.txt
Lines 337-341 (patched)


Maybe add a note that this is a binary and that's why it uses 
`add_dependencies`.


- Andrew Schwartzmeyer


On April 11, 2018, 10:15 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66163/
> ---
> 
> (Updated April 11, 2018, 10:15 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8698
> https://issues.apache.org/jira/browse/MESOS-8698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds CMake rules for compiling necessary source code for
> building storage local resource provider and related tests.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 
>   src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
>   src/examples/CMakeLists.txt e85eed607715d2b3ee596c3c0b51f16682491900 
>   src/resource_provider/storage/CMakeLists.txt PRE-CREATION 
>   src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 
> 
> 
> Diff: https://reviews.apache.org/r/66163/diff/3/
> 
> 
> Testing
> ---
> 
> `sudo make check` with CMake
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66462: Added new operation states to be used for status reconciliation.

[Gaston Kleiman]

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

(Updated April 12, 2018, 9:59 a.m.)


Review request for mesos and Greg Mann.


Changes
---

Addressed feedback from Chun.


Repository: mesos


Description
---

Added new operation states to be used for status reconciliation.


Diffs (updated)
-

  include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
  include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
  src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
  src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 


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

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


Testing
---

`sudo bin/mesos-tests` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 61118: Libprocess: Building gRPC support with CMake.

[Andrew Schwartzmeyer]

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




3rdparty/libprocess/src/tests/CMakeLists.txt
Lines 91 (patched)


I think this should instead be appended above. That is, after setting 
`GRPC_TESTS_PROTOS`, do a `list(APPEND PROCESS_TESTS_SRC 
${GRPC_TESTS_PROTOS})`, to be consistent with the rest.



3rdparty/libprocess/src/tests/CMakeLists.txt
Lines 93 (patched)


Maybe leave a note that this is for the generated gRPC headers.


- Andrew Schwartzmeyer


On March 19, 2018, 4:53 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61118/
> ---
> 
> (Updated March 19, 2018, 4:53 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7881
> https://issues.apache.org/jira/browse/MESOS-7881
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch enables CMake to build code for gRPC support in libprocess
> along with tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 0ce7dac5deea94623530820d173ce3ffe5b42ea4 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> d624402bc9efb43a130a2afdf27cfc1aa69010e4 
> 
> 
> Diff: https://reviews.apache.org/r/61118/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo make check` with CMake
> `cd 3rdparty/libprocess/src/tests && make libprocess-tests && 
> ./libprocess-tests`
> 
> 
> NOTE: Testing on Windowns is not done yet.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66409: Used `csi::v0::VolumeCapability` in disk profile adaptors.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On April 3, 2018, 7:18 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66409/
> ---
> 
> (Updated April 3, 2018, 7:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8717
> https://issues.apache.org/jira/browse/MESOS-8717
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To adapt the change in CSI package names (from `csi` to `csi.v0`), we
> now use `csi::v0::VolumeCapability` in the disk profile adaptor module
> interface.
> 
> NOTE: This is not future-proof if there is a breaking change in
> `VolumeCapability`, we might need to change the module interface to
> support multiple CSI versions in the future.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/storage/disk_profile_adaptor.hpp 
> 3a2d86a630fa949df2a5741f12f80fc434407cdc 
>   src/resource_provider/storage/disk_profile.proto 
> b9448d259b053a62fe2ffe5ae29c9f9be00e 
>   src/resource_provider/storage/disk_profile_utils.hpp 
> 9a0a31b48274dfa4d74427aed2a72a4ff245ef7f 
>   src/resource_provider/storage/disk_profile_utils.cpp 
> d07c11fdcf7108e1d680f8e2df743426a65331ac 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 948f6efc916d6fad0703944ad9acb77d85e4c25e 
> 
> 
> Diff: https://reviews.apache.org/r/66409/diff/1/
> 
> 
> Testing
> ---
> 
> This patch cannot be compiled standalone. Tests done later in the chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66408: Updated CSI helpers for v0.2.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On April 4, 2018, 3:59 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66408/
> ---
> 
> (Updated April 4, 2018, 3:59 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8717
> https://issues.apache.org/jira/browse/MESOS-8717
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds new helper classes for CSI plugin and node capabilities,
> removed helpers for the removed csi.Version proto message, and makes
> the VolumeState proto message uses csi::v0::VolumeCapability.
> 
> NOTE: This is not future-proof if there is a breaking change in
> VolumeCapability, we might need to change VolumeState to support
> multiple CSI versions in the future.
> 
> 
> Diffs
> -
> 
>   src/csi/state.hpp dcbc7aba84e0e66dd0e6caf7a64fb95aae69de0a 
>   src/csi/state.proto 0373c8ad217998a54b06d483f94052bcf4293677 
>   src/csi/utils.hpp 58b071d4f1512e4cf10d077bd1e03e66626f6dff 
>   src/csi/utils.cpp 9e0435762a16f2995bc5a04ec342af6ea231054a 
> 
> 
> Diff: https://reviews.apache.org/r/66408/diff/2/
> 
> 
> Testing
> ---
> 
> This patch cannot be compiled standalone. Tests done later in the chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66407: Updated CSI client to support v0.2.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On April 3, 2018, 7:12 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66407/
> ---
> 
> (Updated April 3, 2018, 7:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8653
> https://issues.apache.org/jira/browse/MESOS-8653
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To adapt the change in CSI package names (from `csi` to `csi.v0`), we
> introduce a new `csi::v0` namespace in Mesos for v0 helpers. The CSI
> client class is now defined in this namespace, and its public methods
> are updated to reflect the changes in CSI v0.2.
> 
> 
> Diffs
> -
> 
>   include/csi/spec.hpp da0caaedab76a203a12db8518a15444958b11975 
>   src/csi/client.hpp 1d55994423df3e539205d5353677b8258fd30abf 
>   src/csi/client.cpp 5df788f9b2f2331428375097660f8e4bd371e70d 
>   src/tests/csi_client_tests.cpp 566a85eba5632d182bb100f793e73321523c94b4 
>   src/tests/mock_csi_plugin.hpp b19870dc46709517a581c0fa4f1ab3cdec27ac82 
>   src/tests/mock_csi_plugin.cpp 1dc87617b4ab904d9c50aaa3b3a07dd101a4a952 
> 
> 
> Diff: https://reviews.apache.org/r/66407/diff/1/
> 
> 
> Testing
> ---
> 
> This patch cannot be compiled standalone. Tests done later in the chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66577: Enabled CSI proto compilation by default.

[Andrew Schwartzmeyer]

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



Note: I did not review the Autotools part at all ;)


src/CMakeLists.txt
Lines 32-35 (patched)


Nit: can this be moved down so it goes "no options", "grpc option", "java 
option", "internal option."


- Andrew Schwartzmeyer


On April 11, 2018, 6:39 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66577/
> ---
> 
> (Updated April 11, 2018, 6:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.
> 
> 
> Bugs: MESOS-8724
> https://issues.apache.org/jira/browse/MESOS-8724
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CSI proto compilation is disabled due to MESOS-8724, which is resolved
> by bumping CSI to v0.2. This patch enables the compilation again.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
>   src/Makefile.am 257ff0e8d21e5240c27a123ed0cd563214e24fce 
>   src/slave/flags.cpp bdfc49a3903899b2741bb60c7e9e89f0196492e4 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66577/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 61096: Building gRPC with CMake.

[Andrew Schwartzmeyer]

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



I want to test on Windows and see if we can get it building there too before 
giving it a ship-it, but this looks good!


3rdparty/CMakeLists.txt
Line 143 (original), 144-145 (patched)


Seems reasonable. Could alternatively setup `ENABLE_GRPC` to set 
`ENABLE_SSL` (and therefore also `libevent`?)



3rdparty/CMakeLists.txt
Lines 1012-1015 (patched)


As stated above, the probably won't work on Windows yet. Ths VS solutions 
generally stick everything under `Debug` or `Release` folders, and it's done at 
build time, not configuration time. It's super annoying.



3rdparty/CMakeLists.txt
Lines 1022-1027 (patched)


Do you want the `C` or `CXX` flags forwarded too?


- Andrew Schwartzmeyer


On April 11, 2018, 8:22 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61096/
> ---
> 
> (Updated April 11, 2018, 8:22 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7881
> https://issues.apache.org/jira/browse/MESOS-7881
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds an `ENABLE_GRPC` option and builds the bundled gRPC
> 3rdparty library in CMake.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 488e906486a583e74faceb44d906cee5036a8b99 
>   3rdparty/cmake/Versions.cmake 93f0322c1ac926bcfdcd4c1cfd9ba9f22bcf7099 
>   cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 
>   src/cmake/MesosProtobuf.cmake dde034fe4d4251dd7c7397d422136bfa3c9c3ebc 
> 
> 
> Diff: https://reviews.apache.org/r/61096/diff/6/
> 
> 
> Testing
> ---
> 
> `sudo make check` with CMake
> 
> NOTE: Testing on Windowns is not done yet.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66541: Added default executor test for agent recovery without metadata.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66538, 66539, 66540, 66541]

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 April 11, 2018, 8:40 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66541/
> ---
> 
> (Updated April 11, 2018, 8:40 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8416
> https://issues.apache.org/jira/browse/MESOS-8416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added default executor test for agent recovery without metadata.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 293dd20d882447401572835bd31e197faf76861b 
> 
> 
> Diff: https://reviews.apache.org/r/66541/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> Verified that the test crashes without the fix in r/66539.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66398: Bumped the CSI spec bundle to 0.2.0.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On April 3, 2018, 6:45 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66398/
> ---
> 
> (Updated April 3, 2018, 6:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8650
> https://issues.apache.org/jira/browse/MESOS-8650
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The bundle is generated as follows:
> 
> 1. Download the tarball from the following link:
>https://github.com/container-storage-interface/spec/archive/
>v0.2.0.tar.gz
> 2. Run the following commands:
>tar zxf v0.2.0.tar.gz
>mv spec-0.2.0 csi-0.2.0
>tar zcf csi-0.2.0.tar.gz csi-0.2.0
> 
> 
> Diffs
> -
> 
>   3rdparty/cmake/Versions.cmake 93f0322c1ac926bcfdcd4c1cfd9ba9f22bcf7099 
>   3rdparty/csi-0.1.0.tar.gz 37b1b4ca71aec3e036796b404baba887ac047957 
>   3rdparty/csi-0.2.0.tar.gz PRE-CREATION 
>   3rdparty/versions.am 1bc87bb1ec2e74ebb2072f63163e3c1e8b4aad00 
> 
> 
> Diff: https://reviews.apache.org/r/66398/diff/1/
> 
> 
> Testing
> ---
> 
> This patch cannot be compiled standalone. Tests done later in the chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66462: Added new operation states to be used for status reconciliation.

[Gaston Kleiman]

> On April 11, 2018, 7:06 p.m., Chun-Hung Hsiao wrote:
> > Are all of these necessary for now?

Yeah, they are all generated/sent by the {{ReconcileOperations}} handler.


> On April 11, 2018, 7:06 p.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 8034-8038 (patched)
> > 
> >
> > Move these outside the block.

lol, good catch, thanks!!!


- Gaston


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


On April 11, 2018, 10:58 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66462/
> ---
> 
> (Updated April 11, 2018, 10:58 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added new operation states to be used for status reconciliation.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
>   src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
>   src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66462/diff/3/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66493: Made FreeBSD default to non-GNU ld.

[Andrew Schwartzmeyer]

> On April 9, 2018, 6:13 a.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 315 (patched)
> > 
> >
> > This conditional seems not needed as cmake will automatically detect if 
> > `LD_PROGRAM` was set by the user. Is this a variable we should document as 
> > a customization point?
> 
> Andrew Schwartzmeyer wrote:
> Do you mean documenting like as an `option(...)` and in the docs, or just 
> in the docs? Maybe we should wait until the FreeBSD build is working, and 
> then add a doc for it (in which we could also explain the weirdness with 
> linkers).
> 
> David Forsythe wrote:
> Hm, I didn't realize find_program wouldn't run if I set this on my own. 
> I'll clean it up.
> 
> As for documenting it, I figured that it would be best to wait until the 
> build was fixed and settled. Even if someone is building on FreeBSD at this 
> point nothing is really working. It might be a bit pre-mature to start 
> surfacing docs about the build (which probably means there should be a ticket 
> about documenting this?).

+1


> On April 9, 2018, 6:13 a.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 326-329 (patched)
> > 
> >
> > I am not sure this is an issue, but customary there are similar 
> > variables defined for other build types like `DEBUG`, `RELEASE`, etc. 
> > (e.g., `CMAKE_EXE_LINKER_FLAGS_DEBUG`).
> > 
> > Do we have something to set all related flags @andschwa?
> 
> Andrew Schwartzmeyer wrote:
> It might soon be time to put this into a function, but yeah, it'd be a 
> nested for loop akin to this:
> 
> ```
> foreach (lang C CXX)
>   list(APPEND CMAKE_${lang}_FORWARD_ARGS
> ${CMAKE_FORWARD_ARGS}
> -DCMAKE_${lang}_COMPILER=${CMAKE_${lang}_COMPILER}
> -DCMAKE_${lang}_COMPILER_LAUNCHER=${CMAKE_${lang}_COMPILER_LAUNCHER}
> -DCMAKE_${lang}_FLAGS=${CMAKE_${lang}_FLAGS})
> 
>   foreach (config DEBUG RELEASE RELWITHDEBINFO MINSIZEREL)
> list(APPEND CMAKE_${lang}_FORWARD_ARGS
>   -DCMAKE_${lang}_FLAGS_${config}=${CMAKE_${lang}_FLAGS_${config}})
>   endforeach ()
> endforeach ()
> 
> ```
> 
> Note that this only matters if you're using a multi-configuration 
> generator (like Visual Studio and some other IDEs), but not `make` nor 
> `ninja`.
> 
> David Forsythe wrote:
> Will switch over to something like that.
> 
> David Forsythe wrote:
> I left off the the config loop because it didn't seem like the other 
> flags being set in this file were using them. It wasn't clear to me if they 
> were actually needed, so if I should add them just let me know.

I wouldn't add it until you actually need it.


- Andrew


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


On April 11, 2018, 10:13 p.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66493/
> ---
> 
> (Updated April 11, 2018, 10:13 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
> Bugs: MESOS-8761
> https://issues.apache.org/jira/browse/MESOS-8761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made FreeBSD default to non-GNU ld.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 
> 
> 
> Diff: https://reviews.apache.org/r/66493/diff/3/
> 
> 
> Testing
> ---
> 
> make on FreeBSD, with both lld and gold.
> 
> 
> Thanks,
> 
> David Forsythe
> 
>

Re: Review Request 64970: Use tox for linting and testing code living uder src/python.

[Eric Chung]

> On April 6, 2018, 9:46 a.m., Armand Grillet wrote:
> > src/python/lib/tox.ini
> > Lines 19 (patched)
> > 
> >
> > As this is here, the line `pylint==1.6.4` in 
> > support/pip-requirements.txt should be removed.

the python files within support/ still require pylint to be installed in 
support/.virtualenv, since they are linted using the pylint installed in 
support/.virtualenv directly.


- Eric


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


On March 14, 2018, 3:05 p.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64970/
> ---
> 
> (Updated March 14, 2018, 3:05 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use tox for linting and testing code living uder src/python.
> 
> At the moment, all linting is done through the same `pylint`
> installation under support/.virtualenv, which requires ALL dependencies
> (i.e. pip-requirements.txt, requirements.in scattered in various
> directories) to be installed in the same virtualenv, making things
> really messy -- e.g. when I've changed some code under `src/python/lib`,
> but don't have the dev virtualenv activated, linting will fail since
> none of the dependencies under `src/python/lib` have been installed.
> 
> Using tox, we can solve this problem by distributing a "test spec"
> (tox.ini) in each of the python source directories which are aware of
> its local dependencies only. To test or lint the code there would be as
> simple as running `tox -e py27-lint `, and the corresponding
> virtualenv and test dependencies would automatically be setup.
> 
> This patch modifies `support/mesos-style.py` to install `tox` in
> `support/.virtualenv` and delegates linting to a `tox` call when it sees
> python directories that have tox setup for it. Linting for all other
> languages will not be effected.
> 
> Testing Done:
> 1. intentionally create a lint error, such as extra spaces before a
> parens in a python file
> 2. run the pre-commit hook and see tox in action
> 
> Reviewed at https://reviews.apache.org/r/64970/
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/tests/__init__.py PRE-CREATION 
>   src/python/cli_new/tox.ini PRE-CREATION 
>   src/python/lib/requirements-test.in 
> b2b73aab65377d9310797203ea84c5150ae60805 
>   src/python/lib/tox.ini fd5e89c77c8608fea21e9caad814c6e111ad57db 
>   support/mesos-style.py 47ec36949010fa511d1b3974739c5ad5c03f6f7f 
> 
> 
> Diff: https://reviews.apache.org/r/64970/diff/6/
> 
> 
> Testing
> ---
> 
> 1. intentionally create a lint error, such as extra spaces before a parens in 
> a python file
> 2. run the pre-commit hook and see tox in action
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 66493: Made FreeBSD default to non-GNU ld.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66493]

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 April 12, 2018, 5:13 a.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66493/
> ---
> 
> (Updated April 12, 2018, 5:13 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
> Bugs: MESOS-8761
> https://issues.apache.org/jira/browse/MESOS-8761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made FreeBSD default to non-GNU ld.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 
> 
> 
> Diff: https://reviews.apache.org/r/66493/diff/3/
> 
> 
> Testing
> ---
> 
> make on FreeBSD, with both lld and gold.
> 
> 
> Thanks,
> 
> David Forsythe
> 
>

Re: Review Request 64211: Added options to build the Python CLI and run unit tests.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On March 24, 2018, 11:39 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64211/
> ---
> 
> (Updated March 24, 2018, 11:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Kevin Klues.
> 
> 
> Bugs: MESOS-8240
> https://issues.apache.org/jira/browse/MESOS-8240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An update of the discarded review /r/52543.
> 
> Works with Autotools and CMake.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 6702f02245e3867c06bbd9efbbf4e3b961a7d9aa 
>   cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 
>   configure.ac f0f901f2e565352c2804cae7b2ac255da81ce45d 
>   docs/configuration/cmake.md 1e34657ea55fa324f65f865f7d0a67084c6719d9 
>   src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
>   src/Makefile.am 257ff0e8d21e5240c27a123ed0cd563214e24fce 
>   src/python/cli_new/CMakeLists.txt PRE-CREATION 
>   src/python/cli_new/tests/CMakeLists.txt PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64211/diff/18/
> 
> 
> Testing
> ---
> 
> Testing done on Fedora 25.
> 
> For Autotools:
> ```
> $ ./bootstrap
> $ mkdir build
> $ cd build
> $ ../configure --enable-new-cli --disable-java --disable-python
> $ make check
> ```
> 
> For CMake:
> ```
> $ ./bootstrap
> $ mkdir build
> $ cd build
> $ cmake .. -DENABLE_NEW_CLI=1
> $ cmake --build . -- -j16
> $ ./src/mesos
> Mesos CLI
> 
> Usage:
>   mesos (-h | --help)
>   mesos --version
>   mesos  [...]
> 
> Options:
>   -h --help  Show this screen.
>   --version  Show version info.
> 
> Commands:
>   agent   Interacts with the Mesos agents
>   config  Interacts with the Mesos CLI configuration file
>   taskInteracts with the tasks running in a Mesos cluster
> 
> See 'mesos help ' for more information on a specific command.
> $ cmake --build . --target tests -- -j16
> $ ctest -R CLI
> Test project /home/agrillet/apache-mesos/build
> Start 4: CLITests
> 1/1 Test #4: CLITests .   Passed3.63 sec
> 
> 100% tests passed, 0 tests failed out of 1
> ```
> 
> Checked that the the CLI tests were run, that the content of the directory 
> `build/src/cli` was as expected, and that `build/src/mesos` was correctly 
> running.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 65705: Fixed CLI bootstrap script to work with long workspace paths.

[Armand Grillet]

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

(Updated April 12, 2018, 12:39 p.m.)


Review request for mesos, Benjamin Bannier and Kevin Klues.


Changes
---

Removed change regarding deactivation of existing virtualenv.


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


Repository: mesos


Description
---

If the absolute path to the Python interpreter is long, it may exceed
the maximum length allowed for a shebang line (limit set to 128 on many
Linux distributions). When the shebang length limit is exceeded, pip
fails with the error: "bad interpreter: No such file or directory".

A work-around for this problem is to run the pip library module which is
what we do in this patch. This will also be done to use PyInstaller.


Diffs (updated)
-

  src/python/cli_new/bootstrap 9329a61a2a1f05286b4ff6e5fe68cd86ed48859a 


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

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


Testing
---

Tested on internal CI.


Thanks,

Armand Grillet

Re: Review Request 65585: Improved documentation regarding the new CLI setup.

[Kevin Klues]

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




src/python/cli_new/README.md
Lines 40-55 (patched)


I am going to change this to "~/.mesos-cli-venv" before I commit it.



src/python/cli_new/bin/mesos
Line 5 (original)


This should probably have been in a spearate commit since it's a 
functionality change and not just documentation.



src/python/cli_new/bin/mesos-cli-tests
Line 5 (original)


This should probably have been in a spearate commit since it's a 
functionality change and not just documentation.


- Kevin Klues


On April 12, 2018, 12:36 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65585/
> ---
> 
> (Updated April 12, 2018, 12:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Kevin Klues.
> 
> 
> Bugs: MESOS-8240
> https://issues.apache.org/jira/browse/MESOS-8240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Explains how to create the necessary virtual environment from
> anywhere and how to set up autocompletion in such case.
> 
> Also removes an unnecessary activation of the virtual environment
> in `mesos` and `mesos-cli-tests`.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/README.md 847141db9a1f9eb5c78b2d50367b599a5c72ce61 
>   src/python/cli_new/bin/mesos c5152a2ebf8704c804bb4f39e46580a512aecdea 
>   src/python/cli_new/bin/mesos-cli-tests 
> 07659e0b4551c2381828b256608d2c6ced3ae745 
>   src/python/cli_new/bootstrap 9329a61a2a1f05286b4ff6e5fe68cd86ed48859a 
> 
> 
> Diff: https://reviews.apache.org/r/65585/diff/6/
> 
> 
> Testing
> ---
> 
> On Fedora 25:
> ```
> apache-mesos (MESOS-8240)$ cd src/python/cli_new/
> cli_new (MESOS-8240)$ ./bootstrap
> cli_new (MESOS-8240)$ source .virtualenv/activate
> (mesos-cli) cli_new (MESOS-8240)$ mesos
> Mesos CLI
> 
> Usage:
>   mesos (-h | --help)
>   mesos --version
>   mesos  [...]
> 
> Options:
>   -h --help  Show this screen.
>   --version  Show version info.
> 
> Commands:
>   agent   Interacts with the Mesos agents
>   config  Interacts with the Mesos CLI configuration file
>   taskInteracts with the tasks running in a Mesos cluster
> 
> See 'mesos help ' for more information on a specific command.
> (mesos-cli) cli_new (MESOS-8240)$ source deactivate
> cli_new (MESOS-8240)$ rm -rf .virtualenv/
> cli_new (MESOS-8240)$ cd ..
> python (MESOS-8240)$ VIRTUALENV_DIRECTORY=$(pwd)/.venv ./cli_new/bootstrap
> python (MESOS-8240)$ source 
> /home/agrillet/apache-mesos/src/python/.venv/bin/activate
> python (MESOS-8240)$ source 
> /home/agrillet/apache-mesos/src/python/.venv/bin/postactivate
> (mesos-cli) python (MESOS-8240)$ mesos
> Mesos CLI
> 
> Usage:
>   mesos (-h | --help)
>   mesos --version
>   mesos  [...]
> 
> Options:
>   -h --help  Show this screen.
>   --version  Show version info.
> 
> Commands:
>   agent   Interacts with the Mesos agents
>   config  Interacts with the Mesos CLI configuration file
>   taskInteracts with the tasks running in a Mesos cluster
> 
> See 'mesos help ' for more information on a specific command.
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 65585: Improved documentation regarding the new CLI setup.

[Armand Grillet]

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

(Updated April 12, 2018, 12:36 p.m.)


Review request for mesos, Benjamin Bannier and Kevin Klues.


Changes
---

Fixed issues.


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


Repository: mesos


Description
---

Explains how to create the necessary virtual environment from
anywhere and how to set up autocompletion in such case.

Also removes an unnecessary activation of the virtual environment
in `mesos` and `mesos-cli-tests`.


Diffs (updated)
-

  src/python/cli_new/README.md 847141db9a1f9eb5c78b2d50367b599a5c72ce61 
  src/python/cli_new/bin/mesos c5152a2ebf8704c804bb4f39e46580a512aecdea 
  src/python/cli_new/bin/mesos-cli-tests 
07659e0b4551c2381828b256608d2c6ced3ae745 
  src/python/cli_new/bootstrap 9329a61a2a1f05286b4ff6e5fe68cd86ed48859a 


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

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


Testing
---

On Fedora 25:
```
apache-mesos (MESOS-8240)$ cd src/python/cli_new/
cli_new (MESOS-8240)$ ./bootstrap
cli_new (MESOS-8240)$ source .virtualenv/activate
(mesos-cli) cli_new (MESOS-8240)$ mesos
Mesos CLI

Usage:
  mesos (-h | --help)
  mesos --version
  mesos  [...]

Options:
  -h --help  Show this screen.
  --version  Show version info.

Commands:
  agent   Interacts with the Mesos agents
  config  Interacts with the Mesos CLI configuration file
  taskInteracts with the tasks running in a Mesos cluster

See 'mesos help ' for more information on a specific command.
(mesos-cli) cli_new (MESOS-8240)$ source deactivate
cli_new (MESOS-8240)$ rm -rf .virtualenv/
cli_new (MESOS-8240)$ cd ..
python (MESOS-8240)$ VIRTUALENV_DIRECTORY=$(pwd)/.venv ./cli_new/bootstrap
python (MESOS-8240)$ source 
/home/agrillet/apache-mesos/src/python/.venv/bin/activate
python (MESOS-8240)$ source 
/home/agrillet/apache-mesos/src/python/.venv/bin/postactivate
(mesos-cli) python (MESOS-8240)$ mesos
Mesos CLI

Usage:
  mesos (-h | --help)
  mesos --version
  mesos  [...]

Options:
  -h --help  Show this screen.
  --version  Show version info.

Commands:
  agent   Interacts with the Mesos agents
  config  Interacts with the Mesos CLI configuration file
  taskInteracts with the tasks running in a Mesos cluster

See 'mesos help ' for more information on a specific command.
```


Thanks,

Armand Grillet

Re: Review Request 65585: Improved documentation regarding the new CLI setup.

[Kevin Klues]

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




src/python/cli_new/bootstrap
Lines 120 (patched)


This should be ${CURRDIR} too.
There is no need to introduce a new ${BINDIR} here.


- Kevin Klues


On March 24, 2018, 11:39 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65585/
> ---
> 
> (Updated March 24, 2018, 11:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Kevin Klues.
> 
> 
> Bugs: MESOS-8240
> https://issues.apache.org/jira/browse/MESOS-8240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Explains how to create the necessary virtual environment from
> anywhere and how to set up autocompletion in such case.
> 
> Also removes an unnecessary activation of the virtual environment
> in `mesos` and `mesos-cli-tests`.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/README.md 847141db9a1f9eb5c78b2d50367b599a5c72ce61 
>   src/python/cli_new/bin/mesos c5152a2ebf8704c804bb4f39e46580a512aecdea 
>   src/python/cli_new/bin/mesos-cli-tests 
> 07659e0b4551c2381828b256608d2c6ced3ae745 
>   src/python/cli_new/bootstrap 9329a61a2a1f05286b4ff6e5fe68cd86ed48859a 
> 
> 
> Diff: https://reviews.apache.org/r/65585/diff/5/
> 
> 
> Testing
> ---
> 
> On Fedora 25:
> ```
> apache-mesos (MESOS-8240)$ cd src/python/cli_new/
> cli_new (MESOS-8240)$ ./bootstrap
> cli_new (MESOS-8240)$ source .virtualenv/activate
> (mesos-cli) cli_new (MESOS-8240)$ mesos
> Mesos CLI
> 
> Usage:
>   mesos (-h | --help)
>   mesos --version
>   mesos  [...]
> 
> Options:
>   -h --help  Show this screen.
>   --version  Show version info.
> 
> Commands:
>   agent   Interacts with the Mesos agents
>   config  Interacts with the Mesos CLI configuration file
>   taskInteracts with the tasks running in a Mesos cluster
> 
> See 'mesos help ' for more information on a specific command.
> (mesos-cli) cli_new (MESOS-8240)$ source deactivate
> cli_new (MESOS-8240)$ rm -rf .virtualenv/
> cli_new (MESOS-8240)$ cd ..
> python (MESOS-8240)$ VIRTUALENV_DIRECTORY=$(pwd)/.venv ./cli_new/bootstrap
> python (MESOS-8240)$ source 
> /home/agrillet/apache-mesos/src/python/.venv/bin/activate
> python (MESOS-8240)$ source 
> /home/agrillet/apache-mesos/src/python/.venv/bin/postactivate
> (mesos-cli) python (MESOS-8240)$ mesos
> Mesos CLI
> 
> Usage:
>   mesos (-h | --help)
>   mesos --version
>   mesos  [...]
> 
> Options:
>   -h --help  Show this screen.
>   --version  Show version info.
> 
> Commands:
>   agent   Interacts with the Mesos agents
>   config  Interacts with the Mesos CLI configuration file
>   taskInteracts with the tasks running in a Mesos cluster
> 
> See 'mesos help ' for more information on a specific command.
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 65705: Fixed CLI bootstrap script to work with long workspace paths.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On March 24, 2018, 11:39 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65705/
> ---
> 
> (Updated March 24, 2018, 11:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Kevin Klues.
> 
> 
> Bugs: MESOS-8240
> https://issues.apache.org/jira/browse/MESOS-8240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the absolute path to the Python interpreter is long, it may exceed
> the maximum length allowed for a shebang line (limit set to 128 on many
> Linux distributions). When the shebang length limit is exceeded, pip
> fails with the error: "bad interpreter: No such file or directory".
> 
> A work-around for this problem is to run the pip library module which is
> what we do in this patch. This will also be done to use PyInstaller.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/bootstrap 9329a61a2a1f05286b4ff6e5fe68cd86ed48859a 
> 
> 
> Diff: https://reviews.apache.org/r/65705/diff/4/
> 
> 
> Testing
> ---
> 
> Tested on internal CI.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 64211: Added options to build the Python CLI and run unit tests.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On March 24, 2018, 11:39 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64211/
> ---
> 
> (Updated March 24, 2018, 11:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Kevin Klues.
> 
> 
> Bugs: MESOS-8240
> https://issues.apache.org/jira/browse/MESOS-8240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An update of the discarded review /r/52543.
> 
> Works with Autotools and CMake.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 6702f02245e3867c06bbd9efbbf4e3b961a7d9aa 
>   cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 
>   configure.ac f0f901f2e565352c2804cae7b2ac255da81ce45d 
>   docs/configuration/cmake.md 1e34657ea55fa324f65f865f7d0a67084c6719d9 
>   src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
>   src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a 
>   src/python/cli_new/CMakeLists.txt PRE-CREATION 
>   src/python/cli_new/tests/CMakeLists.txt PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64211/diff/17/
> 
> 
> Testing
> ---
> 
> Testing done on Fedora 25.
> 
> For Autotools:
> ```
> $ ./bootstrap
> $ mkdir build
> $ cd build
> $ ../configure --enable-new-cli --disable-java --disable-python
> $ make check
> ```
> 
> For CMake:
> ```
> $ ./bootstrap
> $ mkdir build
> $ cd build
> $ cmake .. -DENABLE_NEW_CLI=1
> $ cmake --build . -- -j16
> $ ./src/mesos
> Mesos CLI
> 
> Usage:
>   mesos (-h | --help)
>   mesos --version
>   mesos  [...]
> 
> Options:
>   -h --help  Show this screen.
>   --version  Show version info.
> 
> Commands:
>   agent   Interacts with the Mesos agents
>   config  Interacts with the Mesos CLI configuration file
>   taskInteracts with the tasks running in a Mesos cluster
> 
> See 'mesos help ' for more information on a specific command.
> $ cmake --build . --target tests -- -j16
> $ ctest -R CLI
> Test project /home/agrillet/apache-mesos/build
> Start 4: CLITests
> 1/1 Test #4: CLITests .   Passed3.63 sec
> 
> 100% tests passed, 0 tests failed out of 1
> ```
> 
> Checked that the the CLI tests were run, that the content of the directory 
> `build/src/cli` was as expected, and that `build/src/mesos` was correctly 
> running.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 65705: Fixed CLI bootstrap script to work with long workspace paths.

[Kevin Klues]

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




src/python/cli_new/bootstrap
Line 27 (original), 27 (patched)


I see why this is a good change, but in the future, this should be done in 
a seperate commit.


- Kevin Klues


On March 24, 2018, 11:39 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65705/
> ---
> 
> (Updated March 24, 2018, 11:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Kevin Klues.
> 
> 
> Bugs: MESOS-8240
> https://issues.apache.org/jira/browse/MESOS-8240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the absolute path to the Python interpreter is long, it may exceed
> the maximum length allowed for a shebang line (limit set to 128 on many
> Linux distributions). When the shebang length limit is exceeded, pip
> fails with the error: "bad interpreter: No such file or directory".
> 
> A work-around for this problem is to run the pip library module which is
> what we do in this patch. This will also be done to use PyInstaller.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/bootstrap 9329a61a2a1f05286b4ff6e5fe68cd86ed48859a 
> 
> 
> Diff: https://reviews.apache.org/r/65705/diff/4/
> 
> 
> Testing
> ---
> 
> Tested on internal CI.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 65585: Improved documentation regarding the new CLI setup.

[Kevin Klues]

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




src/python/cli_new/README.md
Lines 40 (patched)


$ export VIRTUALENV_DIRECTORY=~/.mesos-cli-venv
$ ${MESOS_DIR}/src/python/cli_new/bootstrap



src/python/cli_new/README.md
Lines 50-54 (patched)


Likewise, I'd change this to:
```
$ source ~/.mesos-cli-venv/bin/activate
$ source ~/.mesos-cli-venv/bin/postactivate
$ mesos  [...]
$ source ~/.mesos-cli-venv/bin/predeactivate
$ deactivate
```



src/python/cli_new/README.md
Lines 56-61 (patched)


Change referecnes of:

`/home/apache-mesos/`

to

`${MESOS_DIR}`



src/python/cli_new/bootstrap
Lines 105-108 (original), 103-124 (patched)


This will need to be changed to reflect the above comment.


- Kevin Klues


On March 24, 2018, 11:39 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65585/
> ---
> 
> (Updated March 24, 2018, 11:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Kevin Klues.
> 
> 
> Bugs: MESOS-8240
> https://issues.apache.org/jira/browse/MESOS-8240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Explains how to create the necessary virtual environment from
> anywhere and how to set up autocompletion in such case.
> 
> Also removes an unnecessary activation of the virtual environment
> in `mesos` and `mesos-cli-tests`.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/README.md 847141db9a1f9eb5c78b2d50367b599a5c72ce61 
>   src/python/cli_new/bin/mesos c5152a2ebf8704c804bb4f39e46580a512aecdea 
>   src/python/cli_new/bin/mesos-cli-tests 
> 07659e0b4551c2381828b256608d2c6ced3ae745 
>   src/python/cli_new/bootstrap 9329a61a2a1f05286b4ff6e5fe68cd86ed48859a 
> 
> 
> Diff: https://reviews.apache.org/r/65585/diff/5/
> 
> 
> Testing
> ---
> 
> On Fedora 25:
> ```
> apache-mesos (MESOS-8240)$ cd src/python/cli_new/
> cli_new (MESOS-8240)$ ./bootstrap
> cli_new (MESOS-8240)$ source .virtualenv/activate
> (mesos-cli) cli_new (MESOS-8240)$ mesos
> Mesos CLI
> 
> Usage:
>   mesos (-h | --help)
>   mesos --version
>   mesos  [...]
> 
> Options:
>   -h --help  Show this screen.
>   --version  Show version info.
> 
> Commands:
>   agent   Interacts with the Mesos agents
>   config  Interacts with the Mesos CLI configuration file
>   taskInteracts with the tasks running in a Mesos cluster
> 
> See 'mesos help ' for more information on a specific command.
> (mesos-cli) cli_new (MESOS-8240)$ source deactivate
> cli_new (MESOS-8240)$ rm -rf .virtualenv/
> cli_new (MESOS-8240)$ cd ..
> python (MESOS-8240)$ VIRTUALENV_DIRECTORY=$(pwd)/.venv ./cli_new/bootstrap
> python (MESOS-8240)$ source 
> /home/agrillet/apache-mesos/src/python/.venv/bin/activate
> python (MESOS-8240)$ source 
> /home/agrillet/apache-mesos/src/python/.venv/bin/postactivate
> (mesos-cli) python (MESOS-8240)$ mesos
> Mesos CLI
> 
> Usage:
>   mesos (-h | --help)
>   mesos --version
>   mesos  [...]
> 
> Options:
>   -h --help  Show this screen.
>   --version  Show version info.
> 
> Commands:
>   agent   Interacts with the Mesos agents
>   config  Interacts with the Mesos CLI configuration file
>   taskInteracts with the tasks running in a Mesos cluster
> 
> See 'mesos help ' for more information on a specific command.
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 66578: Windows: Ported more unit tests from `os_tests.cpp`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66420, 66421, 66422, 66423, 66424, 66425, 66426, 66427, 
66428, 66455, 66429, 66430, 66431, 66432, 66434, 66435, 66436, 66437, 66433, 
66438, 66439, 66440, 66442, 66443, 66444, 66445, 66578]

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 April 12, 2018, 2:06 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66578/
> ---
> 
> (Updated April 12, 2018, 2:06 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> and Michael Park.
> 
> 
> Bugs: MESOS-3441
> https://issues.apache.org/jira/browse/MESOS-3441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed `os::sleep()` to return an invalid parameter error if given a
> negative value.
> 
> Fixed tests around the `cloexec` and `nonblock` stubs.
> 
> Extended the `bootid` test to use `std::chrono` to assert the boot
> id (which is the system boot time) is a reasonable value.
> 
> Permanently disabled `OsTest.Libraries` because there is no
> equivalent.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/bootid.hpp 
> d24e115b97ebcabfa808a9437fb41512f25a5271 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 900baf9be4e3089cb43e444b14b155a80bcd1591 
>   3rdparty/stout/tests/os_tests.cpp 4221ecdcefb5602ece20cc90b13c3f17057fcb4d 
> 
> 
> Diff: https://reviews.apache.org/r/66578/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66546: Prevent resubscription of resource providers with unknown IDs.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['66308', '66309', '66526', '66310', '66311', '66544', 
'66545', '66546']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
[--] 9 tests from Endpoint/SlaveEndpointTest (1019 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (32 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (36 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (70 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (770 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (795 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (846 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (875 ms total)

[--] Global test environment tear-down
[==] 956 tests from 94 test cases ran. (465470 ms total)
[  PASSED  ] 954 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] 
ContentType/ResourceProviderManagerHttpApiTest.ResubscribeResourceProvider/0, 
where GetParam() = application/x-protobuf
[  FAILED  ] 
ContentType/ResourceProviderManagerHttpApiTest.ResubscribeResourceProvider/1, 
where GetParam() = application/json

 2 FAILED TESTS
  YOU HAVE 214 DISABLED TESTS

```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66546/logs/mesos-tests-stderr.log):

```
I0412 10:31:33.275807 13936 exec.cpp:236] Executor registered on agent 
eb8ea3fc-64a0-4381-a846-aff8f1209e21-S0
I0412 10:31:33.279808  2824 executor.cpp:177] Received SUBSCRIBED event
I0412 10:31:33.283825  2824 executor.cpp:181] Subscribed executor on 
winbldsrv-02.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0412 10:31:33.284823  2824 executor.cpp:177] Received LAUNCH event
I0412 10:31:33.289826  2824 executor.cpp:649] Starting task 
d5c93b38-11d8-4d5c-b312-b920d69adb89
I0412 10:31:33.368827  2824 executor.cpp:484] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0412 10:31:33.397830  2824 executor.cpp:662] Forked command at 11000
I0412 10:31:33.424813  8340 exec.cpp:445] Executor asked to shutdown
I0412 10:31:33.425859  9500 executor.cpp:177] Received SHUTDOWN event
I0412 10:31:33.425859  9500 executor.cpp:759] Shutting down
I0412 10:31:33.425859  9500 executor.cpp:869] Sending SIGTERM to process tree 
at pid 833 11896 hierarchical.cpp:405] Deactivated framework 
eb8ea3fc-64a0-4381-a846-aff8f1209e21-
I0412 10:31:33.423830  7408 master.cpp:10453] Updating the state of task 
d5c93b38-11d8-4d5c-b312-b920d69adb89 of framework 
eb8ea3fc-64a0-4381-a846-aff8f1209e21- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0412 10:31:33.423830  8768 slave.cpp:3971] Shutting down framework 
eb8ea3fc-64a0-4381-a846-aff8f1209e21-
I0412 10:31:33.423830  8768 slave.cpp:6683] Shutting down executor 
'd5c93b38-11d8-4d5c-b312-b920d69adb89' of framework 
eb8ea3fc-64a0-4381-a846-aff8f1209e21- at executor(1)@10.3.1.5:60514
I0412 10:31:33.424813  5652 slave.cpp:919] Agent terminating
W0412 10:31:33.424813  5652 slave.cpp:3967] Ignoring shutdown framework 
eb8ea3fc-64a0-4381-a846-aff8f1209e21- because it is terminating
I0412 10:31:33.426811  7408 master.cpp:10552] Removing task 
d5c93b38-11d8-4d5c-b312-b920d69adb89 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework eb8ea3fc-64a0-4381-a846-aff8f1209e21- on 
agent eb8ea3fc-64a0-4381-a846-aff8f1209e21-S0 at slave(424)@10.3.1.5:60493 
(winbldsrv-02.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0412 10:31:33.428810 12176 containerizer.cpp:2339] Destroying container 
bf59f546-8774-45aa-aa3b-8626ae6fca83 in RUNNING state
I0412 10:31:33.429811 12176 containerizer.cpp:2953] Transitioning the state of 
container bf59f546-8774-45aa-aa3b-8626ae6fca83 from RUNNING to DESTROYING
I0412 10:31:33.429811  7408 master.cpp:1296] Agent 
eb8ea3fc-64a0-4381-a846-aff8f1209e21-S0 at slave(424)@10.3.1.5:60493 

Re: Review Request 66541: Added default executor test for agent recovery without metadata.

[Qian Zhang]

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


Fix it, then Ship it!





src/tests/default_executor_tests.cpp
Lines 1477-1478 (patched)


A newline between.



src/tests/default_executor_tests.cpp
Lines 1502 (patched)


It seems we do not need this since scheduler will not connect with master 
more than once.


- Qian Zhang


On April 12, 2018, 4:40 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66541/
> ---
> 
> (Updated April 12, 2018, 4:40 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8416
> https://issues.apache.org/jira/browse/MESOS-8416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added default executor test for agent recovery without metadata.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 293dd20d882447401572835bd31e197faf76861b 
> 
> 
> Diff: https://reviews.apache.org/r/66541/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> Verified that the test crashes without the fix in r/66539.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66538: Added unit test slave recovery for default executor tests.

[Qian Zhang]

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


Fix it, then Ship it!





src/tests/default_executor_tests.cpp
Lines 1375 (patched)


We are now verifying agent recovery rather than executor re-register. So I 
think the comments should be something like:
```
// This test verifies that the agent could recover if the agent
// metadata is checkpointed
```


- Qian Zhang


On April 12, 2018, 4:40 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66538/
> ---
> 
> (Updated April 12, 2018, 4:40 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8772
> https://issues.apache.org/jira/browse/MESOS-8772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit test slave recovery for default executor tests.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 293dd20d882447401572835bd31e197faf76861b 
> 
> 
> Diff: https://reviews.apache.org/r/66538/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66545: Added admitted resource providers to the manager's registry.

[Benjamin Bannier]

> On April 11, 2018, 2:47 p.m., Jan Schlicht wrote:
> > src/resource_provider/manager.cpp
> > Lines 668 (patched)
> > 
> >
> > Shouldn't this be `false` here? If there's no registrar to admit a 
> > resource provider to, a resource provider cannot be admitted.

Semantically we want to treat operation on a non-existing registrar as always 
successful, so this needs to be a `true`. I some comments.


> On April 11, 2018, 2:47 p.m., Jan Schlicht wrote:
> > src/resource_provider/manager.cpp
> > Lines 679-683 (patched)
> > 
> >
> > We should set this event handler before setting `admitResourceProvider` 
> > to any value, i.e. set this directly after instanciation.

I agree that this would read better and not alter the behavior of the `Future`. 
Unfortunately we still need to mutate `resourceProvider` -- namely possibly 
assign a `ResourceProviderID`. Since the continuation takes ownership of 
`resourceProvider` we wouldn't be able to `move` data before setting it, and I 
don't feel that introducing splitting generating an `ResourceProviderID` and 
setting up actions on the `Registrar` would make things much clearer either.

I'd suggest we keep the code as is.


- Benjamin


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


On April 12, 2018, 11:48 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66545/
> ---
> 
> (Updated April 12, 2018, 11:48 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8402
> https://issues.apache.org/jira/browse/MESOS-8402
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added admitted resource providers to the manager's registry.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a 
>   src/resource_provider/registrar.cpp 
> 92ef9aecb1e93d10f46e53984391558f9901a60b 
>   src/resource_provider/registry.proto 
> 14bd433ef056f8891e9a38153fdb06c39cee8f62 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66545/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66546: Prevent resubscription of resource providers with unknown IDs.

[Benjamin Bannier]

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

(Updated April 12, 2018, 11:48 a.m.)


Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.


Changes
---

Fixed a close comment typo.


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


Repository: mesos


Description
---

This patch adds a check to the resource provider manager's subscribe
functionality making sure that any the ID sent by a resubscribing
resource provider corresponds to some previously known resource
provider.

This not only serves as convenient validation of user-provided data,
but also makes sure that the internal state of the resource provider
manager remains consistent.


Diffs (updated)
-

  src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a 
  src/tests/resource_provider_manager_tests.cpp 
c52541bf130ccf4795b989b53331176a64a097ea 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66545: Added admitted resource providers to the manager's registry.

[Benjamin Bannier]

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

(Updated April 12, 2018, 11:48 a.m.)


Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jan Schlicht.


Changes
---

Addressed comments from Jan.


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


Repository: mesos


Description
---

Added admitted resource providers to the manager's registry.


Diffs (updated)
-

  src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a 
  src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b 
  src/resource_provider/registry.proto 14bd433ef056f8891e9a38153fdb06c39cee8f62 
  src/tests/resource_provider_manager_tests.cpp 
c52541bf130ccf4795b989b53331176a64a097ea 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66554: Updated Web UI controllers to have one file per controller.

[Armand Grillet]

> On April 11, 2018, 10:42 p.m., Benjamin Mahler wrote:
> > src/webui/app/agents/agent-browse-controller.js
> > Lines 35 (patched)
> > 
> >
> > I noticed these .js files just assume the right js files are already 
> > "imported" and there is a globals table in the linter? Is this how it's 
> > normally done or do files explicitly import other files? It struck me as 
> > tedious that we would have to keep a globals table up-to-date for the 
> > linter.

In JavaScript, the scope of a variable is its current execution context, i.e. 
either the enclosing function or global. ESLint offers two options to handle 
global variables as shown in 
https://eslint.org/docs/user-guide/configuring#specifying-globals. 
The issue with the first option, using `/* global var1, var2 */`, is that we 
need to add that comment in each file using `var1` or `var2`. My wish would 
have been to add e.g. `/* global updateMetrics, updateState */` in `utils.js` 
but this is not possible from what I have seen during my tests. 
The second solution, used in this review request, is a burden but does not 
oblige us to specify globals in multiple files.
A third solution would be to disable the `no-undef` rule but, from my 
experience, having those error messages can be useful.


- Armand


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


On April 11, 2018, 3:23 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66554/
> ---
> 
> (Updated April 11, 2018, 3:23 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Bugs: MESOS-8503
> https://issues.apache.org/jira/browse/MESOS-8503
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removes `controllers.js` and adds one file per controller. The shared
> functions are now in a file called `utils.js` and the linter has been
> updated to know about it. This gives more freedom to add or update
> each controller while making the structure of the UI more visible.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a 
>   src/webui/app/agents/agent-browse-controller.js PRE-CREATION 
>   src/webui/app/agents/agent-controller.js PRE-CREATION 
>   src/webui/app/agents/agent-executor-controller.js PRE-CREATION 
>   src/webui/app/agents/agent-framework-controller.js PRE-CREATION 
>   src/webui/app/agents/agent-task-and-executor-rerouter-controller.js 
> PRE-CREATION 
>   src/webui/app/agents/agents-controller.js PRE-CREATION 
>   src/webui/app/app.js 7de0899c2071c5127881fb725d0dd0246b10f849 
>   src/webui/app/controllers.js 66a4f2955a88fa44c3e5c717e9ce6b680050cc34 
>   src/webui/app/frameworks/framework-controller.js PRE-CREATION 
>   src/webui/app/frameworks/frameworks-controller.js PRE-CREATION 
>   src/webui/app/home.html  
>   src/webui/app/home/home-controller.js PRE-CREATION 
>   src/webui/app/maintenance/maintenance-controller.js PRE-CREATION 
>   src/webui/app/offers/offers-controller.js PRE-CREATION 
>   src/webui/app/roles/roles-controller.js PRE-CREATION 
>   src/webui/app/services.js  
>   src/webui/app/shared/main-controller.js PRE-CREATION 
>   src/webui/app/shared/utils.js PRE-CREATION 
>   src/webui/index.html 149d765429dc2211875783129da6c6b101f38ecf 
>   support/.eslintrc.js cd23ddebd2b40f795fa297e6decacada356a01f8 
> 
> 
> Diff: https://reviews.apache.org/r/66554/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> GTEST_FILTER="" make check -j4
> ```
> And tested manually.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 66545: Added admitted resource providers to the manager's registry.

[Benjamin Bannier]

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

(Updated April 12, 2018, 11:16 a.m.)


Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jan Schlicht.


Changes
---

Removed intended persitence of `ResourceProviderInfo` -- these might be added 
later, but not right now.


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


Repository: mesos


Description
---

Added admitted resource providers to the manager's registry.


Diffs (updated)
-

  src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a 
  src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b 
  src/resource_provider/registry.proto 14bd433ef056f8891e9a38153fdb06c39cee8f62 
  src/tests/resource_provider_manager_tests.cpp 
c52541bf130ccf4795b989b53331176a64a097ea 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66540: Added unit test for recovering nested container without slave state.

[Qian Zhang]

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


Fix it, then Ship it!





src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 1709 (patched)


s/verify/verifies/



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 1709-1710 (patched)


I do not see how the agent metadata is removed in this test, I think it is 
simulated by L1792:
```
AWAIT_READY(containerizer->recover(state));
```

So I would suggest to add a comment for L1792 to mention that we simulate 
agent metadata removed by passing an empty slave state into 
`containerizer->recover()`.


- Qian Zhang


On April 11, 2018, 7:41 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66540/
> ---
> 
> (Updated April 11, 2018, 7:41 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8416
> https://issues.apache.org/jira/browse/MESOS-8416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit test for recovering nested container without slave state.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 796bc401d4f922e7a6bd9e5391003cddd4331c95 
> 
> 
> Diff: https://reviews.apache.org/r/66540/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> Verified that the test crashes without the fix in r/66539.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66539: Fixed the agent recovery crash if metadata is missing.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On April 11, 2018, 7:41 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66539/
> ---
> 
> (Updated April 11, 2018, 7:41 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8416
> https://issues.apache.org/jira/browse/MESOS-8416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is the case that is missed when handling orphan containers
> cleanup. When the agent metadata does not exist but the container
> pid is chechpointed under the container runtime dir, then the
> container should be regarded as orphan and should be cleaned up.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 7473a871e626833733f39375b778aff70529dc63 
> 
> 
> Diff: https://reviews.apache.org/r/66539/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66493: Made FreeBSD default to non-GNU ld.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66493 was successfully built and tested.

Reviews applied: `['66493']`

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

- Mesos Reviewbot Windows


On April 12, 2018, 5:13 a.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66493/
> ---
> 
> (Updated April 12, 2018, 5:13 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
> Bugs: MESOS-8761
> https://issues.apache.org/jira/browse/MESOS-8761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made FreeBSD default to non-GNU ld.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 
> 
> 
> Diff: https://reviews.apache.org/r/66493/diff/3/
> 
> 
> Testing
> ---
> 
> make on FreeBSD, with both lld and gold.
> 
> 
> Thanks,
> 
> David Forsythe
> 
>