Re: Review Request 51919: Added test to validate capability of SHARED_RESOURCES.

[Anindya Sinha]

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

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


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Added test to validate capability of SHARED_RESOURCES.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
7705de95a3916310baf4daca62aab1e6b1ca3cb3 

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


Testing
---

Tests passed (including the test added in this RR).


Thanks,

Anindya Sinha

Re: Review Request 51942: Removed scalars() API.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51942]

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. 16, 2016, 1:53 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51942/
> ---
> 
> (Updated Sept. 16, 2016, 1:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Neil Conway, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This API is not used anymore because of performance issues and
> replaced by `createStrippedScalarQuantity`.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 7ba422d57d4e058c682df9aa780557782423 
>   include/mesos/v1/resources.hpp add48c7baf8fabf5cc443d60a6c96d2902fc67de 
>   src/common/resources.cpp aa4d6b5af9c24e90cefaa4f4c1ad593966eceb7c 
>   src/v1/resources.cpp 4b66acbba92bc6316fa77ae4dcabce9fab65f7b5 
> 
> Diff: https://reviews.apache.org/r/51942/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 51942: Removed scalars() API.

[Guangya Liu]

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

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


Repository: mesos


Description
---

This API is not used anymore because of performance issues and
replaced by `createStrippedScalarQuantity`.


Diffs
-

  include/mesos/resources.hpp 7ba422d57d4e058c682df9aa780557782423 
  include/mesos/v1/resources.hpp add48c7baf8fabf5cc443d60a6c96d2902fc67de 
  src/common/resources.cpp aa4d6b5af9c24e90cefaa4f4c1ad593966eceb7c 
  src/v1/resources.cpp 4b66acbba92bc6316fa77ae4dcabce9fab65f7b5 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 50959: Added appc fetcher plugin to use rkt tool.

[Jie Yu]

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




src/Makefile.am (line 854)


I'd rename this to rkt. The fetcher plugin should be named by their 
'method', rather than 'content'. E.g., copy, curl, hadoop, etc.



src/uri/fetchers/appc.cpp (lines 99 - 103)


YOu should do this check in 'create' to fail early.



src/uri/fetchers/appc.cpp (line 108)


`.await` is a blocking call. We cannot use blocking calls as it'll block 
the worker thread. Please use .then



src/uri/fetchers/appc.cpp (line 110)


All error message should be Capitalized.



src/uri/fetchers/appc.cpp (line 116)


Ditto on await. We cannot use that because it's blocking. We only use 
'await' in tests.



src/uri/fetchers/appc.cpp (line 118)


space before and after `+`



src/uri/fetchers/appc.cpp (line 122)


Comments need to be a full sentense. Capitalize please. Please refer to the 
style guide.



src/uri/fetchers/appc.cpp (lines 149 - 151)


I suggest we create a class called Rkt which abstracts the rkt tool.
```
Rkt rkt = Rkt::create();
rkt->fetch(...);
rkt->export(...);
```

You can do the os::which check in Rkt::create.



src/uri/fetchers/appc.cpp (line 252)


Please fix all the space issue. you need space before and after `+`


- Jie Yu


On Sept. 6, 2016, 8:11 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50959/
> ---
> 
> (Updated Sept. 6, 2016, 8:11 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4288
> https://issues.apache.org/jira/browse/MESOS-4288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> AppcFetcherPlugin returns a '.aci' file of a container image.
> 
> Fetch supports all three modes that 'rkt fetch' supports, fetch with
> meta discovery  ex: 'coreos.com/hello:latest', fetch from a specific
> location ex: 'https://github.com/xxx/hello.aci' or fetch from docker
> registry ex: 'docker://hello-world'.
> 
> Added new fetched plugin for Appc fetch algorithm. The new
> AppcFetcherPlugin implements two methods. Method 'rktFetch' is used to
> fetch a container image into a store specified at a directory location
> using 'rkt fetch' command and method 'rktExport' exports the image
> fetched into the directory location as a '.aci' file that is in tar
> file format, usinf 'rkt image export' command.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 15b9a63822eca8d0b428191940026756fba7821e 
>   src/uri/fetcher.hpp 949363ecdb15f5b225db7f3ed67eda52c351bfa1 
>   src/uri/fetcher.cpp 904fce5a203c57ef1b8fdda09c81efbcf18f5d2c 
>   src/uri/fetchers/appc.hpp PRE-CREATION 
>   src/uri/fetchers/appc.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50959/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 51909: Added benchmarks for new registry operations.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 15, 2016, 11:13 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51909/
> ---
> 
> (Updated Sept. 15, 2016, 11:13 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These benchmarks cover:
> 
>   1. The time taken to make all registered agents unreachable and then
>  reachable. This is similar to what happens during a severe network
>  partition.
> 
>   2. The time taken to GC a significant fraction (50%) of the
>  unreachable list from the registry.
> 
> 
> Diffs
> -
> 
>   src/tests/registrar_tests.cpp b04fc92c66449f1094212a534899e16d5f6f01bd 
> 
> Diff: https://reviews.apache.org/r/51909/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> The performance of the new registry operations seems quite acceptable for 
> reasonable cluster sizes, despite the fact that `PruneUnreachable` is 
> quadratic time and `MarkSlaveReachable` and `MarkSlaveUnreachable` are both 
> linear time. For the time being, optimizing these operations doesn't seem 
> like it is needed urgently.
> 
> ```
> [ RUN  ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/0
> Garbage collected 5000 agents in 166351us
> [   OK ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/0 (7316 ms)
> [ RUN  ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/1
> Garbage collected 1 agents in 427487us
> [   OK ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/1 (15048 ms)
> [ RUN  ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/2
> Garbage collected 15000 agents in 587876us
> [   OK ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/2 (22844 ms)
> [ RUN  ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/3
> Garbage collected 25000 agents in 1.170591secs
> [   OK ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/3 (49469 ms)
> [ RUN  ] 
> SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/0
> Marked 1 agents unreachable in 3.040816secs
> Marked 1 agents reachable in 2.322533secs
> [   OK ] 
> SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/0 (7490 ms)
> [ RUN  ] 
> SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/1
> Marked 2 agents unreachable in 8.192604secs
> Marked 2 agents reachable in 5.041321secs
> [   OK ] 
> SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/1 (18846 ms)
> [ RUN  ] 
> SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/2
> Marked 3 agents unreachable in 15.21931secs
> Marked 3 agents reachable in 8.857088secs
> [   OK ] 
> SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/2 (32296 ms)
> [ RUN  ] 
> SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/3
> Marked 5 agents unreachable in 39.277686secs
> Marked 5 agents reachable in 18.483256secs
> [   OK ] 
> SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/3 (73369 ms)
> ```
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 51825: Updated the streaming function for ContainerID to be nesting aware.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [51825, 51816, 51780, 51779, 51778, 51421, 51770, 51420, 
51503, 51402, 51393, 51392, 51323]

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

Error:
2016-09-16 00:18:37 URL:https://reviews.apache.org/r/51778/diff/raw/ [469/469] 
-> "51778.patch" [1]
error: patch failed: src/slave/containerizer/mesos/provisioner/paths.cpp:20
error: src/slave/containerizer/mesos/provisioner/paths.cpp: patch does not apply

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

- Mesos ReviewBot


On Sept. 15, 2016, 7:46 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51825/
> ---
> 
> (Updated Sept. 15, 2016, 7:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6073
> https://issues.apache.org/jira/browse/MESOS-6073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the streaming function for ContainerID to be nesting aware.
> 
> 
> Diffs
> -
> 
>   src/common/type_utils.cpp c7d50334b4e56bc0eb5473b9f9277465b607334f 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/51825/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51921: Added backports for a potential double-close in libevent sockets.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51921]

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. 15, 2016, 5:26 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51921/
> ---
> 
> (Updated Sept. 15, 2016, 5:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, Artem 
> Harutyunyan, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the CHANGELOG to reflect:
>   * Backporting MESOS-5986 to 0.27.4 and 0.28.3.
>   * Backporting MESOS-6104 and MESOS-6152 to 0.27.4, 0.28.3, and 1.0.2.
> 
> 
> Diffs
> -
> 
>   CHANGELOG ef31563e3f4a2ceda598a0ddcc7ad325f64a8538 
> 
> Diff: https://reviews.apache.org/r/51921/diff/
> 
> 
> Testing
> ---
> 
> TODO: Running the 0.27.x, 0.28.x, and 1.0.x branches + backports through an 
> internal CI...
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 51919: Added test to validate capability of SHARED_RESOURCES.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/tests/hierarchical_allocator_tests.cpp (lines 1388 - 1389)


s/in offer to/only offered to/

s/only// 

(Basically move `only` up for more emphasis)



src/tests/hierarchical_allocator_tests.cpp (line 1390)


Mention `Capability` in the test name helps clarify what the test focuses 
on.

Maybe `SharedResourcesCapability`?



src/tests/hierarchical_allocator_tests.cpp (lines 1411 - 1415)


We haven't done so consistently but in new tests let's use abstractions 
more often to make tests shorter.

```
Resources volume = createDiskResource(5, "role1", "id1", None(), None(), 
true);
```

Perhaps we should update `UpdateAllocationSharedPersistentVolume` as well.



src/tests/hierarchical_allocator_tests.cpp (lines 1417 - 1419)


```
CREATE(volume);
```



src/tests/hierarchical_allocator_tests.cpp (line 1462)


`createFrameworkInfo` takes a `capabilities` argument.


- Jiang Yan Xu


On Sept. 15, 2016, 9:46 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51919/
> ---
> 
> (Updated Sept. 15, 2016, 9:46 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4325
> https://issues.apache.org/jira/browse/MESOS-4325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test to validate capability of SHARED_RESOURCES.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 7705de95a3916310baf4daca62aab1e6b1ca3cb3 
> 
> Diff: https://reviews.apache.org/r/51919/diff/
> 
> 
> Testing
> ---
> 
> Tests passed (including the test added in this RR).
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

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

[Vinod Kone]

> On Sept. 6, 2016, 11:33 a.m., Vinod Kone wrote:
> >
> 
> Vinod Kone wrote:
> also, can you add a test for this?
> 
> Qian Zhang wrote:
> Sure, I can add a test. I am just wondering how to use a test to verify 
> the logic we added in this patch, maybe we can just add a test to launch a 
> short task and check `ContainerTermination.status` is 0 when 
> `Slave::executorTerminated()` is call which means the HTTP command executor 
> has received the ACK from agent for the terminal status update. Any comments?

That sounds like a good test. Can you add that?


- Vinod


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


On Sept. 15, 2016, 8:57 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46187/
> ---
> 
> (Updated Sept. 15, 2016, 8:57 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5276
> https://issues.apache.org/jira/browse/MESOS-5276
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Terminate when receiving the ACK of terminal status update.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 5370634ef9e6f3ac9717fed71f6a77707026a16a 
> 
> Diff: https://reviews.apache.org/r/46187/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 15, 2016, 8:57 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46187/
> ---
> 
> (Updated Sept. 15, 2016, 8:57 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5276
> https://issues.apache.org/jira/browse/MESOS-5276
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Terminate when receiving the ACK of terminal status update.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 5370634ef9e6f3ac9717fed71f6a77707026a16a 
> 
> Diff: https://reviews.apache.org/r/46187/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 15, 2016, 4:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51905/
> ---
> 
> (Updated Sept. 15, 2016, 4:34 a.m.)
> 
> 
> 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 51904: Ensured that queued task groups killed before launch are cleaned up.

[Vinod Kone]

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


Fix it, then Ship it!





src/slave/slave.cpp (line 6579)


reword the comment.



src/slave/slave.cpp (line 6592)


```
if (!terminal)
```



src/tests/slave_tests.cpp (line 4860)


Kill a task in the task group


- Vinod Kone


On Sept. 15, 2016, 4:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51904/
> ---
> 
> (Updated Sept. 15, 2016, 4:34 a.m.)
> 
> 
> 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
> 
>

Re: Review Request 51922: Rename Unified Containerizer to Universal Containerizer in docs.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51922]

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. 15, 2016, 5:37 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51922/
> ---
> 
> (Updated Sept. 15, 2016, 5:37 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rename Unified Containerizer to Universal Containerizer in docs.
> 
> We feel that "Universal" is better because it capture the fact that the 
> containerizer can support many image format.
> 
> 
> Diffs
> -
> 
>   docs/container-image.md 0feb0286218d1774f47640a7e41f0f65c6181db0 
>   docs/docker-volume.md d4bea06bafd856679a0b925a6d1eba2d5e8f6594 
> 
> Diff: https://reviews.apache.org/r/51922/diff/
> 
> 
> Testing
> ---
> 
> trivial
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

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.
> 
> Huadong Liu wrote:
> 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."?
> 
> Qian Zhang wrote:
> Huadong, please see my comments below:
> 
> * https://github.com/apache/mesos/blob/master/src/launcher/executor.cpp 
> is the Mesos command executor which is used by Mesos containerizer, it 
> actually can run in two modes (controlled by the agent flag 
> `--http_command_executor`):
>1. HTTP based mode (`--http_command_executor=true`): In this mode, it 
> will use the HTTP based executor library to interact with Mesos agent.
>2. Adapter based mode (`--http_command_executor=false`): In this mode, 
> it will use an adapter (`V0ToV1Adapter`) to call the old `ExecutorDriver` to 
> interact with Mesos agent.
> 

Re: Review Request 51922: Rename Unified Containerizer to Universal Containerizer in docs.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 15, 2016, 5:37 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51922/
> ---
> 
> (Updated Sept. 15, 2016, 5:37 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rename Unified Containerizer to Universal Containerizer in docs.
> 
> We feel that "Universal" is better because it capture the fact that the 
> containerizer can support many image format.
> 
> 
> Diffs
> -
> 
>   docs/container-image.md 0feb0286218d1774f47640a7e41f0f65c6181db0 
>   docs/docker-volume.md d4bea06bafd856679a0b925a6d1eba2d5e8f6594 
> 
> Diff: https://reviews.apache.org/r/51922/diff/
> 
> 
> Testing
> ---
> 
> trivial
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 51825: Updated the streaming function for ContainerID to be nesting aware.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On Sept. 15, 2016, 7:46 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51825/
> ---
> 
> (Updated Sept. 15, 2016, 7:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6073
> https://issues.apache.org/jira/browse/MESOS-6073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the streaming function for ContainerID to be nesting aware.
> 
> 
> Diffs
> -
> 
>   src/common/type_utils.cpp c7d50334b4e56bc0eb5473b9f9277465b607334f 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/51825/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51825: Updated the streaming function for ContainerID to be nesting aware.

[Gilbert Song]

> On Sept. 15, 2016, 12:22 p.m., Avinash sridharan wrote:
> > src/common/type_utils.cpp, line 417
> > 
> >
> > Can we use "." instead of "/" as the delimiter. Reason being that in 
> > `network/cni` isolator we plan to use the stringified version of 
> > `ContainerID` to create directories for containers. This would result in 
> > creating a hierarchy. Using "." instead of "/" would prevent it from 
> > creating hierarchies. Also, recreating `ContainerID` from a flat hierarchy 
> > structure, would much more cleaner.

We will have the recreate mothod later. :)


- Gilbert


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


On Sept. 15, 2016, 12:46 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51825/
> ---
> 
> (Updated Sept. 15, 2016, 12:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6073
> https://issues.apache.org/jira/browse/MESOS-6073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the streaming function for ContainerID to be nesting aware.
> 
> 
> Diffs
> -
> 
>   src/common/type_utils.cpp c7d50334b4e56bc0eb5473b9f9277465b607334f 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/51825/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51825: Updated the streaming function for ContainerID to be nesting aware.

[Gilbert Song]

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

(Updated Sept. 15, 2016, 12:46 p.m.)


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


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


Repository: mesos


Description
---

Updated the streaming function for ContainerID to be nesting aware.


Diffs (updated)
-

  src/common/type_utils.cpp c7d50334b4e56bc0eb5473b9f9277465b607334f 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
96e24500a12825161553eb050da389088b122695 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51919: Added test to validate capability of SHARED_RESOURCES.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45966, 51919]

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. 15, 2016, 4:46 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51919/
> ---
> 
> (Updated Sept. 15, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4325
> https://issues.apache.org/jira/browse/MESOS-4325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test to validate capability of SHARED_RESOURCES.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 7705de95a3916310baf4daca62aab1e6b1ca3cb3 
> 
> Diff: https://reviews.apache.org/r/51919/diff/
> 
> 
> Testing
> ---
> 
> Tests passed (including the test added in this RR).
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51825: Updated the streaming function for ContainerID to be nesting aware.

[Avinash sridharan]

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




src/common/type_utils.cpp (line 417)


Can we use "." instead of "/" as the delimiter. Reason being that in 
`network/cni` isolator we plan to use the stringified version of `ContainerID` 
to create directories for containers. This would result in creating a 
hierarchy. Using "." instead of "/" would prevent it from creating hierarchies. 
Also, recreating `ContainerID` from a flat hierarchy structure, would much more 
cleaner.


- Avinash sridharan


On Sept. 12, 2016, 9:40 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51825/
> ---
> 
> (Updated Sept. 12, 2016, 9:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6073
> https://issues.apache.org/jira/browse/MESOS-6073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the streaming function for ContainerID to be nesting aware.
> 
> 
> Diffs
> -
> 
>   src/common/type_utils.cpp c7d50334b4e56bc0eb5473b9f9277465b607334f 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/51825/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

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

[Joseph Wu]

> On Sept. 13, 2016, 6:35 p.m., Joseph Wu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 818
> > 
> >
> > Will these symlinks resolve correctly if the child has a different 
> > rootfs than the parent container?
> 
> Avinash sridharan wrote:
> The symlinks are created in the hostfs, and they are then bind mounted to 
> /etc/hosts, /etc/hostname, and /etc/resolv.conf in the container's rootfs 
> here:
> 
> https://github.com/apache/mesos/blob/3e52a107c4073778de9c14bf5fcdeb6e342821aa/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp#L1583
> 
> So yeah the symlinks should always resolve correctly, as long as the 
> 'root' parent container is alive.

The isolator will create a directory structure like:
```
/var/run/mesos/isolators/network/cni
  |-- parent-container-id/
  |-- hosts
  |-- hostname
  |-- resolv.conf
  |-- child-container-id/
  |-- hosts --> 
/var/run/mesos/isolators/network/cni/parent-container-id/hosts
  |-- hostname --> 
/var/run/mesos/isolators/network/cni/parent-container-id/hostname
  |-- resolv.conf --> 
/var/run/mesos/isolators/network/cni/parent-container-id/resolve.conf
```

Each file in the `child-container-id` folder is then mounted into the child 
container's rootfs.
After reading this ( http://www.proftpd.org/docs/howto/Chroot.html ), I'm not 
so sure this will work.  

The symlink after chroot-ing will point to an absolute location within the 
child's rootfs; whereas it was originally pointing to the host's rootfs.  A 
relative symlink won't work either, as you're not mounting the entire 
`/var/run/mesos/isolators/network/cni` directory.
The child will simply have a bunch of dangling symlinks:
```
/etc
  |-- hosts --> /var/run/mesos/isolators/network/cni/parent-container-id/hosts
  |-- hostname --> 
/var/run/mesos/isolators/network/cni/parent-container-id/hostname
  |-- resolv.conf --> 
/var/run/mesos/isolators/network/cni/parent-container-id/resolve.conf
```

Based on that, you may need to either hard-link the files or mount the parent's 
files directly.


- Joseph


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


On Sept. 14, 2016, 9:25 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51871/
> ---
> 
> (Updated Sept. 14, 2016, 9:25 a.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 51736: Added fields `chain` and `excludeDevices` to `PortMapper`.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 (lines 127 - 128)


Please add a comment about why we need a required dedicated chain, instead 
of using pre-routing chain.


- Jie Yu


On Sept. 12, 2016, 7:04 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51736/
> ---
> 
> (Updated Sept. 12, 2016, 7:04 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added fields `chain` and `excludeDevices` to `PortMapper`.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  85547533b0b13011615b512ec8c71b7545f33324 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  0ecf64f2de5fc27f208e9dd0e3608b9a6750e9a6 
> 
> Diff: https://reviews.apache.org/r/51736/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

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

[Anand Mazumdar]

> On Sept. 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?
> 
> Guangya Liu wrote:
> ```
> 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);
> ```

Great suggestion. We would like to do that though when we need it i.e. when we 
add support for `LAUNCH/DESTROY` sub containers eventually. For the current 
code, both the scenarios are the same.


- Anand


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


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 51886: Added tests for default executor.

[Anand Mazumdar]

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

(Updated Sept. 15, 2016, 6:25 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments NNFR


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 (updated)
-

  src/Makefile.am f1d202ae08d6bb4fd9e11eb1eae75dd9d5d9d8d5 
  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]

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

(Updated Sept. 15, 2016, 6:05 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments. NNFR


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 (updated)
-

  src/Makefile.am f1d202ae08d6bb4fd9e11eb1eae75dd9d5d9d8d5 
  src/launcher/default_executor.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 51921: Added backports for a potential double-close in libevent sockets.

[Joris Van Remoortere]

> On Sept. 15, 2016, 5:56 p.m., Joris Van Remoortere wrote:
> > Did you want to fold the minor fix into your original patch to backport?
> 
> Joseph Wu wrote:
> Do you mean MESOS-6152?  (i.e. `s/fd/owned_fd`)
> 
> I kept the cherry-picks un-squashed to retain history.

Yeah. Maybe Vinod / BenM can chime in regarding squash vs. retain history.


- Joris


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


On Sept. 15, 2016, 5:26 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51921/
> ---
> 
> (Updated Sept. 15, 2016, 5:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, Artem 
> Harutyunyan, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the CHANGELOG to reflect:
>   * Backporting MESOS-5986 to 0.27.4 and 0.28.3.
>   * Backporting MESOS-6104 and MESOS-6152 to 0.27.4, 0.28.3, and 1.0.2.
> 
> 
> Diffs
> -
> 
>   CHANGELOG ef31563e3f4a2ceda598a0ddcc7ad325f64a8538 
> 
> Diff: https://reviews.apache.org/r/51921/diff/
> 
> 
> Testing
> ---
> 
> TODO: Running the 0.27.x, 0.28.x, and 1.0.x branches + backports through an 
> internal CI...
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 51921: Added backports for a potential double-close in libevent sockets.

[Joseph Wu]

> On Sept. 15, 2016, 10:56 a.m., Joris Van Remoortere wrote:
> > Did you want to fold the minor fix into your original patch to backport?

Do you mean MESOS-6152?  (i.e. `s/fd/owned_fd`)

I kept the cherry-picks un-squashed to retain history.


- Joseph


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


On Sept. 15, 2016, 10:26 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51921/
> ---
> 
> (Updated Sept. 15, 2016, 10:26 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, Artem 
> Harutyunyan, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the CHANGELOG to reflect:
>   * Backporting MESOS-5986 to 0.27.4 and 0.28.3.
>   * Backporting MESOS-6104 and MESOS-6152 to 0.27.4, 0.28.3, and 1.0.2.
> 
> 
> Diffs
> -
> 
>   CHANGELOG ef31563e3f4a2ceda598a0ddcc7ad325f64a8538 
> 
> Diff: https://reviews.apache.org/r/51921/diff/
> 
> 
> Testing
> ---
> 
> TODO: Running the 0.27.x, 0.28.x, and 1.0.x branches + backports through an 
> internal CI...
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

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

[Anindya Sinha]

> On Sept. 14, 2016, 11:53 p.m., Anindya Sinha wrote:
> > 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.
> 
> Jiang Yan Xu wrote:
> Since this is `recover()`, i.e., agent restart, I was referring to the 
> previous agent which laid down the checkpoint file before the restart but 
> would the following be more clear?
> 
> ```
> // This is to verify that the checkpointed resources are
> // compatible with the agent resources specified through the
> // '--resources' command line flag. The compatibility has been
> // verified by the old agent but the flag may have changed during
> // agent restart in an incompatible way and the operator may need
> // to either fix the flag or the checkpointed resources.
> ```

This looks way better. +1.


- Anindya


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


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 51921: Added backports for a potential double-close in libevent sockets.

[Joris Van Remoortere]

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



Did you want to fold the minor fix into your original patch to backport?

- Joris Van Remoortere


On Sept. 15, 2016, 5:26 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51921/
> ---
> 
> (Updated Sept. 15, 2016, 5:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, Artem 
> Harutyunyan, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the CHANGELOG to reflect:
>   * Backporting MESOS-5986 to 0.27.4 and 0.28.3.
>   * Backporting MESOS-6104 and MESOS-6152 to 0.27.4, 0.28.3, and 1.0.2.
> 
> 
> Diffs
> -
> 
>   CHANGELOG ef31563e3f4a2ceda598a0ddcc7ad325f64a8538 
> 
> Diff: https://reviews.apache.org/r/51921/diff/
> 
> 
> Testing
> ---
> 
> TODO: Running the 0.27.x, 0.28.x, and 1.0.x branches + backports through an 
> internal CI...
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

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

[Jiang Yan Xu]

> On Sept. 14, 2016, 4:53 p.m., Anindya Sinha wrote:
> > 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.

Since this is `recover()`, i.e., agent restart, I was referring to the previous 
agent which laid down the checkpoint file before the restart but would the 
following be more clear?

```
// This is to verify that the checkpointed resources are
// compatible with the agent resources specified through the
// '--resources' command line flag. The compatibility has been
// verified by the old agent but the flag may have changed during
// agent restart in an incompatible way and the operator may need
// to either fix the flag or the checkpointed resources.
```


- Jiang Yan


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


On Sept. 14, 2016, 11:21 a.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, 11:21 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
> ---
> 
> 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 51879: Determine disk size when not specified in static resources.

[Jiang Yan Xu]

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



The addition of `parseText` and `parseValidResources` feels unnecessary to me. 
I think we can reuse `convertJSON()` to achieve the same.

Overall I think the following steps would be simpler and more elegant.

## Patch 1: Change convertJSON() to return a Try

This way the empty disks in the result are not dropped.

```
vector convertJSON(
const JSON::Array& resourcesJSON,
const string& defaultRole)
```

I also don't see why `convertJSON()` needs to fail on empty resources since 
they are allowed in the regular string formatted resources specification. Maybe 
we can have a **patch 0** that changes that?

The current lone call site can just construct `Resources` from 
`vector`.

Then make convertJSON a public static member method.

## Patch 2: Let Resources class answer if a resource is a MOUNT disk, PATH disk 
or ROOT disk

e.g.,

```
static bool Resources::isMountDisk(const Resource& resource);
```

These are pretty general purpose and purely for convenience. Add simple tests 
to verify these methods.

## Patch 3: Autodetection for MOUNT and ROOT disks

In `Containerizer::resources()`, process each disk from the result of 
`convertJSON()`.

For MOUNT disks that are empty (`isEmpty()`), detect their sizes and modify the 
Resource objects.
What about the ROOT disk? If `disk:0` is in a plain string we don't do 
anything. With the JSON input the operator can choose to not specify a root 
disk to have it autodetected, so I guess we don't have to separately deal with 
ROOT disks with an explict "zero" size. If all disks have been processed and no 
ROOT disk is encountered, we auto-detect it.

How does it sound?


src/slave/containerizer/containerizer.cpp (line 162)


Call them `disks` to avoid confusion with the volumes as a Mesos concept.



src/slave/containerizer/containerizer.cpp (lines 245 - 250)


This is only for the ROOT disk, i.e., we only leave some headroom for the 
system root partition.

Therefore we probably don't need this method.


- Jiang Yan Xu


On Sept. 13, 2016, 7:29 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> ---
> 
> (Updated Sept. 13, 2016, 7:29 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When static resources indicate disks with a positive size, we use that
> for the disk resources on the agent. However, --resources can include
> disks with size of 0, which indicates that mesos agent determine the
> size of those disks from the host and uses that information instead.
> Note that this is not allowed for PATH disks since PATH disks can be
> carved into smaller chunks and we cannot assume that the whole
> physical disk is available to the PATH disk.
> 
> With this change, JSON or textual representation for resources that
> specify scalar value of 0 would not result in an error, but those
> resources will not be accounted for until a valid size is determined
> for such resources. A scalar value of -1 in JSON or textual formats
> still results in an invalid resource.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 7ba422d57d4e058c682df9aa780557782423 
>   include/mesos/v1/resources.hpp add48c7baf8fabf5cc443d60a6c96d2902fc67de 
>   src/common/resources.cpp f841e91b81975a887e64a76a709708ed7946025f 
>   src/slave/containerizer/containerizer.hpp 
> f13669d0dfc4ce3287cfe630cabd0470dc765b51 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
>   src/v1/resources.cpp 4410a8f95ef0ebe6ec7aee38cd47f6f83f863867 
> 
> Diff: https://reviews.apache.org/r/51879/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51922: Rename Unified Containerizer to Universal Containerizer in docs.

[Joseph Wu]

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


Ship it!




Is it appropriate to retro-actively update the name in some of the blogs?

http://mesos.apache.org/blog/mesos-0-28-0-released/
http://mesos.apache.org/blog/mesos-1-0-0-released/

---

Also, your description text needs some wrapping :)

- Joseph Wu


On Sept. 15, 2016, 10:37 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51922/
> ---
> 
> (Updated Sept. 15, 2016, 10:37 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rename Unified Containerizer to Universal Containerizer in docs.
> 
> We feel that "Universal" is better because it capture the fact that the 
> containerizer can support many image format.
> 
> 
> Diffs
> -
> 
>   docs/container-image.md 0feb0286218d1774f47640a7e41f0f65c6181db0 
>   docs/docker-volume.md d4bea06bafd856679a0b925a6d1eba2d5e8f6594 
> 
> Diff: https://reviews.apache.org/r/51922/diff/
> 
> 
> Testing
> ---
> 
> trivial
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 51913: Ignored agent registrations with duplicate agent IDs.

[Mesos ReviewBot]

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



Patch looks great!

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

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. 15, 2016, 1:40 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51913/
> ---
> 
> (Updated Sept. 15, 2016, 1:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If an agent attempts to register and is assigned an agent ID that
> already appears in the registry, the likely cause is a UUID collision:
> slave IDs are prefixed with master IDs, and master IDs are randomly
> generated UUIDs. UUID collisions are extremely unlikely, so in practice
> this should never happen.
> 
> This commit updates the comments to clarify how unlikely this situation
> is. Furthermore, when it does happen, the master now just ignores the
> registration attempt, rather than trying to shutdown the agent. This is
> simpler; the agent will eventually try to register again and be assigned
> a new agent ID. Finally, it is unclear that shutting down the agent is
> actually appropriate: the previous coding added the duplicate ID to
> `slaves.removed`, which will interfere with the activity of the other
> slave whose ID we collided with.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 4992ab0a0bb5babbf6a4fa3e6eff3577590fc879 
>   src/master/master.cpp b88472f6350d3f71e057bab34822423da5427151 
> 
> Diff: https://reviews.apache.org/r/51913/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 51922: Rename Unified Containerizer to Universal Containerizer in docs.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Sept. 15, 2016, 10:37 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51922/
> ---
> 
> (Updated Sept. 15, 2016, 10:37 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rename Unified Containerizer to Universal Containerizer in docs.
> 
> We feel that "Universal" is better because it capture the fact that the 
> containerizer can support many image format.
> 
> 
> Diffs
> -
> 
>   docs/container-image.md 0feb0286218d1774f47640a7e41f0f65c6181db0 
>   docs/docker-volume.md d4bea06bafd856679a0b925a6d1eba2d5e8f6594 
> 
> Diff: https://reviews.apache.org/r/51922/diff/
> 
> 
> Testing
> ---
> 
> trivial
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 51922: Rename Unified Containerizer to Universal Containerizer in docs.

[Jie Yu]

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

(Updated Sept. 15, 2016, 5:37 p.m.)


Review request for mesos, Gilbert Song and Joseph Wu.


Repository: mesos


Description (updated)
---

Rename Unified Containerizer to Universal Containerizer in docs.

We feel that "Universal" is better because it capture the fact that the 
containerizer can support many image format.


Diffs
-

  docs/container-image.md 0feb0286218d1774f47640a7e41f0f65c6181db0 
  docs/docker-volume.md d4bea06bafd856679a0b925a6d1eba2d5e8f6594 

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


Testing
---

trivial


Thanks,

Jie Yu

Review Request 51922: Rename Unified Containerizer to Universal Containerizer in docs.

[Jie Yu]

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

Review request for mesos, Gilbert Song and Joseph Wu.


Repository: mesos


Description
---

Rename Unified Containerizer to Universal Containerizer in docs.


Diffs
-

  docs/container-image.md 0feb0286218d1774f47640a7e41f0f65c6181db0 
  docs/docker-volume.md d4bea06bafd856679a0b925a6d1eba2d5e8f6594 

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


Testing
---

trivial


Thanks,

Jie Yu

Re: 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/
---

(Updated Sept. 15, 2016, 5:32 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments. NNFR


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 (updated)
-

  src/slave/slave.cpp 7bd5fad13045515ff0929857c032e23e56cf7e1b 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 51921: Added backports for a potential double-close in libevent sockets.

[Joseph Wu]

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

Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, Artem 
Harutyunyan, and Joris Van Remoortere.


Repository: mesos


Description
---

This updates the CHANGELOG to reflect:
  * Backporting MESOS-5986 to 0.27.4 and 0.28.3.
  * Backporting MESOS-6104 and MESOS-6152 to 0.27.4, 0.28.3, and 1.0.2.


Diffs
-

  CHANGELOG ef31563e3f4a2ceda598a0ddcc7ad325f64a8538 

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


Testing
---

TODO: Running the 0.27.x, 0.28.x, and 1.0.x branches + backports through an 
internal CI...


Thanks,

Joseph Wu

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

[Guangya Liu]

> 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.
> 
> Guangya Liu wrote:
> I think that here is not `multiple task groups` but `multiple tasks in 
> one task group`?
> 
> Anand Mazumdar wrote:
> We do accept multiple tasks in one task group for the default executor. 
> But, we only accept a single `LAUNCH_GROUP` operation. Hope that helps.

Got it, thanks. Multiple task groups on one default executor will cause task 
failed.


- 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 45962: Added a persistent volume test framework for shared volumes.

[Anindya Sinha]

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

(Updated Sept. 15, 2016, 4:47 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added a persistent volume test framework for shared volumes.


Diffs (updated)
-

  src/Makefile.am f1d202ae08d6bb4fd9e11eb1eae75dd9d5d9d8d5 
  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/tests/examples_tests.cpp 52fac33733cc86dd718c7836c5031999f1597a5c 
  src/tests/persistent_shared_volume_framework_test.sh PRE-CREATION 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Review Request 51919: Added test to validate capability of SHARED_RESOURCES.

[Anindya Sinha]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added test to validate capability of SHARED_RESOURCES.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
7705de95a3916310baf4daca62aab1e6b1ca3cb3 

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


Testing
---

Tests passed (including the test added in this RR).


Thanks,

Anindya Sinha

Re: Review Request 45966: Offer shared resources to frameworks only if opted in.

[Anindya Sinha]

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

(Updated Sept. 15, 2016, 4:45 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added a new capability SHARED_RESOURCES that frameworks need to opt
in if they are interested in receiving shared resources in their
offers.


Diffs (updated)
-

  include/mesos/mesos.proto 06c2367ef2201be7b70e6015aa7c438893b73068 
  include/mesos/v1/mesos.proto 68b90bdf40cf5505bffd8044f034917d7992f71c 
  src/master/allocator/mesos/hierarchical.hpp 
1f5f3801eff6b15914ad4a740479a008530befc5 
  src/master/allocator/mesos/hierarchical.cpp 
e866fd275a6c435eab6d09862bca46e9df6c4cc2 
  src/tests/hierarchical_allocator_tests.cpp 
7705de95a3916310baf4daca62aab1e6b1ca3cb3 

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


Testing
---

Tests updated with new capability.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45966: Offer shared resources to frameworks only if opted in.

[Anindya Sinha]

> On Sept. 7, 2016, 10:09 p.m., Jiang Yan Xu wrote:
> > Can we have a simple allocator test with two frameworks, one framework 
> > without the capability doesn't get allocated the shared resource and the 
> > other with the capability gets it?
> > 
> > Could be in a follow up review.
> 
> Anindya Sinha wrote:
> Will add this test in a separate review.

Added a test in https://reviews.apache.org/r/51919/


- Anindya


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


On Sept. 7, 2016, 11:55 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45966/
> ---
> 
> (Updated Sept. 7, 2016, 11:55 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4325
> https://issues.apache.org/jira/browse/MESOS-4325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new capability SHARED_RESOURCES that frameworks need to opt
> in if they are interested in receiving shared resources in their
> offers.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 7fbcdf005718ba4d7ec3b02cf70a3fc53d4f4818 
>   include/mesos/v1/mesos.proto 60ec0cce35e5e2d2f4907efadc7ab92680566ef0 
>   src/master/allocator/mesos/hierarchical.hpp 
> 1f5f3801eff6b15914ad4a740479a008530befc5 
>   src/master/allocator/mesos/hierarchical.cpp 
> 992031a6fbc89727725071841bd3ab827737d8bd 
>   src/tests/hierarchical_allocator_tests.cpp 
> b2179215f3a4c2f4018670e8e54f02e06c39c3b1 
> 
> Diff: https://reviews.apache.org/r/45966/diff/
> 
> 
> Testing
> ---
> 
> Tests updated with new capability.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

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

[Anand Mazumdar]

> 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
> 
> Vinod Kone wrote:
> what anand reads correct to me.
> 
> Guangya Liu wrote:
> I think that here is not `multiple task groups` but `multiple tasks in 
> one task group`?

We do accept multiple tasks in one task group for the default executor. But, we 
only accept a single `LAUNCH_GROUP` operation. Hope that helps.


- Anand


---
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 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/#review149067
---



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

Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush

[Mesos ReviewBot]

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



Bad review!

Reviews applied: []

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

- Mesos ReviewBot


On Sept. 15, 2016, 12:03 p.m., Anastasia Braginsky wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51785/
> ---
> 
> (Updated Sept. 15, 2016, 12:03 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: hbase-git
> 
> 
> Description
> ---
> 
> This is a step toward final compacting memstore that allowes two modes of 
> work: index-compaction and data-compaction. 
> 
> The index-compaction means that when the new segment is pushed into the 
> pipeline, it is flattened and probably merged with old segments in the 
> pipeline. The new merge "feature" induces no data-copy-compaction and no 
> speculative SQM scan. 
> The compacting memstore of the data-compaction type means the usage of the 
> data-copy-compaction.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2dd7913a1477f3c3560be4e2c1450b93fb3afc78 
>   src/launcher/default_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51785/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anastasia Braginsky
> 
>

Re: Review Request 51913: Ignored agent registrations with duplicate agent IDs.

[Neil Conway]

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

(Updated Sept. 15, 2016, 1:40 p.m.)


Review request for mesos, Benjamin Mahler and Vinod Kone.


Changes
---

Tweak comments.


Repository: mesos


Description
---

If an agent attempts to register and is assigned an agent ID that
already appears in the registry, the likely cause is a UUID collision:
slave IDs are prefixed with master IDs, and master IDs are randomly
generated UUIDs. UUID collisions are extremely unlikely, so in practice
this should never happen.

This commit updates the comments to clarify how unlikely this situation
is. Furthermore, when it does happen, the master now just ignores the
registration attempt, rather than trying to shutdown the agent. This is
simpler; the agent will eventually try to register again and be assigned
a new agent ID. Finally, it is unclear that shutting down the agent is
actually appropriate: the previous coding added the duplicate ID to
`slaves.removed`, which will interfere with the activity of the other
slave whose ID we collided with.


Diffs (updated)
-

  src/master/master.hpp 4992ab0a0bb5babbf6a4fa3e6eff3577590fc879 
  src/master/master.cpp b88472f6350d3f71e057bab34822423da5427151 

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


Testing
---

`make check`


Thanks,

Neil Conway

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. 15, 2016, 1:34 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Adjusted existing tests for the change.


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/container_logger_tests.cpp 1b121a2fcce2d874aeefc4257b9d4e594866e78d 
  src/tests/containerizer/cni_isolator_tests.cpp 
0d611c196870b6adabea52a48abcd344c8dad5d1 
  src/tests/containerizer/docker_containerizer_tests.cpp 
1671d45171307cda62184505ce1dbadc476abca6 
  src/tests/containerizer/docker_volume_isolator_tests.cpp 
ca7bffd3b1773a11a4679d114885d3edd977b02b 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
df4642d2667407b1758ffe2efcfdbf9968cf2c33 
  src/tests/containerizer/isolator_tests.cpp 
9bb1e69209f34b18b5b64c3daf5ea26780f2ab74 
  src/tests/master_validation_tests.cpp 
16c5773aa44016f923e00cb348ded6b8c46d4b4b 
  src/tests/slave_tests.cpp d9d0a6abdfd56712ddad0a9afebd451c0ec51fc1 

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]

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

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


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Added logs in docker containerizer as well.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

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

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


Testing
---


Thanks,

Alexander Rukletsov

Review Request 51913: Ignored agent registrations with duplicate agent IDs.

[Neil Conway]

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

Review request for mesos, Benjamin Mahler and Vinod Kone.


Repository: mesos


Description
---

If an agent attempts to register and is assigned an agent ID that
already appears in the registry, the likely cause is a UUID collision:
slave IDs are prefixed with master IDs, and master IDs are randomly
generated UUIDs. UUID collisions are extremely unlikely, so in practice
this should never happen.

This commit updates the comments to clarify how unlikely this situation
is. Furthermore, when it does happen, the master now just ignores the
registration attempt, rather than trying to shutdown the agent. This is
simpler; the agent will eventually try to register again and be assigned
a new agent ID. Finally, it is unclear that shutting down the agent is
actually appropriate: the previous coding added the duplicate ID to
`slaves.removed`, which will interfere with the activity of the other
slave whose ID we collided with.


Diffs
-

  src/master/master.hpp 4992ab0a0bb5babbf6a4fa3e6eff3577590fc879 
  src/master/master.cpp b88472f6350d3f71e057bab34822423da5427151 

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


Testing
---

`make check`


Thanks,

Neil Conway

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46187]

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. 15, 2016, 8:57 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46187/
> ---
> 
> (Updated Sept. 15, 2016, 8:57 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5276
> https://issues.apache.org/jira/browse/MESOS-5276
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Terminate when receiving the ACK of terminal status update.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 5370634ef9e6f3ac9717fed71f6a77707026a16a 
> 
> Diff: https://reviews.apache.org/r/46187/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush

[Anastasia Braginsky]

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

(Updated Sept. 15, 2016, 12:03 p.m.)


Review request for mesos.


Repository: hbase-git


Description
---

This is a step toward final compacting memstore that allowes two modes of work: 
index-compaction and data-compaction. 

The index-compaction means that when the new segment is pushed into the 
pipeline, it is flattened and probably merged with old segments in the 
pipeline. The new merge "feature" induces no data-copy-compaction and no 
speculative SQM scan. 
The compacting memstore of the data-compaction type means the usage of the 
data-copy-compaction.


Diffs (updated)
-

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

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


Testing
---


Thanks,

Anastasia Braginsky

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/#review149048
---



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. 15, 2016, 9:13 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51877/
> ---
> 
> (Updated Sept. 15, 2016, 9:13 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 `Resource` objects are implicitly converted to `Resource_`
> objects.
> 
> 
> 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 51909: Added benchmarks for new registry operations.

[Neil Conway]

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

(Updated Sept. 15, 2016, 11:13 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Add benchmark results.


Repository: mesos


Description
---

These benchmarks cover:

  1. The time taken to make all registered agents unreachable and then
 reachable. This is similar to what happens during a severe network
 partition.

  2. The time taken to GC a significant fraction (50%) of the
 unreachable list from the registry.


Diffs
-

  src/tests/registrar_tests.cpp b04fc92c66449f1094212a534899e16d5f6f01bd 

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


Testing (updated)
---

make check

The performance of the new registry operations seems quite acceptable for 
reasonable cluster sizes, despite the fact that `PruneUnreachable` is quadratic 
time and `MarkSlaveReachable` and `MarkSlaveUnreachable` are both linear time. 
For the time being, optimizing these operations doesn't seem like it is needed 
urgently.

```
[ RUN  ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/0
Garbage collected 5000 agents in 166351us
[   OK ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/0 (7316 ms)
[ RUN  ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/1
Garbage collected 1 agents in 427487us
[   OK ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/1 (15048 ms)
[ RUN  ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/2
Garbage collected 15000 agents in 587876us
[   OK ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/2 (22844 ms)
[ RUN  ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/3
Garbage collected 25000 agents in 1.170591secs
[   OK ] SlaveCount/Registrar_BENCHMARK_Test.GcManyAgents/3 (49469 ms)
[ RUN  ] SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/0
Marked 1 agents unreachable in 3.040816secs
Marked 1 agents reachable in 2.322533secs
[   OK ] SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/0 
(7490 ms)
[ RUN  ] SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/1
Marked 2 agents unreachable in 8.192604secs
Marked 2 agents reachable in 5.041321secs
[   OK ] SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/1 
(18846 ms)
[ RUN  ] SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/2
Marked 3 agents unreachable in 15.21931secs
Marked 3 agents reachable in 8.857088secs
[   OK ] SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/2 
(32296 ms)
[ RUN  ] SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/3
Marked 5 agents unreachable in 39.277686secs
Marked 5 agents reachable in 18.483256secs
[   OK ] SlaveCount/Registrar_BENCHMARK_Test.MarkUnreachableThenReachable/3 
(73369 ms)
```


Thanks,

Neil Conway

Review Request 51909: Added benchmarks for new registry operations.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

These benchmarks cover:

  1. The time taken to make all registered agents unreachable and then
 reachable. This is similar to what happens during a severe network
 partition.

  2. The time taken to GC a significant fraction (50%) of the
 unreachable list from the registry.


Diffs
-

  src/tests/registrar_tests.cpp b04fc92c66449f1094212a534899e16d5f6f01bd 

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


Testing
---

make check

The performance of the new registry operations seems quite acceptable for 
reasonable cluster sizes, despite the fact that `PruneUnreachable` is quadratic 
time and `MarkSlaveReachable` and `MarkSlaveUnreachable` are both linear time. 
For the time being, optimizing these operations doesn't seem like it is needed 
urgently.


Thanks,

Neil Conway

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

[Qian Zhang]

> On Sept. 6, 2016, 7:33 p.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.
> 
> Huadong Liu wrote:
> 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, please see my comments below:

* https://github.com/apache/mesos/blob/master/src/launcher/executor.cpp is the 
Mesos command executor which is used by Mesos containerizer, it actually can 
run in two modes (controlled by the agent flag `--http_command_executor`):
   1. HTTP based mode (`--http_command_executor=true`): In this mode, it will 
use the HTTP based executor library to interact with Mesos agent.
   2. Adapter based mode (`--http_command_executor=false`): In this mode, it 
will use an adapter (`V0ToV1Adapter`) to call the old `ExecutorDriver` to 
interact with Mesos agent.
* 

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/#review149040
---



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. 15, 2016, 8:48 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51683/
> ---
> 
> (Updated Sept. 15, 2016, 8:48 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 aa4d6b5af9c24e90cefaa4f4c1ad593966eceb7c 
>   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 fa6789b0436b11a4f1baade9b5cbae0a04e1687d 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 266c2a0ff4a99baa96a7c4980f076755603256a9 
>   src/tests/reservation_endpoints_tests.cpp 
> bee5ea6b3a3ee7fafb7312a6d89de79d62c57ec1 
>   src/tests/reservation_tests.cpp 93e95a9ba33051b9506714a9578118569c09f7e6 
>   src/tests/resources_tests.cpp 157b9f65945b864e463c62a89281a3c80a85929d 
>   src/tests/role_tests.cpp 162c9414a70723a212cfd39ac5c0b8325c3f3b5d 
>   src/v1/resources.cpp 4b66acbba92bc6316fa77ae4dcabce9fab65f7b5 
> 
> 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
> 
>

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

[Guangya Liu]

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

(Updated 九月 15, 2016, 9:13 a.m.)


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


Repository: mesos


Description (updated)
---

The API for += and -= `Resource` object are now removed, we should
update the comments for  += and -= `Resource_` object by telling
end user `Resource` objects are implicitly converted to `Resource_`
objects.


Diffs (updated)
-

  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 51877: Updated comments for += and -= `Resource_` object.

[Guangya Liu]

> On 九月 14, 2016, 6 p.m., Jiang Yan Xu wrote:
> > 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.

Yes, but I think that it is still deserved to update the comments as most of 
the APIs are still accepting the `Resource` object but not `Resource_` object, 
so it is better to mention `that Resource objects are implicitly converted to 
Resource_` in comments.


- Guangya


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


On 九月 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 九月 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 46187: Terminate when receiving the ACK of terminal status update.

[Qian Zhang]

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

(Updated Sept. 15, 2016, 4:57 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Addressed @Vinod's comments.


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


Repository: mesos


Description
---

Terminate when receiving the ACK of terminal status update.


Diffs (updated)
-

  src/launcher/executor.cpp 5370634ef9e6f3ac9717fed71f6a77707026a16a 

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


Testing
---

make check


Thanks,

Qian Zhang

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 九月 15, 2016, 8:48 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 (updated)
-

  include/mesos/resources.hpp 7ba422d57d4e058c682df9aa780557782423 
  include/mesos/v1/resources.hpp add48c7baf8fabf5cc443d60a6c96d2902fc67de 
  src/cli/execute.cpp c9f56af7f37d5b79b51f350d6c533714c170e889 
  src/common/resources.cpp aa4d6b5af9c24e90cefaa4f4c1ad593966eceb7c 
  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 fa6789b0436b11a4f1baade9b5cbae0a04e1687d 
  src/tests/persistent_volume_endpoints_tests.cpp 
266c2a0ff4a99baa96a7c4980f076755603256a9 
  src/tests/reservation_endpoints_tests.cpp 
bee5ea6b3a3ee7fafb7312a6d89de79d62c57ec1 
  src/tests/reservation_tests.cpp 93e95a9ba33051b9506714a9578118569c09f7e6 
  src/tests/resources_tests.cpp 157b9f65945b864e463c62a89281a3c80a85929d 
  src/tests/role_tests.cpp 162c9414a70723a212cfd39ac5c0b8325c3f3b5d 
  src/v1/resources.cpp 4b66acbba92bc6316fa77ae4dcabce9fab65f7b5 

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

Re: Review Request 51375: Introduced MockRegistrar.

[Neil Conway]

> On Sept. 14, 2016, 8:31 p.m., Vinod Kone wrote:
> > src/tests/cluster.hpp, line 137
> > 
> >
> > Can you add a NOTE here regarding why you declared it here? You can use 
> > most of the text from your review comment.

Per discussion, existing comment seems sufficient.


- Neil


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


On Sept. 15, 2016, 7:36 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51375/
> ---
> 
> (Updated Sept. 15, 2016, 7:36 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 f1d202ae08d6bb4fd9e11eb1eae75dd9d5d9d8d5 
>   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. 15, 2016, 7:36 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebase.


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 f1d202ae08d6bb4fd9e11eb1eae75dd9d5d9d8d5 
  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 51905: Added logging when sending a queued task group to the executor.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51904, 51905]

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. 15, 2016, 4:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51905/
> ---
> 
> (Updated Sept. 15, 2016, 4:34 a.m.)
> 
> 
> 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 51683: Avoided resource validation when flatten resources.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/cli/execute.cpp (line 379)


```
// `frameworkInfo.role()` must be valid as it's allowed to register.
```



src/common/resources.cpp (line 1529)


Don't shadow the `flattened` above. Use `_flattened`.

Same for the v1 version.



src/common/resources.cpp (line 1535)


Ditto.

Same for the v1 version.


- Jiang Yan Xu


On Sept. 13, 2016, 11:09 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51683/
> ---
> 
> (Updated Sept. 13, 2016, 11:09 p.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
> 
>