Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51879, 51880]

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 Sept. 14, 2016, 10:57 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51880/
> ---
> 
> (Updated Sept. 14, 2016, 10:57 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit tests to determine disk size for MOUNT disks.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b6b64bc93980cb81fc50380835865af1f6e4e59f 
>   src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51880/diff/
> 
> 
> Testing
> ---
> 
> All tests including the additional tests in this RR passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Abhishek Dasgupta]

> On Sept. 14, 2016, 8:55 p.m., Vinod Kone wrote:
> > any update on this?

Yeah...working on it..will post patch shortly.


- Abhishek


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


On Sept. 3, 2016, 8:36 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51623/
> ---
> 
> (Updated Sept. 3, 2016, 8:36 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> sThis patch updates mesos-execute to use LAUNCH_GROUP
> for launching task groups. I made some assumption
> in this patch, like newly introduced `--commands`
> option in mesos-execute takes semicolon dimilited values
> which will certainly not be the case in final solution. I
> am open to suggestion what can we use as delimitter for
> `--commands`. In this patch, I kept old `--command` option
> as well and it is working as expected. Though new
> `--commands` should be able to launch single as well
> as multiple tasks and I hope in future we will stick
> to one `commands` option.
> 
> Suggestions are very welcome for further improvement.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp b752d057a3d86482ef1a4baaf31052469e38dc76 
> 
> Diff: https://reviews.apache.org/r/51623/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4
> 
> and manually ran these commands:
> **Successful**
> mesos-execute --master=127.0.0.1:5050 --name="LetsCitar" 
> --containerizer=docker --docker_image="ubuntu" --command="echo hello"
> 
> **Stuck after submitting task to agent as agent code for LAUNCH_GROUP is not 
> yet completed**
> mesos-execute --master=127.0.0.1:5050 --name="LetsCitar" 
> --containerizer=docker --docker_image="ubuntu" --commands="echo hello"
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Review Request 51904: Ensured that queued task groups killed before launch are cleaned up.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Previously, we were not correctly sending TASK_KILLED status updates
for a queued task group if some/all of its tasks were killed before
launch on the agent. Also, we would have still sent the killed task
group erroneously to the executor upon subscribing with the agent
even if it was killed.


Diffs
-

  src/slave/slave.cpp 863ab9d9c9332ac618fd102c096c7aa39343c0fe 
  src/tests/slave_tests.cpp e8b7dc0c1daf11aaf5e917e3a20897712c2983b6 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 51905: Added logging when sending a queued task group to the executor.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

We were not logging the task group before sending the queued
task group to the executor via a `LAUNCH_GROUP` event.


Diffs
-

  src/slave/slave.cpp 863ab9d9c9332ac618fd102c096c7aa39343c0fe 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 51891: Added test case for registry GC race condition.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [51891, 51845, 51805, 51707, 51653, 51706, 51021, 51377, 
51376, 51375, 51374, 51371, 51020, 50846, 50845, 50844, 50707, 50706, 50705, 
50704, 50703, 50702, 50701, 50700, 50699, 50422, 50418, 50417, 50416, 50235]

Failed command: ./support/apply-review.sh -n -r 51375

Error:
2016-09-15 03:57:58 URL:https://reviews.apache.org/r/51375/diff/raw/ 
[8917/8917] -> "51375.patch" [1]
error: patch failed: src/Makefile.am:988
error: src/Makefile.am: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/15253/console

- Mesos ReviewBot


On Sept. 14, 2016, 3:04 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51891/
> ---
> 
> (Updated Sept. 14, 2016, 3:04 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If GC occurs concurrently with another operation that changes the
> registry (e.g., reregistration of an agent that is also going to be
> removed by the GC operation), the GC might not be able to prune as
> many entries as expected from the registry.
> 
> Along the way, this commit updates the master's logging to emit
> a warning in this situation. It also corrects an inaccuracy: we
> should report the number of agents we actually pruned from the
> registry, not the number we _attempted_ to reclaim.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b88472f6350d3f71e057bab34822423da5427151 
>   src/tests/partition_tests.cpp f3142ad8d50daafcdb70ad9dbb2772f8ba30db00 
> 
> Diff: https://reviews.apache.org/r/51891/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 51886: Added tests for default executor.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51884, 51885, 51886]

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 Sept. 14, 2016, 9:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51886/
> ---
> 
> (Updated Sept. 14, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6077
> https://issues.apache.org/jira/browse/MESOS-6077
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a basic test to ensure that the basic executor
> sends a TASK_RUNNING update upon launch.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
>   src/tests/default_executor_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51886/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51780: Changed isolator recover interface using 'ContainerRecoverInfo'.

[haosdent huang]

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




src/slave/containerizer/mesos/isolators/cgroups/cpushare.hpp (line 52)


This patch need to rebase lah.


- haosdent huang


On Sept. 12, 2016, 6:41 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51780/
> ---
> 
> (Updated Sept. 12, 2016, 6:41 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6150
> https://issues.apache.org/jira/browse/MESOS-6150
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the isolator::recover interface to use the new
> protobuf message 'ContainerRecoverInfo'. Please see the previous
> commit message for reasons.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/isolator.hpp d5880eee3c8aee3555630afd21bdd6af5854d11b 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 078ef4f4e7bf5e1522804a720c51cfa5518d8efd 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 1a120f191e4ff0e2b31dd0a9a6bced784a56612c 
>   src/slave/containerizer/mesos/isolator.hpp 
> b94947bc5e83b4f2a2174cb21ed559b4241d2c0a 
>   src/slave/containerizer/mesos/isolator.cpp 
> 253ff3cea8aff3e7a3051fb5a763cc081f455f18 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 38d1428f5425566502747d2a8394e246e0b3fd9e 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 8b6dfde366caf82d30afb891c8f1337ceed12157 
>   src/slave/containerizer/mesos/isolators/cgroups/cpushare.hpp 
> 74982a610b6c0a74734165a0c6aa8c9f72f54deb 
>   src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp 
> 221e814a448c4b5df9dab98de597451a24e2b89c 
>   src/slave/containerizer/mesos/isolators/cgroups/devices.hpp 
> 7ad96440b21a380c5d9af27b0168e9abf47769af 
>   src/slave/containerizer/mesos/isolators/cgroups/devices.cpp 
> f1b5e75d23780c0e1d53487852422b02c88de9e8 
>   src/slave/containerizer/mesos/isolators/cgroups/mem.hpp 
> b3ce6ed2505312bdd2d800164c2f57cd7625c9fa 
>   src/slave/containerizer/mesos/isolators/cgroups/mem.cpp 
> 0a4f38ded5cffd438e2a3b1e7d066c3077557f0d 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> 452eeddcf81232174f00afc1f0ddbd410e4540b4 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> dcbc499a7ac60a7be28cd889abc73155a6c1ac83 
>   src/slave/containerizer/mesos/isolators/cgroups/perf_event.hpp 
> 4abde12af68a26b94b3706cdb38bf9890d811039 
>   src/slave/containerizer/mesos/isolators/cgroups/perf_event.cpp 
> 31f35385691681ef5da14be747edfb5f57c5d05a 
>   src/slave/containerizer/mesos/isolators/docker/runtime.hpp 
> ac0397f65bb2a675ad1eae0f7cfa95f10145fc63 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> b589cd691ae6aacd2dcd00878e43d58f15abfe11 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> 2cc8e764ff18c95c29598df75cdb370ccf120662 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> af9f3736b487b595e8768e56ce60dc4823db28a1 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> 0a85935550e36c9142d845465cfa70a1634a647a 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> ea418252956c8089acc5a491888ed7f6df6cafcd 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
> 794b6e5990db5f8eb21a6535872f284ca02e0553 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> af427c6e5691f1770ab3ebef79502eb2c2176c4a 
>   src/slave/containerizer/mesos/isolators/filesystem/shared.cpp 
> a1283e5ee92c916baaf9fca8ce314d597e8421b3 
>   src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 
> d94f6cf4da9a06cc0da23294c6fab1ca31920149 
>   src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
> 1383ba2dbb2be99b3eca0f2f62386438827e0bd6 
>   src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 
> 1c74ba2561c113c4611577c541b2baed13717ece 
>   src/slave/containerizer/mesos/isolators/namespaces/pid.cpp 
> b41e2665e4c9089da55b38aa5d0bedbc1a60e6a8 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> eea80355d9a12f7b9571c55194ef3ab4931e6aed 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 822f11eab5b00c014563322a8c3b2c14cb440e0b 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
> e852c46a5027c7c911bb7f0c9364ff830f4df086 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 55a06cfe1edbb7699c04101f78cc4979176e33f6 
>   src/slave/containerizer/mesos/isolators/posix.hpp 
> 26c693a9a228d41f38854698496612b063baa68c 
>   src/slave/containerizer/mesos/isolators/posix/disk.hpp 
> 

Re: Review Request 51882: Reverted "Added a test case `ROOT_CGROUPS_CFS_BigQuotaDecimal`.".

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 14, 2016, 3:54 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51882/
> ---
> 
> (Updated Sept. 14, 2016, 3:54 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reverts commit 6cfa4a6f07f61c9b5edc5cce4b778fdf58baa81a.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cgroups_tests.cpp 
> 6ebcebbcdda6b829a91f8e04128063a5f02eb48b 
> 
> Diff: https://reviews.apache.org/r/51882/diff/
> 
> 
> Testing
> ---
> 
> Refer to @kaysoky's comment https://reviews.apache.org/r/51794/#comment216330 
> It is no different between `ROOT_CGROUPS_CFS_Big_Quota` and 
> `ROOT_CGROUPS_CFS_BigQuotaDecimal` if we use `Duration quota = 
> Milliseconds(100500)`.
> 
> To avoid the compile error in clang, we should revert this.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 51881: Fixed failed cgroups isolator test cases in CentOS 6.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 14, 2016, 2:54 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51881/
> ---
> 
> (Updated Sept. 14, 2016, 2:54 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed failed cgroups isolator test cases in CentOS 6.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 31c95c85da54b553b2bba8018a27438381085c99 
> 
> Diff: https://reviews.apache.org/r/51881/diff/
> 
> 
> Testing
> ---
> 
> Tested in CentOS 6 by
> ```
> GLOG_v=1 sudo ./bin/mesos-tests.sh --gtest_filter='CgroupsIsolatorTest.*' 
> --verbose
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 51902: Added protobuf definition for SANDBOX_PATH volume source.

[Joseph Wu]

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


Fix it, then Ship it!




LGTM!

Just a bit of language tweaking... (ditto for the v1 protos).


include/mesos/mesos.proto (lines 1873 - 1874)


How about `Base` or `Type` or `Root`?

I feel `Container` is over-used in this protobuf :)



include/mesos/mesos.proto (line 1881)


I'm guessing upwards traversal (i.e. `../../somewhere`) is not allowed.  
Should we note that here?


- Joseph Wu


On Sept. 14, 2016, 6:35 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51902/
> ---
> 
> (Updated Sept. 14, 2016, 6:35 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6160
> https://issues.apache.org/jira/browse/MESOS-6160
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This new volume source allows sibling containers to share an ephemeral
> volume from their parent container's sandbox. In the future, it'll
> also allow sibling containers to share persistent volume, external
> volume or host volume from their parent.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto eb61b6243202464da2307d06d80700f19f9c25d4 
>   include/mesos/v1/mesos.proto 62231522f4bfddfc6c440a299dd01080cbe25f6a 
> 
> Diff: https://reviews.apache.org/r/51902/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 51902: Added protobuf definition for SANDBOX_PATH volume source.

[Jie Yu]

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

(Updated Sept. 15, 2016, 1:35 a.m.)


Review request for mesos, Gilbert Song, Joseph Wu, and Vinod Kone.


Changes
---

UPdated to use SANDBOX_PATH.


Summary (updated)
-

Added protobuf definition for SANDBOX_PATH volume source.


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


Repository: mesos


Description
---

This new volume source allows sibling containers to share an ephemeral
volume from their parent container's sandbox. In the future, it'll
also allow sibling containers to share persistent volume, external
volume or host volume from their parent.


Diffs (updated)
-

  include/mesos/mesos.proto eb61b6243202464da2307d06d80700f19f9c25d4 
  include/mesos/v1/mesos.proto 62231522f4bfddfc6c440a299dd01080cbe25f6a 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 51715: Added a parallel gtest runner.

[Kevin Klues]

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




support/mesos-gtest-runner.py (line 13)


Can we add some comment (or docstring) at the top here that actually 
describe how to invoke this script? I know other scripts don't necessarily do 
this, but it would be really nice to open this up and immediately see how it 
should be used.



support/mesos-gtest-runner.py (lines 15 - 29)


I'd prefer to see each of the variables in this class not include the whole 
`'if sys.stdout.isatty()'` suffix and instead provide a `COLORIZE` helper that 
does that instead.

i.e.
```
class Bcolors:
HEADER = '\033[95m'
OKBLUE = '\033[94m'
OKGREEN = '\033[92m'
WARNING = '\033[93m'
FAIL = '\033[91m'
BOLD = '\033[1m'
UNDERLINE = '\033[4m'
ENDC = '\033[0m'

@staticmethod
def COLORIZE(string, color_codes):
colors = "".join(color_codes)
return '{}{}{}'.format(colors if sys.stdout.isatty() else '',
   string,
   ENDC if sys.stdout.isatty() else '')
```

This would then be used below as e.g.:
```
Bcolors.COLORIZE('[FAIL]', [Bcolors.FAIL, Bcolors.BOLD])
```
instead of 
```
Bcolors.FAIL + Bcolors.BOLD + '[FAIL]' + Bcolors.ENDC
```



support/mesos-gtest-runner.py (line 32)


This function name doesn't tell me anything. If it is intended to run an 
executable, why not call it `run_executable()` or better `run_test()`?



support/mesos-gtest-runner.py (line 34)


s/acutal/actual



support/mesos-gtest-runner.py (lines 36 - 37)


What is a shard in this context?



support/mesos-gtest-runner.py (line 63)


I would prefer to see this function inline under the `__main__` directive 
below, and instead have a helper function for parsing the args instead of 
inlining that content as done below.



support/mesos-gtest-runner.py (line 67)


Here you call it a 'binary', whereas above you call it an 'executable'. We 
should probably be consistent (and I prefer 'exectuable' because it may be a 
script and not necessarily an actual binary file).



support/mesos-gtest-runner.py (line 70)


At first glance it's not clear to me what this function is used for. Can 
you add a more descriptive comment about what this is used for?

i.e. explain how this is passed to each  subcommand that gets spawned 
asynchronously in order to pass it the proper options



support/mesos-gtest-runner.py (lines 122 - 123)


Sometimes you use `.format()` sometimes you use `"" + ""`.

Can we be more consistent? I prefer `.format()`.



support/mesos-gtest-runner.py (lines 131 - 147)


In the first exception you print then terminate, then exit.

In the second exception you terminate then print then exit.

Is there a reason for the inconsistency? Could we do the operations in the 
same order for more consistency?



support/mesos-gtest-runner.py (line 151)


As mentioned above, I would move all of this stuff into a 
`parse_arguments()` helper function instead of putting the main stuff in a 
helper function as is currently done.



support/mesos-gtest-runner.py (line 154)


How did you come up with this default number of jobs? I would think we 
should spawn slightly *less* jobs than the number of CPUs on the machine (not 
1.5 times more).

Also it's not a big deal since we only have one constant for now, but I 
would prefer to move this line to the top of the file instead of in line here 
(e.g. similar to how we keep all constants at the top of the file in 
support/generate-endpoint-help.py). If we keep it here, I would add a newline 
after it.



support/mesos-gtest-runner.py (line 178)


I typically prefer named format parameters over unnamed ones. If we use the 
COLORIZE helper as suggested above, we can wrap this string in the COLORIZE 
call, and then use a named parameter inline here (as well as all instances 
below).


- Kevin Klues


On Sept. 14, 2016, 12:02 a.m., Benjamin Bannier wrote:
> 
> 

Re: Review Request 51797: Fixed comments closing namespaces in libprocess.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Sept. 14, 2016, 1:32 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51797/
> ---
> 
> (Updated Sept. 14, 2016, 1:32 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/mime.hpp 
> 9d0dd1def4cfc7a047059e96ec61f4904c1c000a 
> 
> Diff: https://reviews.apache.org/r/51797/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51798: Fixed comments closing namespaces.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Sept. 14, 2016, 1:32 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51798/
> ---
> 
> (Updated Sept. 14, 2016, 1:32 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/contender/contender.cpp c11506f6310bf8b40c0acda661d976bb9dff38df 
>   src/master/contender/standalone.cpp 
> 6d6d5277635ef65422b07022198d6a0779da5ab3 
>   src/master/contender/zookeeper.cpp e1a69d3f5a3057761c71cb59c925c35ceba641bd 
> 
> Diff: https://reviews.apache.org/r/51798/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51673: Update mesos containerizer launch for sub-container support.

[Gilbert Song]

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

(Updated Sept. 14, 2016, 5:38 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


Repository: mesos


Description
---

Update mesos containerizer launch for sub-container support.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
89b7e8db38916d69d9b2d4fe305d4397b0859a10 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51871: Modified the `isolate` method to be nesting aware.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51857, 51871]

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 Sept. 14, 2016, 4:25 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51871/
> ---
> 
> (Updated Sept. 14, 2016, 4:25 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network file setup in the `network/cni` isolator is now nesting
> aware. Since the children share the network and UTS namespace with the
> parent, the network files need to be created only for the parent
> container. For the child containers, the network files will be simply
> a symlink to a parents network files.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 822f11eab5b00c014563322a8c3b2c14cb440e0b 
> 
> Diff: https://reviews.apache.org/r/51871/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> sudo ./bin/mesos-tests.sh
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51517: Added MOUNT or PATH disk type in logging resources.

[Jiang Yan Xu]

> On Sept. 14, 2016, 1:59 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, line 1745
> > 
> >
> > It would be great if you could avoid using `default` when switching 
> > over enum values. See https://issues.apache.org/jira/browse/MESOS-3754 for 
> > some background.
> 
> Anindya Sinha wrote:
> Done at both call sites.
> 
> Benjamin Bannier wrote:
> Sorry for being unclear, but I think since these enums are just glorified 
> names for integers you still need to handle unknown values. What about a 
> pattern like this:
> 
> ostream& operator<<(ostream& stream, const 
> Resource::DiskInfo::Source& source)
> {
>   switch (source.type()) {
> case Resource::DiskInfo::Source::MOUNT:
>   return stream << "MOUNT:" + source.mount().root();
> case Resource::DiskInfo::Source::PATH:
>   return stream << "PATH:" + source.path().root();
>   }
> 
>   UNREACHABLE();
> }
> 
> Jiang Yan Xu wrote:
> This style LGTM. Just to clarify, "since these enums are just glorified 
> names" you mean because we haven't turned on `-Wswitch`? In the proposed new 
> style we use `UNREACHABLE();` for readability and not for "unknown values" 
> right?
> 
> Anindya Sinha wrote:
> Updated.
> 
> Benjamin Bannier wrote:
> Thanks for accommodating these last minute requests Anindya.
> 
> In general above pattern could still trigger `UNREACHABLE` at runtime when
> 
> a) a caller forces an uncovered integer into `Type`, e.g., with 
> `source.set_type(static_cast(42));`, or
> b) when new values are added to the `Type` enum, but code switching over 
> values is not recompiled.
> 
> It looks like neither issue is likely for this particular code, but the 
> suggested pattern should be a pretty safe general solution.

OK then `-Wswitch` can only help to some extent I guess. In Mesos code itself 
we would certainly discourage code that would introduce the two above cases. I 
guess it's valuable to mention these in MESOS-3754? In cases where you can't 
directly return from switch cases it seems we cannot completely eliminate the 
need for

```
default:
  UNREACHABLE();
```

when considering the above cases, no?

Of course none of these concerns are applicable to this patch. Just thought you 
should share these points on MESOS-3754 too. :)


- Jiang Yan


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


On Sept. 14, 2016, 3:32 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51517/
> ---
> 
> (Updated Sept. 14, 2016, 3:32 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6060
> https://issues.apache.org/jira/browse/MESOS-6060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We also log the disk type (MOUNT or PATH) for each persistent disk as
> well as the root path for these disks while logging resources. Note
> that this is logged only when source is present, otherwise there is
> no additional content logged.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f841e91b81975a887e64a76a709708ed7946025f 
>   src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
>   src/v1/resources.cpp 4410a8f95ef0ebe6ec7aee38cd47f6f83f863867 
> 
> Diff: https://reviews.apache.org/r/51517/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51885: Added a basic default executor implementation.

[Guangya Liu]

> On 九月 14, 2016, 1:08 p.m., Guangya Liu wrote:
> > How about rename this file as `default_pod_executor.cpp`?
> 
> Vinod Kone wrote:
> we are calling it "pod executor" because this executor will be able to 
> launch pod and non-pods in the future. command executor will be deprecated in 
> favor of this.

got it, thanks.


> On 九月 14, 2016, 1:08 p.m., Guangya Liu wrote:
> > src/launcher/default_executor.cpp, line 203
> > 
> >
> > s/multiple task groups /multiple tasks in task group
> 
> Vinod Kone wrote:
> what anand reads correct to me.

I think that here is not `multiple task groups` but `multiple tasks in one task 
group`?


- Guangya


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


On 九月 14, 2016, 9:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51885/
> ---
> 
> (Updated 九月 14, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6077
> https://issues.apache.org/jira/browse/MESOS-6077
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a basic default executor implementation that
> just sends a TASK_RUNNING update followed by a TASK_FINISHED
> update upon the receipt of a LAUNCH_GROUP event from the executor.
> This allows us to test the entire workflow for now. We can iterate
> upon this to later send `LAUNCH_CONTAINER`/`DESTROY_CONTAINER`
> to the agent to actually launch the tasks in a sub container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
>   src/launcher/default_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51885/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51869: Export "reserved_resources" in the agent HTTP endpoint.

[Anindya Sinha]

> On Sept. 15, 2016, 12:10 a.m., Anindya Sinha wrote:
> >

Small type in description:
s/resource/resources


- Anindya


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


On Sept. 14, 2016, 6:21 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51869/
> ---
> 
> (Updated Sept. 14, 2016, 6:21 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-6085
> https://issues.apache.org/jira/browse/MESOS-6085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also the 'resource' field now exposes the total resources, which is
> consistent with the same field in the master's /slaves endpoint.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 67463105d7fa38b2158a64f6994e3dd353a9fcc7 
> 
> Diff: https://reviews.apache.org/r/51869/diff/
> 
> 
> Testing
> ---
> 
> make check. A new test is added in https://reviews.apache.org/r/51870/.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 51902: Added protobuf definition for ParentContainerSandboxPath volume source.

[Jie Yu]

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

Review request for mesos, Gilbert Song, Joseph Wu, and Vinod Kone.


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


Repository: mesos


Description
---

This new volume source allows sibling containers to share an ephemeral
volume from their parent container's sandbox. In the future, it'll
also allow sibling containers to share persistent volume, external
volume or host volume from their parent.


Diffs
-

  include/mesos/mesos.proto eb61b6243202464da2307d06d80700f19f9c25d4 
  include/mesos/v1/mesos.proto 62231522f4bfddfc6c440a299dd01080cbe25f6a 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 51869: Export "reserved_resources" in the agent HTTP endpoint.

[Anindya Sinha]

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




src/slave/http.cpp (line 985)


Maybe good to add a comment and/or document regarding what the field 
`resources` contains... since it changed from `info.resources()` to 
`info.resources() + checkointed resources applied`.



src/slave/http.cpp (line 986)


Add this new field `reserved_resources` in `string 
Slave::Http::STATE_HELP()`.



src/slave/http.cpp (line 987)


Add this new field `unreserved_resources` in `string 
Slave::Http::STATE_HELP()`.


- Anindya Sinha


On Sept. 14, 2016, 6:21 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51869/
> ---
> 
> (Updated Sept. 14, 2016, 6:21 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-6085
> https://issues.apache.org/jira/browse/MESOS-6085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also the 'resource' field now exposes the total resources, which is
> consistent with the same field in the master's /slaves endpoint.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 67463105d7fa38b2158a64f6994e3dd353a9fcc7 
> 
> Diff: https://reviews.apache.org/r/51869/diff/
> 
> 
> Testing
> ---
> 
> make check. A new test is added in https://reviews.apache.org/r/51870/.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 51868: Expose full reservation info in the agent's http endpoint.

[Anindya Sinha]

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




src/slave/http.cpp (line 986)


Add this new field in the output of help, i.e. `string 
Slave::Http::STATE_HELP()`?


- Anindya Sinha


On Sept. 14, 2016, 6:21 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51868/
> ---
> 
> (Updated Sept. 14, 2016, 6:21 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-4668
> https://issues.apache.org/jira/browse/MESOS-4668
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Expose full reservation info in the agent's http endpoint.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 67463105d7fa38b2158a64f6994e3dd353a9fcc7 
> 
> Diff: https://reviews.apache.org/r/51868/diff/
> 
> 
> Testing
> ---
> 
> make check. A new test is added in https://reviews.apache.org/r/51870/.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 51867: Add a member variable for the agent's total resources.

[Anindya Sinha]

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


Fix it, then Ship it!




Ship It!


src/slave/slave.hpp (line 671)


nitpik: totalResources is the same as `info.resources()` with checkpointed 
resources applied (and nothing else). So, how about:

s/includes/is the same as


- Anindya Sinha


On Sept. 14, 2016, 6:21 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51867/
> ---
> 
> (Updated Sept. 14, 2016, 6:21 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-4668
> https://issues.apache.org/jira/browse/MESOS-4668
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `totalResources` is updated by CheckpointResourcesMessages and doesn't
> have to be re-calculated in Slave::usage(). This change allows us to
> expose `totalResources` for other purposes as well.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp e659c44fcc415f10a6980e7b29cc1ce536dfe657 
>   src/slave/slave.cpp 2c7b5ada1aa81babb9ceee0c9f928685878a778c 
> 
> Diff: https://reviews.apache.org/r/51867/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 51866: Made the agent verify resource compatibility in checkpointResources().

[Anindya Sinha]

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




src/slave/slave.cpp (line 5064)


Why is it verified against the old flag? `Slave::recover()` is the last 
step in `Slave::initialize()` by which time `info.resources()` reflects the 
`--resources` flag in this instance of mesos-slave process. So we are verifying 
with the new flag.


- Anindya Sinha


On Sept. 14, 2016, 6:21 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51866/
> ---
> 
> (Updated Sept. 14, 2016, 6:21 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-4668
> https://issues.apache.org/jira/browse/MESOS-4668
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In handling CheckpointResourcesMessage, the agent should first make
> sure the checkpointed resources are compatible before it syncs local
> disk state.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2c7b5ada1aa81babb9ceee0c9f928685878a778c 
> 
> Diff: https://reviews.apache.org/r/51866/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 51517: Added MOUNT or PATH disk type in logging resources.

[Benjamin Bannier]

> On Sept. 14, 2016, 10:59 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, line 1745
> > 
> >
> > It would be great if you could avoid using `default` when switching 
> > over enum values. See https://issues.apache.org/jira/browse/MESOS-3754 for 
> > some background.
> 
> Anindya Sinha wrote:
> Done at both call sites.
> 
> Benjamin Bannier wrote:
> Sorry for being unclear, but I think since these enums are just glorified 
> names for integers you still need to handle unknown values. What about a 
> pattern like this:
> 
> ostream& operator<<(ostream& stream, const 
> Resource::DiskInfo::Source& source)
> {
>   switch (source.type()) {
> case Resource::DiskInfo::Source::MOUNT:
>   return stream << "MOUNT:" + source.mount().root();
> case Resource::DiskInfo::Source::PATH:
>   return stream << "PATH:" + source.path().root();
>   }
> 
>   UNREACHABLE();
> }
> 
> Jiang Yan Xu wrote:
> This style LGTM. Just to clarify, "since these enums are just glorified 
> names" you mean because we haven't turned on `-Wswitch`? In the proposed new 
> style we use `UNREACHABLE();` for readability and not for "unknown values" 
> right?
> 
> Anindya Sinha wrote:
> Updated.

Thanks for accommodating these last minute requests Anindya.

In general above pattern could still trigger `UNREACHABLE` at runtime when

a) a caller forces an uncovered integer into `Type`, e.g., with 
`source.set_type(static_cast(42));`, or
b) when new values are added to the `Type` enum, but code switching over values 
is not recompiled.

It looks like neither issue is likely for this particular code, but the 
suggested pattern should be a pretty safe general solution.


- Benjamin


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


On Sept. 15, 2016, 12:32 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51517/
> ---
> 
> (Updated Sept. 15, 2016, 12:32 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6060
> https://issues.apache.org/jira/browse/MESOS-6060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We also log the disk type (MOUNT or PATH) for each persistent disk as
> well as the root path for these disks while logging resources. Note
> that this is logged only when source is present, otherwise there is
> no additional content logged.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f841e91b81975a887e64a76a709708ed7946025f 
>   src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
>   src/v1/resources.cpp 4410a8f95ef0ebe6ec7aee38cd47f6f83f863867 
> 
> Diff: https://reviews.apache.org/r/51517/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

[Anindya Sinha]

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

(Updated Sept. 14, 2016, 10:57 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Run the linux based tests for mount disks as root.


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


Repository: mesos


Description
---

Added unit tests to determine disk size for MOUNT disks.


Diffs (updated)
-

  src/Makefile.am b6b64bc93980cb81fc50380835865af1f6e4e59f 
  src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 

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


Testing
---

All tests including the additional tests in this RR passed.


Thanks,

Anindya Sinha

Re: Review Request 51517: Added MOUNT or PATH disk type in logging resources.

[Anindya Sinha]

> On Sept. 14, 2016, 8:59 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, line 1745
> > 
> >
> > It would be great if you could avoid using `default` when switching 
> > over enum values. See https://issues.apache.org/jira/browse/MESOS-3754 for 
> > some background.
> 
> Anindya Sinha wrote:
> Done at both call sites.
> 
> Benjamin Bannier wrote:
> Sorry for being unclear, but I think since these enums are just glorified 
> names for integers you still need to handle unknown values. What about a 
> pattern like this:
> 
> ostream& operator<<(ostream& stream, const 
> Resource::DiskInfo::Source& source)
> {
>   switch (source.type()) {
> case Resource::DiskInfo::Source::MOUNT:
>   return stream << "MOUNT:" + source.mount().root();
> case Resource::DiskInfo::Source::PATH:
>   return stream << "PATH:" + source.path().root();
>   }
> 
>   UNREACHABLE();
> }
> 
> Jiang Yan Xu wrote:
> This style LGTM. Just to clarify, "since these enums are just glorified 
> names" you mean because we haven't turned on `-Wswitch`? In the proposed new 
> style we use `UNREACHABLE();` for readability and not for "unknown values" 
> right?

Updated.


- Anindya


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


On Sept. 14, 2016, 10:32 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51517/
> ---
> 
> (Updated Sept. 14, 2016, 10:32 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6060
> https://issues.apache.org/jira/browse/MESOS-6060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We also log the disk type (MOUNT or PATH) for each persistent disk as
> well as the root path for these disks while logging resources. Note
> that this is logged only when source is present, otherwise there is
> no additional content logged.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f841e91b81975a887e64a76a709708ed7946025f 
>   src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
>   src/v1/resources.cpp 4410a8f95ef0ebe6ec7aee38cd47f6f83f863867 
> 
> Diff: https://reviews.apache.org/r/51517/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51517: Added MOUNT or PATH disk type in logging resources.

[Jiang Yan Xu]

> On Sept. 14, 2016, 1:59 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, line 1745
> > 
> >
> > It would be great if you could avoid using `default` when switching 
> > over enum values. See https://issues.apache.org/jira/browse/MESOS-3754 for 
> > some background.
> 
> Anindya Sinha wrote:
> Done at both call sites.
> 
> Benjamin Bannier wrote:
> Sorry for being unclear, but I think since these enums are just glorified 
> names for integers you still need to handle unknown values. What about a 
> pattern like this:
> 
> ostream& operator<<(ostream& stream, const 
> Resource::DiskInfo::Source& source)
> {
>   switch (source.type()) {
> case Resource::DiskInfo::Source::MOUNT:
>   return stream << "MOUNT:" + source.mount().root();
> case Resource::DiskInfo::Source::PATH:
>   return stream << "PATH:" + source.path().root();
>   }
> 
>   UNREACHABLE();
> }

This style LGTM. Just to clarify, "since these enums are just glorified names" 
you mean because we haven't turned on `-Wswitch`? In the proposed new style we 
use `UNREACHABLE();` for readability and not for "unknown values" right?


- Jiang Yan


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


On Sept. 14, 2016, 3:32 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51517/
> ---
> 
> (Updated Sept. 14, 2016, 3:32 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6060
> https://issues.apache.org/jira/browse/MESOS-6060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We also log the disk type (MOUNT or PATH) for each persistent disk as
> well as the root path for these disks while logging resources. Note
> that this is logged only when source is present, otherwise there is
> no additional content logged.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f841e91b81975a887e64a76a709708ed7946025f 
>   src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
>   src/v1/resources.cpp 4410a8f95ef0ebe6ec7aee38cd47f6f83f863867 
> 
> Diff: https://reviews.apache.org/r/51517/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51517: Added MOUNT or PATH disk type in logging resources.

[Anindya Sinha]

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

(Updated Sept. 14, 2016, 10:32 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Abort if we get to unreachable section of the code.


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


Repository: mesos


Description
---

We also log the disk type (MOUNT or PATH) for each persistent disk as
well as the root path for these disks while logging resources. Note
that this is logged only when source is present, otherwise there is
no additional content logged.


Diffs (updated)
-

  src/common/resources.cpp f841e91b81975a887e64a76a709708ed7946025f 
  src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
  src/v1/resources.cpp 4410a8f95ef0ebe6ec7aee38cd47f6f83f863867 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51798: Fixed comments closing namespaces.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51798]

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 Sept. 14, 2016, 1:32 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51798/
> ---
> 
> (Updated Sept. 14, 2016, 1:32 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/contender/contender.cpp c11506f6310bf8b40c0acda661d976bb9dff38df 
>   src/master/contender/standalone.cpp 
> 6d6d5277635ef65422b07022198d6a0779da5ab3 
>   src/master/contender/zookeeper.cpp e1a69d3f5a3057761c71cb59c925c35ceba641bd 
> 
> Diff: https://reviews.apache.org/r/51798/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51517: Added MOUNT or PATH disk type in logging resources.

[Benjamin Bannier]

> On Sept. 14, 2016, 10:59 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, line 1745
> > 
> >
> > It would be great if you could avoid using `default` when switching 
> > over enum values. See https://issues.apache.org/jira/browse/MESOS-3754 for 
> > some background.
> 
> Anindya Sinha wrote:
> Done at both call sites.

Sorry for being unclear, but I think since these enums are just glorified names 
for integers you still need to handle unknown values. What about a pattern like 
this:

ostream& operator<<(ostream& stream, const Resource::DiskInfo::Source& 
source)
{
  switch (source.type()) {
case Resource::DiskInfo::Source::MOUNT:
  return stream << "MOUNT:" + source.mount().root();
case Resource::DiskInfo::Source::PATH:
  return stream << "PATH:" + source.path().root();
  }

  UNREACHABLE();
}


> On Sept. 14, 2016, 10:59 p.m., Benjamin Bannier wrote:
> > src/v1/resources.cpp, line 1748
> > 
> >
> > It would be great if you could avoid using `default` when switching 
> > over enum values. See https://issues.apache.org/jira/browse/MESOS-3754 for 
> > some background.

Ditto.


- Benjamin


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


On Sept. 14, 2016, 11:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51517/
> ---
> 
> (Updated Sept. 14, 2016, 11:42 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6060
> https://issues.apache.org/jira/browse/MESOS-6060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We also log the disk type (MOUNT or PATH) for each persistent disk as
> well as the root path for these disks while logging resources. Note
> that this is logged only when source is present, otherwise there is
> no additional content logged.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f841e91b81975a887e64a76a709708ed7946025f 
>   src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
>   src/v1/resources.cpp 4410a8f95ef0ebe6ec7aee38cd47f6f83f863867 
> 
> Diff: https://reviews.apache.org/r/51517/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51517: Added MOUNT or PATH disk type in logging resources.

[Anindya Sinha]

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

(Updated Sept. 14, 2016, 9:42 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

We also log the disk type (MOUNT or PATH) for each persistent disk as
well as the root path for these disks while logging resources. Note
that this is logged only when source is present, otherwise there is
no additional content logged.


Diffs (updated)
-

  src/common/resources.cpp f841e91b81975a887e64a76a709708ed7946025f 
  src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
  src/v1/resources.cpp 4410a8f95ef0ebe6ec7aee38cd47f6f83f863867 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51517: Added MOUNT or PATH disk type in logging resources.

[Anindya Sinha]

> On Sept. 14, 2016, 8:59 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, line 1745
> > 
> >
> > It would be great if you could avoid using `default` when switching 
> > over enum values. See https://issues.apache.org/jira/browse/MESOS-3754 for 
> > some background.

Done at both call sites.


- Anindya


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


On Sept. 9, 2016, 4:26 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51517/
> ---
> 
> (Updated Sept. 9, 2016, 4:26 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6060
> https://issues.apache.org/jira/browse/MESOS-6060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We also log the disk type (MOUNT or PATH) for each persistent disk as
> well as the root path for these disks while logging resources. Note
> that this is logged only when source is present, otherwise there is
> no additional content logged.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 77ee92193de1f34131f7a86b7d16731c9c3e17e2 
>   src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
>   src/v1/resources.cpp 4ba5bf87c9b9fef67eeb447a3f3b520f1e81ad77 
> 
> Diff: https://reviews.apache.org/r/51517/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51517: Added MOUNT or PATH disk type in logging resources.

[Jiang Yan Xu]

> On Sept. 14, 2016, 1:59 p.m., Benjamin Bannier wrote:
> >

OK makes sense. Wasn't aware of this. I'll refrain from committing. Anindya 
could you update the review?


- Jiang Yan


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


On Sept. 9, 2016, 9:26 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51517/
> ---
> 
> (Updated Sept. 9, 2016, 9:26 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6060
> https://issues.apache.org/jira/browse/MESOS-6060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We also log the disk type (MOUNT or PATH) for each persistent disk as
> well as the root path for these disks while logging resources. Note
> that this is logged only when source is present, otherwise there is
> no additional content logged.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 77ee92193de1f34131f7a86b7d16731c9c3e17e2 
>   src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
>   src/v1/resources.cpp 4ba5bf87c9b9fef67eeb447a3f3b520f1e81ad77 
> 
> Diff: https://reviews.apache.org/r/51517/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51517: Added MOUNT or PATH disk type in logging resources.

[Benjamin Bannier]

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




src/common/resources.cpp (line 1745)


It would be great if you could avoid using `default` when switching over 
enum values. See https://issues.apache.org/jira/browse/MESOS-3754 for some 
background.



src/v1/resources.cpp (line 1748)


It would be great if you could avoid using `default` when switching over 
enum values. See https://issues.apache.org/jira/browse/MESOS-3754 for some 
background.


- Benjamin Bannier


On Sept. 9, 2016, 6:26 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51517/
> ---
> 
> (Updated Sept. 9, 2016, 6:26 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6060
> https://issues.apache.org/jira/browse/MESOS-6060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We also log the disk type (MOUNT or PATH) for each persistent disk as
> well as the root path for these disks while logging resources. Note
> that this is logged only when source is present, otherwise there is
> no additional content logged.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 77ee92193de1f34131f7a86b7d16731c9c3e17e2 
>   src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
>   src/v1/resources.cpp 4ba5bf87c9b9fef67eeb447a3f3b520f1e81ad77 
> 
> Diff: https://reviews.apache.org/r/51517/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Vinod Kone]

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



any update on this?

- Vinod Kone


On Sept. 3, 2016, 8:36 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51623/
> ---
> 
> (Updated Sept. 3, 2016, 8:36 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> sThis patch updates mesos-execute to use LAUNCH_GROUP
> for launching task groups. I made some assumption
> in this patch, like newly introduced `--commands`
> option in mesos-execute takes semicolon dimilited values
> which will certainly not be the case in final solution. I
> am open to suggestion what can we use as delimitter for
> `--commands`. In this patch, I kept old `--command` option
> as well and it is working as expected. Though new
> `--commands` should be able to launch single as well
> as multiple tasks and I hope in future we will stick
> to one `commands` option.
> 
> Suggestions are very welcome for further improvement.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp b752d057a3d86482ef1a4baaf31052469e38dc76 
> 
> Diff: https://reviews.apache.org/r/51623/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4
> 
> and manually ran these commands:
> **Successful**
> mesos-execute --master=127.0.0.1:5050 --name="LetsCitar" 
> --containerizer=docker --docker_image="ubuntu" --command="echo hello"
> 
> **Stuck after submitting task to agent as agent code for LAUNCH_GROUP is not 
> yet completed**
> mesos-execute --master=127.0.0.1:5050 --name="LetsCitar" 
> --containerizer=docker --docker_image="ubuntu" --commands="echo hello"
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 51517: Added MOUNT or PATH disk type in logging resources.

[Jiang Yan Xu]

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


Ship it!




Will commit with the following edits.


src/common/resources.cpp (lines 1746 - 1747)


Let's just use `UNREACHABLE();` here as it's impossible to be anything else.



src/v1/resources.cpp (lines 1749 - 1750)


Ditto.


- Jiang Yan Xu


On Sept. 9, 2016, 9:26 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51517/
> ---
> 
> (Updated Sept. 9, 2016, 9:26 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6060
> https://issues.apache.org/jira/browse/MESOS-6060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We also log the disk type (MOUNT or PATH) for each persistent disk as
> well as the root path for these disks while logging resources. Note
> that this is logged only when source is present, otherwise there is
> no additional content logged.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 77ee92193de1f34131f7a86b7d16731c9c3e17e2 
>   src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
>   src/v1/resources.cpp 4ba5bf87c9b9fef67eeb447a3f3b520f1e81ad77 
> 
> Diff: https://reviews.apache.org/r/51517/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51797: Fixed comments closing namespaces in libprocess.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51797]

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 Sept. 14, 2016, 1:32 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51797/
> ---
> 
> (Updated Sept. 14, 2016, 1:32 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/mime.hpp 
> 9d0dd1def4cfc7a047059e96ec61f4904c1c000a 
> 
> Diff: https://reviews.apache.org/r/51797/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51375: Introduced MockRegistrar.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/cluster.hpp (line 131)


Can you add a NOTE here regarding why you declared it here? You can use 
most of the text from your review comment.


- Vinod Kone


On Sept. 14, 2016, 10:35 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51375/
> ---
> 
> (Updated Sept. 14, 2016, 10:35 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4049
> https://issues.apache.org/jira/browse/MESOS-4049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows test cases to inspect how the master interacts with
> the registry.
> 
> This commit changes `tests::cluster::Master` to use `MockRegistrar`
> exclusively, even for test cases that aren't interested in mocking
> aspects of the master <-> registrar interaction. However, this should be
> harmless, since by default `MockRegistrar` behaves identically to the
> normal registrar.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
>   src/master/registrar.hpp c39dd1b5430084e51376143b5441db346e85a153 
>   src/tests/cluster.hpp c6fbbf24e04c38cc9a94ce0c13db445c03e804c7 
>   src/tests/cluster.cpp b04653af97d17aaa9d0d3ee872169b66cd67126b 
>   src/tests/mock_registrar.hpp PRE-CREATION 
>   src/tests/mock_registrar.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51375/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 51891: Added test case for registry GC race condition.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 14, 2016, 3:04 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51891/
> ---
> 
> (Updated Sept. 14, 2016, 3:04 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If GC occurs concurrently with another operation that changes the
> registry (e.g., reregistration of an agent that is also going to be
> removed by the GC operation), the GC might not be able to prune as
> many entries as expected from the registry.
> 
> Along the way, this commit updates the master's logging to emit
> a warning in this situation. It also corrects an inaccuracy: we
> should report the number of agents we actually pruned from the
> registry, not the number we _attempted_ to reclaim.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b88472f6350d3f71e057bab34822423da5427151 
>   src/tests/partition_tests.cpp f3142ad8d50daafcdb70ad9dbb2772f8ba30db00 
> 
> Diff: https://reviews.apache.org/r/51891/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 46187: Terminate when receiving the ACK of terminal status update.

[Huadong Liu]

> On Sept. 6, 2016, 11:33 a.m., Vinod Kone wrote:
> > src/launcher/executor.cpp, line 682
> > 
> >
> > hmm. what's the guarantee that an HTTP based executor receives an ACK 
> > within a second? what if the agent is down?
> 
> Qian Zhang wrote:
> If executor does not receives an ACK from agent within 1 second, that 
> means there should be something wrong in agent, so with this code, the 
> executor will exit with -1 and a message so that we can catch such situation. 
> Maybe we should enlarge this timeout a bit (e.g., 3 seconds) to be safer?
> 
> Vinod Kone wrote:
> Executor exiting with -1 code when an agent is down or restarting 
> (probably for an upgrade) seems unfortunate. Since we allow agents to be down 
> for upto "MESOS_RECOVERY_TIMEOUT" (default 15 mins) if "MESOS_CHECKPOINT" is 
> set, maybe the command executor could wait for "MESOS_RECOVERY_TIMEOUT" if it 
> is disconnected? If it is connected then yes, it probably should wait for 
> less time (1s is too short?) and then exit because that's seems like there is 
> something wrong with the agent. Does that make sense?
> 
> Qian Zhang wrote:
> Yes, it makes sense. So what we need to do is:
> 1. For adapter based executor, keep what I am doing now, i.e., in 
> `reaped()` do the self termination with 0 as exit code after 1s.
> 2. For HTTP based executor, in `reaped()`, check if `MESOS_CHECKPOINT` is 
> set:
>2.1 If it is not set, do the self termination with 1 as exit code 
> after 3s (1s is too short).
>2.2 If it is set, check if the executor is disconnected from agent, if 
> yes, do the self termination with 1 as exit code after 
> `MESOS_RECOVERY_TIMEOUT`, if no, do the self termination with 1 as exit code 
> after 3s.
> 
> Please let me know if you have further comments, I will update the patch 
> soon.
> 
> Huadong Liu wrote:
> Hi Qian, Vinod, I have tried 3s in Yelp's mesos/chronos environments. It 
> helped at some extent, but seemed not enough for all the cases. Since the 
> executor will terminate once an ACK is received, why not delay for an 
> extended period of time, say 10s, or even 30s for rare cases (e.g. agent 
> being extermely busy)?
> 
> Vinod Kone wrote:
> For 2.2) what if agent goes down right after executor checks for 
> connectedness in `reaped()`? seems like it would only wait 3s which seems 
> non-ideal. Maybe this logic is becoming too complex? To simplify, how about 
> having the executor always wait for a resonable amount of time (60s) in 
> `reaped()`? HTTP based executor would exit early if they get an ACK for 
> terminal update. How does that sound? We would need to make sure that our 
> unit tests (that use command executor) are not slowed down because of this.
> 
> Qian Zhang wrote:
> > what if agent goes down right after executor checks for connectedness 
> in reaped()?
> 
> Yes, I thought about this case as well, to handle it, it will make the 
> logic here even more complex though the logic of what I posted above is 
> already complex and not that readable. So I agree we should simplify it, what 
> about the following?
> 1. For adapter based executor, in `reaped()` do the self termination with 
> 0 as exit code after 1s.
> 2. For HTTP based executor, in `reaped()`, do the self termination with 1 
> as exit code after 60s (@huadongliu, this should satisfy your requirement :-).
> Or you think we should always wait for 60s for both adapter based 
> executor and HTTP based executor?
> 
> Vinod Kone wrote:
> your plan sounds good to me.

Thanks Qian. I am a bit confused with the terminology here. 

+Is HTTP command executor the same thing as Mesos command executor in master, 
i.e. https://github.com/apache/mesos/blob/master/src/launcher/executor.cpp? 
Chronos uses the Mesos command executor.
+Is the "docker executor" one of the adapter based executors? It uses the 
ExecutorDriver C++ interface. However, 
https://github.com/apache/mesos/blob/master/src/docker/executor.cpp#L451 
comments about HTTP API. Will the comments simply get removed for "1. For 
adapter based executor, in reaped() do the self termination with 0 as exit code 
after 1s."?


- Huadong


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


On Sept. 7, 2016, 2:03 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46187/
> ---
> 
> (Updated Sept. 7, 2016, 2:03 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5276
> https://issues.apache.org/jira/browse/MESOS-5276
> 
> 
> Repository: mesos

Re: Review Request 51864: Added logs for container state transitions.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51865, 51864]

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 Sept. 14, 2016, 11:53 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51864/
> ---
> 
> (Updated Sept. 14, 2016, 11:53 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> dc18e4e3b0eca3f116f1e240217bbebf64a75e3a 
> 
> Diff: https://reviews.apache.org/r/51864/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 51886: Added tests for default executor.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/default_executor_tests.cpp (line 114)


for now, just do

```
Future update1;
Future update2;
EXPECT_CALL(*scheduler, update(_, _))
.WillOnce(FutureArg<1>())
.WillOnce(FutureArg<1>());
```


- Vinod Kone


On Sept. 14, 2016, 9:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51886/
> ---
> 
> (Updated Sept. 14, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6077
> https://issues.apache.org/jira/browse/MESOS-6077
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a basic test to ensure that the basic executor
> sends a TASK_RUNNING update upon launch.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
>   src/tests/default_executor_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51886/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51885: Added a basic default executor implementation.

[Anand Mazumdar]

> On Sept. 14, 2016, 6:04 p.m., Vinod Kone wrote:
> > src/launcher/default_executor.cpp, line 212
> > 
> >
> > why capture the task group as a member variable when you already have 
> > `tasks`?
> 
> Anand Mazumdar wrote:
> `tasks` are a list of unacknowledged tasks i.e. as soon as I receive an 
> acknowledgement for a status update pertaining to that task, we would remove 
> it from `tasks`. Hence, we still need a member variable to store the task 
> group.

I see what you mean now. We can add it later though since we would be needing 
it for `killTask(TaskID)` i.e. we need this information for killing all the 
other tasks in the task group. I would fix it while committing.


- Anand


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


On Sept. 14, 2016, 9:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51885/
> ---
> 
> (Updated Sept. 14, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6077
> https://issues.apache.org/jira/browse/MESOS-6077
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a basic default executor implementation that
> just sends a TASK_RUNNING update followed by a TASK_FINISHED
> update upon the receipt of a LAUNCH_GROUP event from the executor.
> This allows us to test the entire workflow for now. We can iterate
> upon this to later send `LAUNCH_CONTAINER`/`DESTROY_CONTAINER`
> to the agent to actually launch the tasks in a sub container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
>   src/launcher/default_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51885/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51885: Added a basic default executor implementation.

[Anand Mazumdar]

> On Sept. 14, 2016, 6:04 p.m., Vinod Kone wrote:
> > src/launcher/default_executor.cpp, line 212
> > 
> >
> > why capture the task group as a member variable when you already have 
> > `tasks`?

`tasks` are a list of unacknowledged tasks i.e. as soon as I receive an 
acknowledgement for a status update pertaining to that task, we would remove it 
from `tasks`. Hence, we still need a member variable to store the task group.


> On Sept. 14, 2016, 6:04 p.m., Vinod Kone wrote:
> > src/launcher/default_executor.cpp, lines 60-62
> > 
> >
> > for constructors we don't put the first argument on the next line IIRC.
> > 
> > ```
> > DefaultExecutor(arg1,
> > arg2);
> > ```

hmm, that's not entirely true: 
https://github.com/apache/mesos/blob/master/src/master/master.cpp#L285
https://github.com/apache/mesos/blob/master/src/launcher/executor.cpp#L123

We should make it consistent throughout the code though.


- Anand


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


On Sept. 14, 2016, 9:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51885/
> ---
> 
> (Updated Sept. 14, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6077
> https://issues.apache.org/jira/browse/MESOS-6077
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a basic default executor implementation that
> just sends a TASK_RUNNING update followed by a TASK_FINISHED
> update upon the receipt of a LAUNCH_GROUP event from the executor.
> This allows us to test the entire workflow for now. We can iterate
> upon this to later send `LAUNCH_CONTAINER`/`DESTROY_CONTAINER`
> to the agent to actually launch the tasks in a sub container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
>   src/launcher/default_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51885/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51885: Added a basic default executor implementation.

[Vinod Kone]

> On Sept. 14, 2016, 1:08 p.m., Guangya Liu wrote:
> > src/launcher/default_executor.cpp, line 203
> > 
> >
> > s/multiple task groups /multiple tasks in task group

what anand reads correct to me.


- Vinod


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


On Sept. 14, 2016, 9:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51885/
> ---
> 
> (Updated Sept. 14, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6077
> https://issues.apache.org/jira/browse/MESOS-6077
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a basic default executor implementation that
> just sends a TASK_RUNNING update followed by a TASK_FINISHED
> update upon the receipt of a LAUNCH_GROUP event from the executor.
> This allows us to test the entire workflow for now. We can iterate
> upon this to later send `LAUNCH_CONTAINER`/`DESTROY_CONTAINER`
> to the agent to actually launch the tasks in a sub container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
>   src/launcher/default_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51885/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51885: Added a basic default executor implementation.

[Vinod Kone]

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


Fix it, then Ship it!





src/launcher/default_executor.cpp (lines 60 - 62)


for constructors we don't put the first argument on the next line IIRC.

```
DefaultExecutor(arg1,
arg2);
```



src/launcher/default_executor.cpp (line 212)


why capture the task group as a member variable when you already have 
`tasks`?


- Vinod Kone


On Sept. 14, 2016, 9:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51885/
> ---
> 
> (Updated Sept. 14, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6077
> https://issues.apache.org/jira/browse/MESOS-6077
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a basic default executor implementation that
> just sends a TASK_RUNNING update followed by a TASK_FINISHED
> update upon the receipt of a LAUNCH_GROUP event from the executor.
> This allows us to test the entire workflow for now. We can iterate
> upon this to later send `LAUNCH_CONTAINER`/`DESTROY_CONTAINER`
> to the agent to actually launch the tasks in a sub container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
>   src/launcher/default_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51885/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51877: Updated comments for += and -= `Resource_` object.

[Jiang Yan Xu]

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



I didn't add additional comments because the conversion is intentionally 
implicit. I guess we can clarify further by saying "Note that `Resource` 
objects are implicitly converted to `Resource_`" here but this is true 
elsewhere in resources.cpp as well.

- Jiang Yan Xu


On Sept. 13, 2016, 7:03 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51877/
> ---
> 
> (Updated Sept. 13, 2016, 7:03 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The API for += and -= `Resource` object are now removed, we should
> update the comments for  += and -= `Resource_` object by telling
> end user for how to convert a `Resource` object to a `Resource_`
> object when += and -= a `Resource` object.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 7ba422d57d4e058c682df9aa780557782423 
>   include/mesos/v1/resources.hpp add48c7baf8fabf5cc443d60a6c96d2902fc67de 
> 
> Diff: https://reviews.apache.org/r/51877/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 51885: Added a basic default executor implementation.

[Vinod Kone]

> On Sept. 14, 2016, 1:08 p.m., Guangya Liu wrote:
> > How about rename this file as `default_pod_executor.cpp`?

we are calling it "pod executor" because this executor will be able to launch 
pod and non-pods in the future. command executor will be deprecated in favor of 
this.


- Vinod


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


On Sept. 14, 2016, 9:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51885/
> ---
> 
> (Updated Sept. 14, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6077
> https://issues.apache.org/jira/browse/MESOS-6077
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a basic default executor implementation that
> just sends a TASK_RUNNING update followed by a TASK_FINISHED
> update upon the receipt of a LAUNCH_GROUP event from the executor.
> This allows us to test the entire workflow for now. We can iterate
> upon this to later send `LAUNCH_CONTAINER`/`DESTROY_CONTAINER`
> to the agent to actually launch the tasks in a sub container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
>   src/launcher/default_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51885/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51884: Removed tasks belonging to a task group from queued tasks.

[Vinod Kone]

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


Fix it, then Ship it!





src/slave/slave.cpp (line 2212)


s/already/also/



src/slave/slave.cpp (line 2213)


s/also//


- Vinod Kone


On Sept. 14, 2016, 9:33 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51884/
> ---
> 
> (Updated Sept. 14, 2016, 9:33 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When sending the `LAUNCH_GROUP` event to the executor, we were
> not removing the tasks present in the task group from queued
> tasks. This resulted in the tasks being erroneously still
> being present in queued tasks thereby not able to receive
> terminal status updates from the executor.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2c7b5ada1aa81babb9ceee0c9f928685878a778c 
> 
> Diff: https://reviews.apache.org/r/51884/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

[Anindya Sinha]

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

(Updated Sept. 14, 2016, 5:34 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Added include for fs.hpp for linux.


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


Repository: mesos


Description
---

Added unit tests to determine disk size for MOUNT disks.


Diffs (updated)
-

  src/Makefile.am b6b64bc93980cb81fc50380835865af1f6e4e59f 
  src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 

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


Testing
---

All tests including the additional tests in this RR passed.


Thanks,

Anindya Sinha

Review Request 51870: Added a test for reservation info in the agent's endpoint.

[Jiang Yan Xu]

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

Review request for mesos, Anindya Sinha and Neil Conway.


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


Repository: mesos


Description
---

Added a test for reservation info from the agent's endpoint.


Diffs
-

  src/tests/reservation_endpoints_tests.cpp 
bee5ea6b3a3ee7fafb7312a6d89de79d62c57ec1 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Review Request 51869: Export "reserved_resources" in the agent HTTP endpoint.

[Jiang Yan Xu]

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

Review request for mesos, Anindya Sinha and Neil Conway.


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


Repository: mesos


Description
---

Also the 'resource' field now exposes the total resources, which is
consistent with the same field in the master's /slaves endpoint.


Diffs
-

  src/slave/http.cpp 67463105d7fa38b2158a64f6994e3dd353a9fcc7 

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


Testing
---

make check. A new test is added in https://reviews.apache.org/r/51870/.


Thanks,

Jiang Yan Xu

Re: Review Request 51683: Avoided resource validation when flatten resources.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51683]

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 Sept. 14, 2016, 6:09 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51683/
> ---
> 
> (Updated Sept. 14, 2016, 6:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6131
> https://issues.apache.org/jira/browse/MESOS-6131
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> 1) Previously if role or reservation info are invalid,
> the result is an empty Resources object, now it returns
> an error.
> 2) Added a new flatten() method to flatten resources with
> star role and empty reservation.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 7ba422d57d4e058c682df9aa780557782423 
>   include/mesos/v1/resources.hpp add48c7baf8fabf5cc443d60a6c96d2902fc67de 
>   src/cli/execute.cpp c9f56af7f37d5b79b51f350d6c533714c170e889 
>   src/common/resources.cpp f841e91b81975a887e64a76a709708ed7946025f 
>   src/examples/dynamic_reservation_framework.cpp 
> 850bb2a5b243dd5d5a8b6476570b4f943fde6185 
>   src/examples/test_framework.cpp a83766a92617d50eadd92ec55113e049a7290d03 
>   src/examples/test_http_framework.cpp 
> 441e86c88b035d9a268b8b51b95da3e1eb842a62 
>   src/tests/api_tests.cpp e440d1b44fb0b40dd1f68209c71e5ca0cd19f4fb 
>   src/tests/hierarchical_allocator_tests.cpp 
> 7705de95a3916310baf4daca62aab1e6b1ca3cb3 
>   src/tests/mesos.hpp 92d7d94733d461eb0c565830cc1c8e709e7a2ef7 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 266c2a0ff4a99baa96a7c4980f076755603256a9 
>   src/tests/reservation_endpoints_tests.cpp 
> bee5ea6b3a3ee7fafb7312a6d89de79d62c57ec1 
>   src/tests/reservation_tests.cpp 000957826011bf28f7550a83db3e60a796162fb3 
>   src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
>   src/tests/role_tests.cpp 162c9414a70723a212cfd39ac5c0b8325c3f3b5d 
>   src/v1/resources.cpp 4410a8f95ef0ebe6ec7aee38cd47f6f83f863867 
> 
> Diff: https://reviews.apache.org/r/51683/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh  --gtest_filter="ResourcesOperationTest.FlattenResources"
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from ResourcesOperationTest
> [ RUN  ] ResourcesOperationTest.FlattenResources
> [   OK ] ResourcesOperationTest.FlattenResources (3 ms)
> [--] 1 test from ResourcesOperationTest (3 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (24 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 51868: Expose full reservation info in the agent's http endpoint.

[Jiang Yan Xu]

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

Review request for mesos, Anindya Sinha and Neil Conway.


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


Repository: mesos


Description
---

Expose full reservation info in the agent's http endpoint.


Diffs
-

  src/slave/http.cpp 67463105d7fa38b2158a64f6994e3dd353a9fcc7 

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


Testing
---

make check. A new test is added in https://reviews.apache.org/r/51870/.


Thanks,

Jiang Yan Xu

Re: Review Request 51867: Add a member variable for the agent's total resources.

[Jiang Yan Xu]

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

(Updated Sept. 14, 2016, 9:34 a.m.)


Review request for mesos, Anindya Sinha and Neil Conway.


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


Repository: mesos


Description
---

`totalResources` is updated by CheckpointResourcesMessages and doesn't
have to be re-calculated in Slave::usage(). This change allows us to
expose `totalResources` for other purposes as well.


Diffs
-

  src/slave/slave.hpp e659c44fcc415f10a6980e7b29cc1ce536dfe657 
  src/slave/slave.cpp 2c7b5ada1aa81babb9ceee0c9f928685878a778c 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Review Request 51867: Add a member variable for the agent's total resources.

[Jiang Yan Xu]

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

Review request for mesos, Anindya Sinha and Neil Conway.


Repository: mesos


Description
---

`totalResources` is updated by CheckpointResourcesMessages and doesn't
have to be re-calculated in Slave::usage(). This change allows us to
expose `totalResources` for other purposes as well.


Diffs
-

  src/slave/slave.hpp e659c44fcc415f10a6980e7b29cc1ce536dfe657 
  src/slave/slave.cpp 2c7b5ada1aa81babb9ceee0c9f928685878a778c 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Review Request 51866: Made the agent verify resource compatibility in checkpointResources().

[Jiang Yan Xu]

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

Review request for mesos, Anindya Sinha and Neil Conway.


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


Repository: mesos


Description
---

In handling CheckpointResourcesMessage, the agent should first make
sure the checkpointed resources are compatible before it syncs local
disk state.


Diffs
-

  src/slave/slave.cpp 2c7b5ada1aa81babb9ceee0c9f928685878a778c 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 51871: Modified the `isolate` method to be nesting aware.

[Avinash sridharan]

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

(Updated Sept. 14, 2016, 4:25 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.


Changes
---

Addressed Joseph's comemnts.


Summary (updated)
-

Modified the `isolate` method to be nesting aware.


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


Repository: mesos


Description
---

The network file setup in the `network/cni` isolator is now nesting
aware. Since the children share the network and UTS namespace with the
parent, the network files need to be created only for the parent
container. For the child containers, the network files will be simply
a symlink to a parents network files.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
822f11eab5b00c014563322a8c3b2c14cb440e0b 

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


Testing
---

make
make check
sudo ./bin/mesos-tests.sh


Thanks,

Avinash sridharan

Re: Review Request 51857: Modified the `prepare` method to be aware of nested containers.

[Avinash sridharan]

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

(Updated Sept. 14, 2016, 4:23 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.


Changes
---

Addressed Joseph's comemnts.


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


Repository: mesos


Description
---

Modified the `prepare` method to be aware of nested containers.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
822f11eab5b00c014563322a8c3b2c14cb440e0b 

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


Testing
---

make 
make check
and
sudo ./bin/mesos-tests.sh

The only tests that failed were the SUDO make check tests:
[  FAILED  ] 3 tests, listed below:
[  FAILED  ] CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen
[  FAILED  ] CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS
[  FAILED  ] LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume


Thanks,

Avinash sridharan

Re: Review Request 51675: Fixed mesos containerizer recover leak on provisioner orphan destroy.

[Jie Yu]

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



I think we should call 'destroy' on orphans as well.

- Jie Yu


On Sept. 6, 2016, 9:53 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51675/
> ---
> 
> (Updated Sept. 6, 2016, 9:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed mesos containerizer recover leak on provisioner orphan destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 078ef4f4e7bf5e1522804a720c51cfa5518d8efd 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 89b7e8db38916d69d9b2d4fe305d4397b0859a10 
> 
> Diff: https://reviews.apache.org/r/51675/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51673: Update mesos containerizer launch for sub-container support.

[Jie Yu]

> On Sept. 7, 2016, 5:12 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1152-1160
> > 
> >
> > why we need to do that?
> 
> Gilbert Song wrote:
> for the pod case, we have different tasks. and each task may have a 
> rootfs provisioned. we need to pass the rootfs to the pod executor 
> launchtask().
> 
> Jie Yu wrote:
> Really? Can you read the design doc more carefully?

who does the pivot_root? pod executor? or mesos-containerizer launch?


- Jie


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


On Sept. 6, 2016, 9:53 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51673/
> ---
> 
> (Updated Sept. 6, 2016, 9:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update mesos containerizer launch for sub-container support.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 89b7e8db38916d69d9b2d4fe305d4397b0859a10 
> 
> Diff: https://reviews.apache.org/r/51673/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51673: Update mesos containerizer launch for sub-container support.

[Jie Yu]

> On Sept. 7, 2016, 5:12 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1152-1160
> > 
> >
> > why we need to do that?
> 
> Gilbert Song wrote:
> for the pod case, we have different tasks. and each task may have a 
> rootfs provisioned. we need to pass the rootfs to the pod executor 
> launchtask().

Really? Can you read the design doc more carefully?


- Jie


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


On Sept. 6, 2016, 9:53 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51673/
> ---
> 
> (Updated Sept. 6, 2016, 9:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update mesos containerizer launch for sub-container support.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 89b7e8db38916d69d9b2d4fe305d4397b0859a10 
> 
> Diff: https://reviews.apache.org/r/51673/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51621: WIP: Made recovered resource allocated as soon as possible.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51028, 51027, 51621]

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 Sept. 14, 2016, 5:47 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51621/
> ---
> 
> (Updated Sept. 14, 2016, 5:47 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jacob Janco, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3078
> https://issues.apache.org/jira/browse/MESOS-3078
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Please do not review for now as it is WIP.
> 
> Enable the `recoverResources` logic to call `ensureAllocation`,
> this can make sure the `allocationCandidates` can be updated
> as soon as possible and allocate the resources as soon as possible
> if there are no request in the message queue.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 992031a6fbc89727725071841bd3ab827737d8bd 
>   src/tests/hierarchical_allocator_tests.cpp 
> 7705de95a3916310baf4daca62aab1e6b1ca3cb3 
>   src/tests/master_allocator_tests.cpp 
> 7eed672be14f116d0aa5058da98a1db3a146dad9 
>   src/tests/persistent_volume_tests.cpp 
> d07ff1fd2f14c73c924a07ce3850bf0d9de2135a 
>   src/tests/reservation_endpoints_tests.cpp 
> bee5ea6b3a3ee7fafb7312a6d89de79d62c57ec1 
>   src/tests/reservation_tests.cpp 000957826011bf28f7550a83db3e60a796162fb3 
>   src/tests/slave_tests.cpp 2f3fa5fae634b6250f3c00bbef3077493f79af95 
> 
> Diff: https://reviews.apache.org/r/51621/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="*PersistentVolumeTest.BadACL*/*" --verbose --gtest_repeat=20
> GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="*PersistentVolumeTest.GoodACL*/*" --verbose --gtest_repeat=20
> GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="ReservationEndpointsTest.UnreserveAvailableAndOfferedResources"
>   --gtest_repeat=20
> GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="ReservationEndpointsTest.ReserveAvailableAndOfferedResources" 
>  --gtest_repeat=20
> GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="ReservationTest.ACLMultipleOperations" --verbose  
> --gtest_repeat=20
> GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="MasterAllocatorTest/1.RebalancedForUpdatedWeights" 
> --gtest_repeat=30
> GLOG_v=1 ./bin/mesos-tests.sh  
> --gtest_filter="HierarchicalAllocatorTest.UpdateWeight" --verbose 
> --gtest_repeat=20
> GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="MasterAllocatorTest/*.SlaveLost" --gtest_repeat=20
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 51891: Added test case for registry GC race condition.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

If GC occurs concurrently with another operation that changes the
registry (e.g., reregistration of an agent that is also going to be
removed by the GC operation), the GC might not be able to prune as
many entries as expected from the registry.

Along the way, this commit updates the master's logging to emit
a warning in this situation. It also corrects an inaccuracy: we
should report the number of agents we actually pruned from the
registry, not the number we _attempted_ to reclaim.


Diffs
-

  src/master/master.cpp b88472f6350d3f71e057bab34822423da5427151 
  src/tests/partition_tests.cpp f3142ad8d50daafcdb70ad9dbb2772f8ba30db00 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 51625: Refactored `GET_CONTAINERS` Call in v1 Agent API.

[haosdent huang]

> On Sept. 12, 2016, 8:19 p.m., Vinod Kone wrote:
> > Can you separate out filtering of containers and the refactor into separate 
> > commits? Ideally do filtering first.
> > 
> > Also, what's the motivation for including containers in GetState? I 
> > couldn't get the motivation from the linked ticket.

hi, @vinodkone, thanks for your review. At first I try to include 
`GetContainers` in the `GetState` of agent just as we include `GetAgents` in 
the `GetState` of master.
But now looks like `GetContainers` is a heavy operation, include it in 
`GetState` may be what user do not need. Only the other hand, user always could 
get it by `GET_CONTAINERS`. How about let me discard this and 
https://reviews.apache.org/r/51626/?

For filtering out containers by `frameworksApprover` and `executorsApprover`, I 
think need to add it to the `/containers` endpoint as well. Let me post a 
separate patch for it.


- haosdent


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


On Sept. 12, 2016, 4:50 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51625/
> ---
> 
> (Updated Sept. 12, 2016, 4:50 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jay Guo, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-6124
> https://issues.apache.org/jira/browse/MESOS-6124
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add a `_getContainers` method to return the containers information in
> Operator API v1 protobuf format and switch `getContainers` to use it
> directly. Then it would be easier to show the containers
> information in the response of `getState` by reusing `_getContainers`.
> Additionally, add supporting to filter containers by `principal` as
> well.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.cpp 9ae720cdc22c6bcdd56bc6bb391419c52df446f3 
>   src/slave/http.cpp 2cbf3c57e294f0d47d64915371b4a6bbf9cdfc0a 
>   src/slave/slave.hpp 4add4c0180ea56039e0d5009bad4d9346128bde6 
> 
> Diff: https://reviews.apache.org/r/51625/diff/
> 
> 
> Testing
> ---
> 
> ```
> sudo GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="*AgentAPITest.GetContainers*" --verbose
> [   OK ] ContentType/AgentAPITest.GetContainers/1 (85 ms)
> [--] 2 tests from ContentType/AgentAPITest (263 ms total)
> 
> [--] Global test environment tear-down
> [==] 2 tests from 1 test case ran. (284 ms total)
> [  PASSED  ] 2 tests.
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 51886: Added tests for default executor.

[Guangya Liu]

> On 九月 14, 2016, 1:41 p.m., Guangya Liu wrote:
> > src/tests/default_executor_tests.cpp, lines 119-123
> > 
> >
> > How about adding another task to the task group?

```
v1::TaskInfo taskInfo1 =
  evolve(createTask(slaveId, resources, "sleep 1000"));

v1::TaskInfo taskInfo2 =
  evolve(createTask(slaveId, resources, "sleep 1000"));

v1::TaskGroupInfo taskGroup;
taskGroup.add_tasks()->CopyFrom(taskInfo1);
taskGroup.add_tasks()->CopyFrom(taskInfo2);
```


- Guangya


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


On 九月 14, 2016, 9:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51886/
> ---
> 
> (Updated 九月 14, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6077
> https://issues.apache.org/jira/browse/MESOS-6077
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a basic test to ensure that the basic executor
> sends a TASK_RUNNING update upon launch.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
>   src/tests/default_executor_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51886/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51886: Added tests for default executor.

[Guangya Liu]

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




src/tests/default_executor_tests.cpp (lines 119 - 123)


How about adding another task to the task group?


- Guangya Liu


On 九月 14, 2016, 9:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51886/
> ---
> 
> (Updated 九月 14, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6077
> https://issues.apache.org/jira/browse/MESOS-6077
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a basic test to ensure that the basic executor
> sends a TASK_RUNNING update upon launch.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
>   src/tests/default_executor_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51886/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 51797: Fixed comments closing namespaces in libprocess.

[Benjamin Bannier]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/include/process/mime.hpp 
9d0dd1def4cfc7a047059e96ec61f4904c1c000a 

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


Testing
---

make


Thanks,

Benjamin Bannier

Review Request 51798: Fixed comments closing namespaces.

[Benjamin Bannier]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/contender/contender.cpp c11506f6310bf8b40c0acda661d976bb9dff38df 
  src/master/contender/standalone.cpp 6d6d5277635ef65422b07022198d6a0779da5ab3 
  src/master/contender/zookeeper.cpp e1a69d3f5a3057761c71cb59c925c35ceba641bd 

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


Testing
---

make


Thanks,

Benjamin Bannier

Re: Review Request 51885: Added a basic default executor implementation.

[Guangya Liu]

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



How about rename this file as `default_pod_executor.cpp`?


src/launcher/default_executor.cpp (lines 20 - 22)


How about adjust the order as following?

```
#include 

#include 
```



src/launcher/default_executor.cpp (line 57)


How about rename this as `DefaultPodExecutor`?



src/launcher/default_executor.cpp (line 203)


s/multiple task groups /multiple tasks in task group


- Guangya Liu


On 九月 14, 2016, 9:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51885/
> ---
> 
> (Updated 九月 14, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6077
> https://issues.apache.org/jira/browse/MESOS-6077
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a basic default executor implementation that
> just sends a TASK_RUNNING update followed by a TASK_FINISHED
> update upon the receipt of a LAUNCH_GROUP event from the executor.
> This allows us to test the entire workflow for now. We can iterate
> upon this to later send `LAUNCH_CONTAINER`/`DESTROY_CONTAINER`
> to the agent to actually launch the tasks in a sub container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
>   src/launcher/default_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51885/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51882: Reverted "Added a test case `ROOT_CGROUPS_CFS_BigQuotaDecimal`.".

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51878, 51881, 51882]

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 Sept. 14, 2016, 3:54 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51882/
> ---
> 
> (Updated Sept. 14, 2016, 3:54 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reverts commit 6cfa4a6f07f61c9b5edc5cce4b778fdf58baa81a.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cgroups_tests.cpp 
> 6ebcebbcdda6b829a91f8e04128063a5f02eb48b 
> 
> Diff: https://reviews.apache.org/r/51882/diff/
> 
> 
> Testing
> ---
> 
> Refer to @kaysoky's comment https://reviews.apache.org/r/51794/#comment216330 
> It is no different between `ROOT_CGROUPS_CFS_Big_Quota` and 
> `ROOT_CGROUPS_CFS_BigQuotaDecimal` if we use `Duration quota = 
> Milliseconds(100500)`.
> 
> To avoid the compile error in clang, we should revert this.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 51864: Added logs for container state transitions.

[Alexander Rukletsov]

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

(Updated Sept. 14, 2016, 11:53 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
dc18e4e3b0eca3f116f1e240217bbebf64a75e3a 

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


Testing
---


Thanks,

Alexander Rukletsov

Re: Review Request 51865: Added validation for `ContainerInfo`.

[Alexander Rukletsov]

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

(Updated Sept. 14, 2016, 11:52 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/validation.cpp aa743d730bebebc0353fa0a7a31f68bc94e7e25d 
  src/tests/master_validation_tests.cpp 
16c5773aa44016f923e00cb348ded6b8c46d4b4b 

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


Testing
---

make check on Mac OS


Thanks,

Alexander Rukletsov

Re: Review Request 51865: Added validation for `ContainerInfo`.

[Alexander Rukletsov]

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

(Updated Sept. 14, 2016, 11:34 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/validation.cpp 137b9f912029d6b0287b49541d1af9bb1fdb80b3 

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


Testing
---

make check on Mac OS


Thanks,

Alexander Rukletsov

Re: Review Request 51865: Added validation for `ContainerInfo`.

[Alexander Rukletsov]

> On Sept. 13, 2016, 11:30 p.m., Gilbert Song wrote:
> > src/master/validation.cpp, lines 61-63
> > 
> >
> > This is not necessary, because it is a required field.

That's correct, however, due to 
https://issues.apache.org/jira/browse/MESOS-4997 we may change all required 
enums to optional soon. This code already does what it should.


- Alexander


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


On Sept. 13, 2016, 10:47 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51865/
> ---
> 
> (Updated Sept. 13, 2016, 10:47 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 137b9f912029d6b0287b49541d1af9bb1fdb80b3 
> 
> Diff: https://reviews.apache.org/r/51865/diff/
> 
> 
> Testing
> ---
> 
> make check on Mac OS
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 51864: Added logs for container state transitions.

[Alexander Rukletsov]

> On Sept. 13, 2016, 10:54 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 903-904
> > 
> >
> > We already have such `LOG(INFO)` in provisioner::_provisioner().
> > 
> > Why do we add this here?

We may not see that line if container type is not `MESOS`.

Regardless, I see value in explicitly printing at what stage the container is 
to facilitate triaging.


On Sept. 13, 2016, 10:54 p.m., Alexander Rukletsov wrote:
> > And each isolator should already have similar log info message as well. 
> > This would make it duplicate.

I have a log where the container is sruck somewhere before any isolator printed 
something.


- Alexander


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


On Sept. 13, 2016, 10:47 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51864/
> ---
> 
> (Updated Sept. 13, 2016, 10:47 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> dc18e4e3b0eca3f116f1e240217bbebf64a75e3a 
> 
> Diff: https://reviews.apache.org/r/51864/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 51501: Exposed metrics in scheduler library.

[Abhishek Dasgupta]

> On Sept. 14, 2016, 12:26 a.m., Anand Mazumdar wrote:
> > src/scheduler/scheduler.cpp, line 744
> > 
> >
> > hmm, this won't quite work for the scheduler library i.e. the messages 
> > metric would always be 0.
> > 
> > The library uses the `Connection` abstraction that wraps a socket under 
> > the hood and it does not go through the libprocess `MessageEvent` handler 
> > route. 
> > 
> > A possible solution might be to add the metric for messages directly in 
> > the `Connection` abstraction itself?

Yeah, message_queue metric has value of 0 even in case of sched.cpp. i didn't 
find any MessageEvent is getting used in scheduler library/driver. May be I am 
wrong. But message_queue is measured in old scheduler driver as well so I 
decided to introduce it though it is having 0 value. 

So, as per your comment, as we are sending subscribe.send() call and 
non-subscribe.send() call in scheduler library, we can initiate a counter 
metrics in Connection and increment it in Connection::send(). Is this the 
approach we are going to follow?

Also, as message_queue metric is giving 0 value for scheduler driver as well, 
are we going to introduce some new logics to collect message metrics there??

I will raise new issues based on your answer.


- Abhishek


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


On Sept. 6, 2016, 8:53 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51501/
> ---
> 
> (Updated Sept. 6, 2016, 8:53 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6080
> https://issues.apache.org/jira/browse/MESOS-6080
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Exposed metrics scheduler/event_queue_messages(size of
> message queue) and scheduler/event_queue_dispatches
> (size of dispatch queue) in scheduler library.
> 
> 
> Diffs
> -
> 
>   src/scheduler/scheduler.cpp 276ed10bd439c4a7830447bec5053292fb2ca4e7 
>   src/tests/scheduler_tests.cpp 931c1857f1ab454c67eece2f34c4649a426178de 
> 
> Diff: https://reviews.apache.org/r/51501/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04
> sudo GTEST_FILTER="*SchedulerTest.MetricsEndpoint*" make -j4 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 51375: Introduced MockRegistrar.

[Neil Conway]

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



More detail on the recent fix: the bug arose because I reordered the fields of 
`cluster::Master`. The order in which the fields are declared should match the 
order in which the fields are initialized in `cluster::Master`'s ctor. This is 
because the fields have dependencies on one another (registrar depends on 
`storage`, which depends on `log`, etc.). We shouldn't destruct a field if any 
of the fields that depend on it are still alive.

I fixed this by keeping the field order the same, and adding a comment to 
document this behavior. We might alternatively use explicit `reset` calls in 
the dtor of `cluster::Master`; in that case, the order of the `reset`s should 
be the reverse of the order in which the fields are initialized in the ctor. 
That is also fragile, however.

- Neil Conway


On Sept. 14, 2016, 10:35 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51375/
> ---
> 
> (Updated Sept. 14, 2016, 10:35 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4049
> https://issues.apache.org/jira/browse/MESOS-4049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows test cases to inspect how the master interacts with
> the registry.
> 
> This commit changes `tests::cluster::Master` to use `MockRegistrar`
> exclusively, even for test cases that aren't interested in mocking
> aspects of the master <-> registrar interaction. However, this should be
> harmless, since by default `MockRegistrar` behaves identically to the
> normal registrar.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
>   src/master/registrar.hpp c39dd1b5430084e51376143b5441db346e85a153 
>   src/tests/cluster.hpp c6fbbf24e04c38cc9a94ce0c13db445c03e804c7 
>   src/tests/cluster.cpp b04653af97d17aaa9d0d3ee872169b66cd67126b 
>   src/tests/mock_registrar.hpp PRE-CREATION 
>   src/tests/mock_registrar.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51375/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 51375: Introduced MockRegistrar.

[Neil Conway]

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

(Updated Sept. 14, 2016, 10:35 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Fix bug: possible crash on destruction of `cluster::Master`.


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


Repository: mesos


Description
---

This allows test cases to inspect how the master interacts with
the registry.

This commit changes `tests::cluster::Master` to use `MockRegistrar`
exclusively, even for test cases that aren't interested in mocking
aspects of the master <-> registrar interaction. However, this should be
harmless, since by default `MockRegistrar` behaves identically to the
normal registrar.


Diffs (updated)
-

  src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
  src/master/registrar.hpp c39dd1b5430084e51376143b5441db346e85a153 
  src/tests/cluster.hpp c6fbbf24e04c38cc9a94ce0c13db445c03e804c7 
  src/tests/cluster.cpp b04653af97d17aaa9d0d3ee872169b66cd67126b 
  src/tests/mock_registrar.hpp PRE-CREATION 
  src/tests/mock_registrar.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 51856: Updated agent to correctly populate `CommandInfo` for default executor.

[Guangya Liu]

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


Fix it, then Ship it!




Ship It!


src/slave/slave.hpp (line 1022)


s/string/std::string


- Guangya Liu


On 九月 13, 2016, 9:20 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51856/
> ---
> 
> (Updated 九月 13, 2016, 9:20 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6151
> https://issues.apache.org/jira/browse/MESOS-6151
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds the relevant logic to ensure that agent correctly
> populates the command info for default executors. Since, we
> don't yet have the executable `mesos-default-execute`, added
> a `TODO` to the test to add a check to ensure the command
> has that information.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp 0ad59adc1d44014acd756b87d572e835c6325b5f 
>   src/slave/slave.hpp e659c44fcc415f10a6980e7b29cc1ce536dfe657 
>   src/slave/slave.cpp 2c7b5ada1aa81babb9ceee0c9f928685878a778c 
>   src/tests/slave_tests.cpp 2f3fa5fae634b6250f3c00bbef3077493f79af95 
> 
> Diff: https://reviews.apache.org/r/51856/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 51884: Removed tasks belonging to a task group from queued tasks.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

When sending the `LAUNCH_GROUP` event to the executor, we were
not removing the tasks present in the task group from queued
tasks. This resulted in the tasks being erroneously still
being present in queued tasks thereby not able to receive
terminal status updates from the executor.


Diffs
-

  src/slave/slave.cpp 2c7b5ada1aa81babb9ceee0c9f928685878a778c 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 51886: Added tests for default executor.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This change adds a basic test to ensure that the basic executor
sends a TASK_RUNNING update upon launch.


Diffs
-

  src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
  src/tests/default_executor_tests.cpp PRE-CREATION 

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


Testing
---


Thanks,

Anand Mazumdar

Review Request 51885: Added a basic default executor implementation.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This change adds a basic default executor implementation that
just sends a TASK_RUNNING update followed by a TASK_FINISHED
update upon the receipt of a LAUNCH_GROUP event from the executor.
This allows us to test the entire workflow for now. We can iterate
upon this to later send `LAUNCH_CONTAINER`/`DESTROY_CONTAINER`
to the agent to actually launch the tasks in a sub container.


Diffs
-

  src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
  src/launcher/default_executor.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 46028: Improved comments in SocketManager::next().

[Joris Van Remoortere]

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



Closing this review due to inactivity. Please see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On June 16, 2016, 8:55 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46028/
> ---
> 
> (Updated June 16, 2016, 8:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved comments in SocketManager::next().
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> b4cdba6b0cd4f8f373f37118cd2e9d4955f2425a 
> 
> Diff: https://reviews.apache.org/r/46028/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 51877: Updated comments for += and -= `Resource_` object.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51877]

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 Sept. 14, 2016, 2:03 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51877/
> ---
> 
> (Updated Sept. 14, 2016, 2:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The API for += and -= `Resource` object are now removed, we should
> update the comments for  += and -= `Resource_` object by telling
> end user for how to convert a `Resource` object to a `Resource_`
> object when += and -= a `Resource` object.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 7ba422d57d4e058c682df9aa780557782423 
>   include/mesos/v1/resources.hpp add48c7baf8fabf5cc443d60a6c96d2902fc67de 
> 
> Diff: https://reviews.apache.org/r/51877/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45673: PoC: Docker Volume Isolator.

[Joris Van Remoortere]

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



Closing this review due to inactivity. Please see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On April 19, 2016, 4:45 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45673/
> ---
> 
> (Updated April 19, 2016, 4:45 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> PoC: Docker Volume Isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt aabb33dfd4b985fe5774f87bb63b4ec1ec853962 
>   src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac 
>   src/cli/execute.cpp 087a73143d739f41638b8335c7e24dfcd14bc7fb 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 1e1a36903f4377497bb72b69e4ead63675d453c0 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> 786f917c0ec04b6839bbd524fa7c8de3729f9bdb 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> 0ad473dc3bd45e122fba55a670e1a893e61c977a 
>   src/slave/containerizer/mesos/isolators/docker/volume/paths.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/paths.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/state.hpp 
> PRE-CREATION 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
> 
> Diff: https://reviews.apache.org/r/45673/diff/
> 
> 
> Testing
> ---
> 
> state.proto
> 
> message DockerVolumeMount {
>   required string volume_driver = 1;
>   required string volume_name = 2;
> }
> 
> 
> message DockerVolumeMountList {
>   repeated DockerVolumeMount mounts = 1;
> }
> 
> 
> message DockerVolumeInfo {
>   // Docker volume mount info for volume driver and volume name.
>   required DockerVolumeMount dvm = 1;
> 
>   // Container path, this is optional as this is not needed by recover()
>   // but only for prepare() .
>   optional string container_path = 2;
> 
>   // Mount point for the container, this is optiona as this is only needed
>   // by prepare().
>   optional string mount_point = 3;
>   
>   // Volume driver mount options.
>   optional Parameters driver_options = 4;
> }
> 
> DockerVolumeMount will be checkpointed.
> 
> 
> //   /var/run/mesos/isolators/docker/volume
> //|- /
> //|  |-- DockerVolumeMountList
> //|-- /
> //|  |-- DockerVolumeMountList
> //|-/
> //|- ...
> 
> Info structure
> We assume that one container do not use multiple same volume driver and 
> volume name pair.
> struct Info
> {
>   Info (const list& _volumeInfos)
> : volumeInfos(_volumeInfos) {}
> 
>   list volumeInfos;
> };
> 
> Hashmap in isolator:
> hashmap infos;
> hashmap volumeReference; // string is 
> volume_driver::volume_name, e.g. convoy:dvd1
> 
> Cleanup()
> When cleanup, check the container’s mount point reference counter, if it is 
> greater than 1, do not remove the mount point; If it is 1, unmount the mount 
> point.
> driver.cpp::mount
> We need to transfer the whole mount info to mount() as parameter to make sure 
> that the container path can get its own related mount point. If the mount() 
> only return a string, then we cannot get the relationship of container path 
> and mount point.
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 40545: MESOS-3970: fix cpu usage in mesos ui

[Joris Van Remoortere]

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



@Ian can you please re-open if this is still an issue? I could not reproduce.
Closing this review due to inactivity. Please see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On Sept. 14, 2016, 9:16 a.m., Ian Babrou wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40545/
> ---
> 
> (Updated Sept. 14, 2016, 9:16 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3970
> https://issues.apache.org/jira/browse/MESOS-3970
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3970: fix cpu usage in mesos ui
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/services.js 
> d41bc7142ae0182e25b2ae9dce960ee38ab22361 
> 
> Diff: https://reviews.apache.org/r/40545/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Babrou
> 
>

Re: Review Request 40545: MESOS-3970: fix cpu usage in mesos ui

[Ian Babrou]

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

(Updated Sept. 14, 2016, 9:16 a.m.)


Review request for mesos.


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


Repository: mesos


Description
---

MESOS-3970: fix cpu usage in mesos ui


Diffs
-

  src/webui/master/static/js/services.js 
d41bc7142ae0182e25b2ae9dce960ee38ab22361 

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


Testing
---


Thanks,

Ian Babrou

Re: Review Request 46799: Added capabilities support to mesos-execute.

[Joris Van Remoortere]

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



Closing this review as Benjamin has followed up with separate reviews. Please 
see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On May 13, 2016, 4:46 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46799/
> ---
> 
> (Updated May 13, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two flags: `user` and `capabilities`.
> 
>user:  used to specify the user name for the command.
>capabilities: comma separated list of capabilities the task
>  requires.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 
> 
> Diff: https://reviews.apache.org/r/46799/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 46370: Introduced linux capabilities API.

[Joris Van Remoortere]

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



Closing this review as Benjamin has followed up with separate reviews. Please 
see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On May 26, 2016, 3:01 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> ---
> 
> (Updated May 26, 2016, 3:01 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-5051
> https://issues.apache.org/jira/browse/MESOS-5051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces basic API for linux capabilities. This is not a
> comprehensive API but is strictly limited to the need for securing Mesos
> containers using linux capabilities.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
>   src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
>   src/linux/capabilities.hpp PRE-CREATION 
>   src/linux/capabilities.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46370/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 46371: Added basic tests for capabilities API.

[Joris Van Remoortere]

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



Closing this review as Benjamin has followed up with separate reviews. Please 
see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On May 26, 2016, 3:01 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46371/
> ---
> 
> (Updated May 26, 2016, 3:01 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added basic tests for capabilities API.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
>   src/tests/capabilities_test_helper.hpp PRE-CREATION 
>   src/tests/capabilities_test_helper.cpp PRE-CREATION 
>   src/tests/capabilities_test_helper_main.cpp PRE-CREATION 
>   src/tests/capabilities_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46371/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Joris Van Remoortere]

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



Closing this review as Benjamin has followed up with separate reviews. Please 
see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On June 26, 2016, 8:23 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated June 26, 2016, 8:23 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gilbert Song, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added templatized implementations for digests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 404a7438e7cd53d9295fe6025b5af5fb83df939b 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> 99c9754b6a5c7ceb12808a782da9cb9fb3fc731e 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

[Joris Van Remoortere]

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



Closing this review as Benjamin has follow up with separate reviews. Please see 
our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On May 26, 2016, 3:03 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46798/
> ---
> 
> (Updated May 26, 2016, 3:03 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces linux capability based security for unified
> containerizer. A new agent flag \`allowed_capabilities\` has been
> introduced to override the default capabilities of the user or the
> capabilities requested by the user.
> 
> This feature is only available on linux.
> 
> 
> Diffs
> -
> 
>   src/common/parse.hpp 19e56dbeb765f8bec92e0a3615f6f7c12466fa9e 
>   src/launcher/executor.cpp 7d111e668e0a139a98bdeb959997843180b40452 
>   src/slave/containerizer/mesos/containerizer.hpp 
> a1a00020668f6da8d611f26e5637afffc87d09ba 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 75e5a32a3e70ec60a6800e21a621673184ea0956 
>   src/slave/containerizer/mesos/launch.hpp 
> c716e0396736d1f2f60ec31540f12f4f7597d081 
>   src/slave/containerizer/mesos/launch.cpp 
> e22106b014c871e2184a15c2ab154a0674874e47 
>   src/slave/flags.hpp 80ba2887448e91c40ae68fc2d9f0c0bee1a49f48 
>   src/slave/flags.cpp b7df8f760d0f75459f1e80e3d8e18d49a3995df8 
>   src/tests/container_logger_tests.cpp 
> efadceafca5721bce4dbffadb35f54fd5365abb0 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> c524f42743bf08ee54f1cbb083d0d3c85a8b70c9 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 4293416ac8434e9eb7e80724480a54936a2fe24a 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 09742ff21513dc2570684d384b257868dd57a9ce 
> 
> Diff: https://reviews.apache.org/r/46798/diff/
> 
> 
> Testing
> ---
> 
> make check; used mesos cli to test end to end functionality.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 49617: Add benchmark for failover of many frameworks.

[Guangya Liu]

> On 八月 24, 2016, 9:12 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 4091-4099
> > 
> >
> > Can we make sure all of the `allocations` are available when 
> > `recoverResources`? Do we need a `Clock::settle();` after all 
> > `suppressOffers` finshed in line 4090?

Looking forward to the updated patch, just want to highlight that adding 
`Clock::settle` after `activateFramework` maybe better as we can make sure the 
all of `allocate()` operations are finshed before suppress offer. 

The logic could be 
```
activateFramework()

Clock::settle()

suppressOffers()

recoverResources()

Clock::settle()
```


- Guangya


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


On 八月 17, 2016, 2:26 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49617/
> ---
> 
> (Updated 八月 17, 2016, 2:26 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5780
> https://issues.apache.org/jira/browse/MESOS-5780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This benchmark measures latency to stability of
>   the allocator following disconnection and
>   reconnection of all frameworks.
> - In this scenario, frameworks are offered resources
>   and suppressed in batches.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> cbed333f497016fe2811f755028796012b41db77 
> 
> Diff: https://reviews.apache.org/r/49617/diff/
> 
> 
> Testing
> ---
> 
> MESOS_BENCHMARK=1 GTEST_FILTER="*BENCHMARK_Test.FrameworkFailover*" make check
> 
> Sample Output:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/23
> Using 1 agents and 6000 frameworks
> Added 6000 frameworks in 113410us
> Added 1 agents in 6.8398066333mins
> allocator settled after  3.286837mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/23
>  (609255 ms)[ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/24
> Using 2 agents and 1 frameworks
> Added 1 frameworks in 190us
> Added 2 agents in 4.752954secs
> allocator settled after  7us
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/24
>  (6332 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51683: Avoided resource validation when flatten resources.

[Guangya Liu]

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

(Updated 九月 14, 2016, 6:09 a.m.)


Review request for mesos, Benjamin Mahler and Jiang Yan Xu.


Changes
---

Rebase


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


Repository: mesos


Description
---

1) Previously if role or reservation info are invalid,
the result is an empty Resources object, now it returns
an error.
2) Added a new flatten() method to flatten resources with
star role and empty reservation.


Diffs (updated)
-

  include/mesos/resources.hpp 7ba422d57d4e058c682df9aa780557782423 
  include/mesos/v1/resources.hpp add48c7baf8fabf5cc443d60a6c96d2902fc67de 
  src/cli/execute.cpp c9f56af7f37d5b79b51f350d6c533714c170e889 
  src/common/resources.cpp f841e91b81975a887e64a76a709708ed7946025f 
  src/examples/dynamic_reservation_framework.cpp 
850bb2a5b243dd5d5a8b6476570b4f943fde6185 
  src/examples/test_framework.cpp a83766a92617d50eadd92ec55113e049a7290d03 
  src/examples/test_http_framework.cpp 441e86c88b035d9a268b8b51b95da3e1eb842a62 
  src/tests/api_tests.cpp e440d1b44fb0b40dd1f68209c71e5ca0cd19f4fb 
  src/tests/hierarchical_allocator_tests.cpp 
7705de95a3916310baf4daca62aab1e6b1ca3cb3 
  src/tests/mesos.hpp 92d7d94733d461eb0c565830cc1c8e709e7a2ef7 
  src/tests/persistent_volume_endpoints_tests.cpp 
266c2a0ff4a99baa96a7c4980f076755603256a9 
  src/tests/reservation_endpoints_tests.cpp 
bee5ea6b3a3ee7fafb7312a6d89de79d62c57ec1 
  src/tests/reservation_tests.cpp 000957826011bf28f7550a83db3e60a796162fb3 
  src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
  src/tests/role_tests.cpp 162c9414a70723a212cfd39ac5c0b8325c3f3b5d 
  src/v1/resources.cpp 4410a8f95ef0ebe6ec7aee38cd47f6f83f863867 

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


Testing
---

make
make check

```
./bin/mesos-tests.sh  --gtest_filter="ResourcesOperationTest.FlattenResources"
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from ResourcesOperationTest
[ RUN  ] ResourcesOperationTest.FlattenResources
[   OK ] ResourcesOperationTest.FlattenResources (3 ms)
[--] 1 test from ResourcesOperationTest (3 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (24 ms total)
[  PASSED  ] 1 test.
```


Thanks,

Guangya Liu