Re: Review Request 59194: Validate DESTROY operation in `Resources::apply()`.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/common/resources.cpp
Lines 1417-1419 (patched)


The explanation reads like there's a particular rule that stipulates the 
requirement of "a single copy" but ultimately it just comes down to "if the 
volume is removed", which is self-evident in the code. The error message also 
reminds the reader about the "additional shared copy". I feel it's clear enough 
without the sentence.



src/tests/resources_tests.cpp
Lines 2577 (patched)


`__total`?


- Jiang Yan Xu


On May 12, 2017, 9:50 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59194/
> ---
> 
> (Updated May 12, 2017, 9:50 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7403
> https://issues.apache.org/jira/browse/MESOS-7403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure that applying `DESTROY` operation removes this volume. This is
> needed to handle `DESTROY` of shared volumes to ensure that the
> resultant `Resources` object has this `Resource` removed (i.e. we
> allow this operation to succeed if the original `Resources` object
> had a single copy of the shared resource).
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f6f02eb0a7ca05ab8fd9a670f32428862009b7d5 
>   src/tests/resources_tests.cpp 4cf320d802a749f1419ac5b9f63b6c73b0c974be 
>   src/v1/resources.cpp cad069defb34d5ccc20a0e083eb88cb80f70e415 
> 
> 
> Diff: https://reviews.apache.org/r/59194/diff/2/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 53840: Metric in the allocator to track latency in running allocations.

[Jiang Yan Xu]

> On April 25, 2017, 9:41 a.m., James Peach wrote:
> > src/master/allocator/mesos/metrics.hpp
> > Lines 73 (patched)
> > 
> >
> > Since this metric is measuring the latency from when the allocation was 
> > dispatched to when it executed, how about calling it 
> > `allocation_run_latency`? I find "interval" misleading since it is not the 
> > interval between runs.

+1 on `allocation_run_latency`. A short description could be 

```
// The latency of allocation runs due to the batching of allocation requests.
```


- Jiang Yan


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


On April 21, 2017, 10:53 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53840/
> ---
> 
> (Updated April 21, 2017, 10:53 a.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6579
> https://issues.apache.org/jira/browse/MESOS-6579
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a metric 'allocator/mesos/allocation_run_interval_ms' which
> tracks the latency between scheduling an allocation to an actual
> allocation run.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 051f749dd5921a322ca930a042c31814616d38f9 
>   src/master/allocator/mesos/metrics.hpp 
> ce486eb079dc44bf0bbfad8668426c7ae87c97b3 
>   src/master/allocator/mesos/metrics.cpp 
> ec987fe0ce8605bf9e7c0ccbe3ac384deaf53be1 
>   src/tests/hierarchical_allocator_tests.cpp 
> 33e7b455f8664858eb4f03727b076a10c80cd6e0 
> 
> 
> Diff: https://reviews.apache.org/r/53840/diff/4/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 53840: Metric in the allocator to track latency in running allocations.

[Jiang Yan Xu]

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




src/master/allocator/mesos/hierarchical.cpp
Lines 1425-1435 (original), 1426-1437 (patched)


As James commented, we should also stop the timer even if we return early 
but instead of adding it above `return Nothing()`, it seems appropriate to just 
use `metrics.allocation_run_interval.stop()` as the first statement of the 
method to mark the end of the batching delay.



src/tests/hierarchical_allocator_tests.cpp
Lines 3459-3463 (original), 3459-3462 (patched)


Why this change? Seems like the two ways of expressing the expectation are 
equivalent.



src/tests/hierarchical_allocator_tests.cpp
Lines 3537 (patched)


Seems inelegant to reuse the test for `allocation_run`, for 
`allocation_run` we are storing a full list of statistics in `statistics` but 
we are not doing the same for the new metric. Can we create an independent test?



src/tests/hierarchical_allocator_tests.cpp
Line 3544 (original), 3544 (patched)


Can it realistically be 0.0?


- Jiang Yan Xu


On April 21, 2017, 10:53 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53840/
> ---
> 
> (Updated April 21, 2017, 10:53 a.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6579
> https://issues.apache.org/jira/browse/MESOS-6579
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a metric 'allocator/mesos/allocation_run_interval_ms' which
> tracks the latency between scheduling an allocation to an actual
> allocation run.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 051f749dd5921a322ca930a042c31814616d38f9 
>   src/master/allocator/mesos/metrics.hpp 
> ce486eb079dc44bf0bbfad8668426c7ae87c97b3 
>   src/master/allocator/mesos/metrics.cpp 
> ec987fe0ce8605bf9e7c0ccbe3ac384deaf53be1 
>   src/tests/hierarchical_allocator_tests.cpp 
> 33e7b455f8664858eb4f03727b076a10c80cd6e0 
> 
> 
> Diff: https://reviews.apache.org/r/53840/diff/4/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 59177: CLI: Added Config class to manage the config file.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [59177]

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

- Mesos Reviewbot


On May 15, 2017, 1:59 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59177/
> ---
> 
> (Updated May 15, 2017, 1:59 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This new class simplifies the management of the configuration file
> given by the user; it loads the TOML file on initialization and
> has one method for each element that the user can set.
> 
> This new class and its associated content is also given to the plugins
> at initialization so that they can read the user configuration and use
> it.
> 
> 
> Diffs
> -
> 
>   src/cli_new/bin/main.py 397c120eaaa8f21030dedb1ed552e65c704ee7da 
>   src/cli_new/bin/settings.py 0ef07cc67e8020be0424e939e5b19475fb998ac7 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/config.py PRE-CREATION 
>   src/cli_new/lib/cli/plugins/base.py 
> c10d70fb1b3d232008dd908ea747ec6782a9d47e 
>   src/cli_new/lib/cli/plugins/config/main.py 
> d95a36f4a66c66b4477c6816b7fa5a721f9212f7 
>   src/cli_new/lib/cli/util.py 27c4f17e4b75f63f2fb31f0ad27a464227d29448 
> 
> 
> Diff: https://reviews.apache.org/r/59177/diff/2/
> 
> 
> Testing
> ---
> 
> Tested manually, PEP8 and Pylint used to make sure that the code style is 
> correct.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 59177: CLI: Added Config class to manage the config file.

[Armand Grillet]

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

(Updated May 15, 2017, 1:59 a.m.)


Review request for mesos and Kevin Klues.


Changes
---

Patch modified to resolve the comments I have received offline.


Repository: mesos


Description (updated)
---

This new class simplifies the management of the configuration file
given by the user; it loads the TOML file on initialization and
has one method for each element that the user can set.

This new class and its associated content is also given to the plugins
at initialization so that they can read the user configuration and use
it.


Diffs (updated)
-

  src/cli_new/bin/main.py 397c120eaaa8f21030dedb1ed552e65c704ee7da 
  src/cli_new/bin/settings.py 0ef07cc67e8020be0424e939e5b19475fb998ac7 
  src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
  src/cli_new/lib/cli/config.py PRE-CREATION 
  src/cli_new/lib/cli/plugins/base.py c10d70fb1b3d232008dd908ea747ec6782a9d47e 
  src/cli_new/lib/cli/plugins/config/main.py 
d95a36f4a66c66b4477c6816b7fa5a721f9212f7 
  src/cli_new/lib/cli/util.py 27c4f17e4b75f63f2fb31f0ad27a464227d29448 


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

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


Testing
---

Tested manually, PEP8 and Pylint used to make sure that the code style is 
correct.


Thanks,

Armand Grillet

Re: Review Request 58760: Added default secret resolver module.

[Gilbert Song]

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


Fix it, then Ship it!





src/secret/resolver.cpp
Lines 76 (patched)


Newline above.



src/secret/resolver.cpp
Lines 78 (patched)


Newline above.


- Gilbert Song


On May 12, 2017, 10:52 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58760/
> ---
> 
> (Updated May 12, 2017, 10:52 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7418
> https://issues.apache.org/jira/browse/MESOS-7418
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added default secret resolver module.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 40d921ee7026f5ac47efbf0243e1cafab57825f9 
>   src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a 
>   src/secret/resolver.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/58760/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 58759: Introduced SecretResolver module interface.

[Gilbert Song]

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




include/mesos/secret/resolver.hpp
Lines 26-35 (patched)


Did we try to get rid of the c style comments:
```
/**
 *
 */
```

and insist on c++ ctyle comments:
```
//
//
//
```

ditto to the others.


- Gilbert Song


On May 12, 2017, 10:52 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58759/
> ---
> 
> (Updated May 12, 2017, 10:52 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7418
> https://issues.apache.org/jira/browse/MESOS-7418
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced SecretResolver module interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/module.hpp c28d01df54906b2b8ebfe7ac9e719a0d1be45614 
>   include/mesos/module/secret_resolver.hpp PRE-CREATION 
>   include/mesos/secret/resolver.hpp PRE-CREATION 
>   src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a 
>   src/module/manager.cpp 7d875fcb7fcec0d57274e644b0a3b67b333ac193 
> 
> 
> Diff: https://reviews.apache.org/r/58759/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 58759: Introduced SecretResolver module interface.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On May 12, 2017, 10:52 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58759/
> ---
> 
> (Updated May 12, 2017, 10:52 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7418
> https://issues.apache.org/jira/browse/MESOS-7418
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced SecretResolver module interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/module.hpp c28d01df54906b2b8ebfe7ac9e719a0d1be45614 
>   include/mesos/module/secret_resolver.hpp PRE-CREATION 
>   include/mesos/secret/resolver.hpp PRE-CREATION 
>   src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a 
>   src/module/manager.cpp 7d875fcb7fcec0d57274e644b0a3b67b333ac193 
> 
> 
> Diff: https://reviews.apache.org/r/58759/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>