Re: Review Request 52516: Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51774, 52584, 52515, 52516]

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 Oct. 5, 2016, 11:48 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52516/
> ---
> 
> (Updated Oct. 5, 2016, 11:48 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-6102
> https://issues.apache.org/jira/browse/MESOS-6102
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 
> 
> Diff: https://reviews.apache.org/r/52516/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52347: Send last `TaskStatus` in `TaskUpdated`.

[Zhitao Li]

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

(Updated Oct. 6, 2016, 5:13 a.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Vinod Kone.


Changes
---

Fix the state.


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


Repository: mesos


Description
---

Review: https://reviews.apache.org/r/52347


Diffs (updated)
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 
  src/common/protobuf_utils.hpp c5e5a9ad10134bcf87b59460bec7152e4916c75d 
  src/common/protobuf_utils.cpp 3ff37f498d6c830eb2f4b3d9395f9416cb01f5d3 
  src/master/master.cpp 66a672f6d16233e96b29e330a9e6c474546fa851 
  src/tests/api_tests.cpp e857b17cfe5f05d59859263c025564d33700a26c 

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


Testing
---

unit test.


Thanks,

Zhitao Li

Re: Review Request 52311: Pass the user value from executor of switch_user flag is set.

[Sivaram Kannan]

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

(Updated Oct. 6, 2016, 3:30 a.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description (updated)
---

Pass the user value from executor of switch_user flag is set.


Diffs (updated)
-

  src/slave/slave.cpp d30001bc0d1798311ba3966f67aadc6be2c92306 

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


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52574: Parameterized the image volume tests for nested containers.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52538, 52539, 52540, 52541, 52546, 52547, 52572, 52573, 52574]

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 Oct. 5, 2016, 9:15 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52574/
> ---
> 
> (Updated Oct. 5, 2016, 9:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Parameterized the image volume tests for nested containers.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/volume_image_isolator_tests.cpp PRE-CREATION 
>   src/tests/mesos.hpp a05e3e0fad92169dc550f1a1a8b704074716d99e 
> 
> Diff: https://reviews.apache.org/r/52574/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 51784: Supported merging the launch command from isolators.

[Jie Yu]

> On Sept. 12, 2016, 8:16 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1109
> > 
> >
> > Could we just have `arguments` MergeFrom? Because singular field (e.g., 
> > `value`) may be overwritten by some user modules accidentally.
> 
> Benjamin Bannier wrote:
> +1
> 
> Taking this a step further, given own function to merge a list of 
> `CommandInfo` into a single `CommandInfo`, e.g., `[CommandInfo] -> 
> Try(CommandInfo)`, would give us a chance to sanitize (e.g., are all 
> non-ignored scalar fields compatible?), and explicitly drop ignored elements 
> in a single, exposed spot.
> 
> Benjamin Bannier wrote:
> What is the status on this?
> 
> I still believe it is important to verify that isolators play nice with 
> each other as writers of isolators have very little control over this. I have 
> sketched this on top of this change over here, 
> https://github.com/bbannier/mesos/tree/r/51784. Currently that code emits 
> `VLOG` messages whenever `CommandInfo`s from `ContainerLaunchInfo`s not as 
> compatible as we wish, but I think ultimately we'd want to fail hard for such 
> scenarios.

OK, I'll add a LOG(WARNING) here. I think ultimately, this hack will be removed 
when we switch to use default executor for launching command tasks (using 
nested launch API).


- Jie


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


On Sept. 11, 2016, 12:17 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51784/
> ---
> 
> (Updated Sept. 11, 2016, 12:17 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gilbert Song.
> 
> 
> Bugs: MESOS-5275
> https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we only allow one isolator to specify the launch command
> for the container. This is not ideal because multiple isolators might
> want to add some flags to the command executor. For instance, the
> 'docker/runtime' isolator wants to specify '--task_command' and
> '--working_directory', and 'linux/capabilities' isolator wants to
> specify '--capabilities'.
> 
> This patch changes the semantics so that launch command from isolators
> are merged. However, it is isolator's responsibility to make sure the
> merged command is a valid command.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 89b7e8db38916d69d9b2d4fe305d4397b0859a10 
> 
> Diff: https://reviews.apache.org/r/51784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52309: Pass the user variable from library to binary.

[Sivaram Kannan]

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

(Updated Oct. 6, 2016, 3:07 a.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Pass the user variable from library to binary.


Diffs (updated)
-

  src/slave/container_loggers/lib_logrotate.cpp 
0ca2b3d7dbb57c11c0740aed3914a6b75329af99 

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


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52310: Switch the uid of the binary if a user is passed from the lib_logrotate.

[Sivaram Kannan]

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

(Updated Oct. 6, 2016, 3:07 a.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Switch the uid of the binary if a user is passed from the lib_logrotate.


Diffs (updated)
-

  src/slave/container_loggers/logrotate.cpp 
431bc3cbb54e94359078e4dae0b32ad301393640 

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


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Review Request 52593: Mesos is over

[Chris Lambert]

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

Review request for mesos.


Repository: mesos


Description
---

Fuck Mesos


Diffs
-

  src/slave/http.cpp bdafe1d 
  src/tests/api_tests.cpp 7b0ad3c 

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


Testing
---


Thanks,

Chris Lambert

Re: Review Request 52465: Fixed the race in master update slave.

[Guangya Liu]

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

(Updated 十月 6, 2016, 2:23 a.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description (updated)
---

The reason that we need `updateSlave` first and then rescind offer
is because of a race condition case: there may be a batch allocation
triggered between rescind offer and `updateSlave`. In this case, the
order will be rescind offer -> batch allocation -> update slave. This
order will cause some issues when the oversubscribed resources was
decreased.

Suppose the oversubscribed resources was decreased from 2 to 1, then
after rescind offer finished, the batch allocation will allocate the
old 2 oversubscribed resources again, then update slave will update
the total oversubscribed resources to 1. This will cause the agent
host have some time overcommitted due to the tasks can still use 2
oversubscribed resources but not 1 oversubscribed resources, once
the tasks using the 2 oversubscribed resources finished, everything
goes back.

If we update slave first then rescind offer, the order will be update
slave -> batch allocation -> rescind offer, this order will have no
problem when shrinking resources. Suppose the oversubscribed resources
was shrinked from 2 to 1, then update slave will update total
oversubscribed resources to 1 directly, then the batch allocation will
not allocate any oversubscribed resources since there are more
allocated than total oversubscribed resources, then rescind offer
will rescind all offers using oversubscribed resources. This will
not lead the agent host to be overcommitted.


Diffs (updated)
-

  src/master/master.cpp 02a2fb29bdd8484fc90e5cb033ac29b49a141860 
  src/tests/oversubscription_tests.cpp 3dd34ea78ac795a6b0d342dcae86642c51841eea 

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


Testing (updated)
---

make
make check

```
./bin/mesos-tests.sh  
--gtest_filter="OversubscriptionTest.RescindRevocableOffer*" --gtest_repeat=20
```


Thanks,

Guangya Liu

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

[Anindya Sinha]

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

(Updated Oct. 6, 2016, 2 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added unit tests to determine disk size for MOUNT disks.


Diffs (updated)
-

  src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 
  src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 

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


Testing
---

All tests including the additional tests in this RR passed.


Thanks,

Anindya Sinha

Re: Review Request 52071: Updated docs to handle resources with no size in agent flags.

[Anindya Sinha]

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

(Updated Oct. 6, 2016, 2:01 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

When a resource with no size is specified in `--resources` startup
flag of mesos agent, the size is auto detected by the agent. This
is enabled for all known resource types (except gpus).
For disk resources, this is done for both root disks as well as
MOUNT disks, but is not allowed for PATH disks since PATH disks can
be carved into smaller volumes.


Diffs (updated)
-

  docs/attributes-resources.md 2caa44927d45c0ab13f8160794b50f4fde3711aa 

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


Testing
---

Documentation change only. All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources.

[Anindya Sinha]

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

(Updated Oct. 6, 2016, 2 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments, and rebased.


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


Repository: mesos


Description (updated)
---

Refactored `Resources::parse()` into 2 separate static functions:
1. Resources::fromJSON() to parse JSON representation of resources.
2. Resources::fromSimpleString() to parse text representation of
   resources.

Since these 2 new functions return a `Try>`, the
existing `Resources::parse()` implicitly converts that to a
`Resources` object. This refactor is done to retrieve all resources
(include empty resources) required for auto detection of root
and MOUNT disks.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51879: Autodetect value of resource when not specified in static resources.

[Anindya Sinha]

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

(Updated Oct. 6, 2016, 2 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Updates based on previous changes in this chain, and rebased.


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


Repository: mesos


Description
---

When static resources indicate resources with a positive size, we use
that for the resources on the agent. However, --resources can include
resources with no size, which indicates that mesos agent determine the
size of those resources from the agent and uses that information. For
disk resources, 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 disk 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 (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 
  src/slave/containerizer/mesos/isolators/gpu/allocator.hpp 
b2eabfebef99ccebef427d144bb816adc0175ecf 
  src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
2e722691475c84afae14009014ea70cc0fdd0e65 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

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

(Updated Oct. 6, 2016, 2 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52569: Updated the nested container launch to correctly determine the user.

[Benjamin Mahler]

> On Oct. 6, 2016, 12:34 a.m., Vinod Kone wrote:
> > src/slave/slave.hpp, line 916
> > 
> >
> > This and the corresponding changes in slave.cpp could've been done in a 
> > separate review?

Yeah that's true, I'll split them out before committing.


> On Oct. 6, 2016, 12:34 a.m., Vinod Kone wrote:
> > src/tests/api_tests.cpp, lines 3356-3370
> > 
> >
> > could have used the LaunchTask action?

Ah I see, there are some others in this file that could use it as well, I just 
copied this. Will update just these tests for now.


- Benjamin


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


On Oct. 5, 2016, 8:45 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52569/
> ---
> 
> (Updated Oct. 5, 2016, 8:45 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Vinod Kone.
> 
> 
> Bugs: MESOS-6289
> https://issues.apache.org/jira/browse/MESOS-6289
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This locates the executor that corresponds to the parent container,
> and uses the user that was used to launch the executor.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp bdafe1db9f1b22ac0761f5119e73f87b531c6a3c 
>   src/slave/slave.hpp 9beefa12283e936684d82c97ebd3ac6506475edf 
>   src/slave/slave.cpp fba089506850eec87f67a46694a1ef645cdf4ad5 
>   src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 
> 
> Diff: https://reviews.apache.org/r/52569/diff/
> 
> 
> Testing
> ---
> 
> Now that the executor is looked up, the tests have been updated to
> launch an executor and use it's container id as the parent.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 50556: Added benchmark test for `Resources::filter`.

[Guangya Liu]

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

(Updated 十月 6, 2016, 1:44 a.m.)


Review request for mesos, Benjamin Mahler, Joris Van Remoortere, Klaus Ma, and 
Jiang Yan Xu.


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


Repository: mesos


Description
---

This benchmark test is using nonRevocable, revocable, unreserved
and reserved API benchmark test to test the API of filter for
Resources.


Diffs (updated)
-

  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 

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


Testing (updated)
---

make
make check

```
./bin/mesos-tests.sh  --benchmark 
--gtest_filter="Resources_Filter_BENCHMARK_Test.Filters"
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from Resources_Filter_BENCHMARK_Test
[ RUN  ] Resources_Filter_BENCHMARK_Test.Filters
Took 351728us to perform 5 'r.nonRevocable()' operations on cpus(*):1; 
gpus(*):1; mem(*):128; disk(*):256
Took 65710us to perform 5 'r.revocable()' operations on cpus(*){REV}:1
Took 351148us to perform 5 'r.unreserved()' operations on cpus(*):1; 
gpus(*):1; mem(*):128; disk(*):256
Took 418719us to perform 5 'r.reserved(role)' operations on cpus(role):1; 
gpus(role):1; mem(role):128; disk(role):256
[   OK ] Resources_Filter_BENCHMARK_Test.Filters (1190 ms)
[--] 1 test from Resources_Filter_BENCHMARK_Test (1190 ms total)

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


Thanks,

Guangya Liu

Re: Review Request 52568: Added support for nested containers to the test containerizer.

[Benjamin Mahler]

> On Oct. 6, 2016, 12:11 a.m., Vinod Kone wrote:
> > src/tests/containerizer.cpp, lines 128-129
> > 
> >
> > why do you use `at` here but `[]` up above in the constructors?

Ah thanks I try to use `.at` anywhere that I want retrieval with no insertion, 
so I'll update the constructor code accordingly.


- Benjamin


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


On Oct. 5, 2016, 8:45 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52568/
> ---
> 
> (Updated Oct. 5, 2016, 8:45 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the process, the maps that were stored were simplified.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer.hpp 3cf418a040dfedad2eb6d771e7cc9cc179bcf55c 
>   src/tests/containerizer.cpp 89c5400d3a2e771278ba370a939a6c68bc57a31f 
> 
> Diff: https://reviews.apache.org/r/52568/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

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

[Benjamin Mahler]

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


Ship it!




Looks like the review summary is a bit inaccurate? This documents `add` and 
`subtract`.


include/mesos/resources.hpp (line 505)


We try to have newlines between comment paragraphs.


- Benjamin Mahler


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 52347: Send last `TaskStatus` in `TaskUpdated`.

[Zhitao Li]

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

(Updated Oct. 6, 2016, 12:45 a.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Vinod Kone.


Changes
---

Adjust field ordering.


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


Repository: mesos


Description
---

Review: https://reviews.apache.org/r/52347


Diffs (updated)
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 
  src/common/protobuf_utils.hpp c5e5a9ad10134bcf87b59460bec7152e4916c75d 
  src/common/protobuf_utils.cpp 3ff37f498d6c830eb2f4b3d9395f9416cb01f5d3 
  src/master/master.cpp 66a672f6d16233e96b29e330a9e6c474546fa851 
  src/tests/api_tests.cpp e857b17cfe5f05d59859263c025564d33700a26c 

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


Testing
---

unit test.


Thanks,

Zhitao Li

Re: Review Request 52543: Added configure/make options to build the new CLI and run unit tests.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 5, 2016, 4:09 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52543/
> ---
> 
> (Updated Oct. 5, 2016, 4:09 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-6008 and MESOS-6032
> https://issues.apache.org/jira/browse/MESOS-6008
> https://issues.apache.org/jira/browse/MESOS-6032
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added configure/make options to build the new CLI and run unit tests.
> 
> 
> Diffs
> -
> 
>   configure.ac 8f971c96bfea0afd7462ea9b91c69458324435b6 
>   docs/getting-started.md de3fba4a821ca7fa9b9e2fd692b33b40edd35b68 
>   src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 
> 
> Diff: https://reviews.apache.org/r/52543/diff/
> 
> 
> Testing
> ---
> 
> ../configure --enable-new-cli
> make -C src mesos
> src/mesos
> 
> GTEST_FILTER="" make -j check
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51008: Added infrastructure for unit tests in the new python-based CLI.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 5, 2016, 4:11 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51008/
> ---
> 
> (Updated Oct. 5, 2016, 4:11 a.m.)
> 
> 
> Review request for mesos, Haris Choudhary and Joseph Wu.
> 
> 
> Bugs: MESOS-6032
> https://issues.apache.org/jira/browse/MESOS-6032
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added infrastructure for unit tests in the new python-based CLI.
> 
> 
> Diffs
> -
> 
>   src/cli_new/bin/mesos-tests PRE-CREATION 
>   src/cli_new/bin/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51008/diff/
> 
> 
> Testing
> ---
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 50912: Added the infrastructure for a new python-based CLI.

[Vinod Kone]

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



LGTM as much as I understood. I'll defer to Joseph for the final ship it.


src/cli_new/bin/config.py (lines 23 - 25)


reorder alphabetically



src/cli_new/bin/config.py (line 50)


s/parse/open/ ?



src/cli_new/bin/config.py (line 57)


s/json/JSON/



src/cli_new/bin/main.py (line 22)


why the space?

also alphabetical ordering.



src/cli_new/bin/main.py (line 87)


s/a/plugin/



src/cli_new/bootstrap (line 2)


Can you add a comment here about what this script does at a high level?



src/cli_new/bootstrap (line 12)


I got confused between VIRTUALENV which represents its location and 
VIRTUAL_ENV below. maybe call this VIRTUALENV_PATH ?



src/cli_new/bootstrap (line 19)


who sets VIRTUAL_ENV env variable?



src/cli_new/mesos.bash_completion (line 5)


s/it/to it/


- Vinod Kone


On Oct. 5, 2016, 4:12 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50912/
> ---
> 
> (Updated Oct. 5, 2016, 4:12 a.m.)
> 
> 
> Review request for mesos, Haris Choudhary and Joseph Wu.
> 
> 
> Bugs: MESOS-6008
> https://issues.apache.org/jira/browse/MESOS-6008
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the infrastructure for a new python-based CLI.
> 
> 
> Diffs
> -
> 
>   src/cli_new/.gitignore PRE-CREATION 
>   src/cli_new/README.md PRE-CREATION 
>   src/cli_new/activate PRE-CREATION 
>   src/cli_new/bin/config.py PRE-CREATION 
>   src/cli_new/bin/main.py PRE-CREATION 
>   src/cli_new/bin/mesos PRE-CREATION 
>   src/cli_new/bootstrap PRE-CREATION 
>   src/cli_new/deactivate PRE-CREATION 
>   src/cli_new/lib/mesos/__init__.py PRE-CREATION 
>   src/cli_new/lib/mesos/docopt.py PRE-CREATION 
>   src/cli_new/lib/mesos/exceptions.py PRE-CREATION 
>   src/cli_new/lib/mesos/plugins/__init__.py PRE-CREATION 
>   src/cli_new/lib/mesos/plugins/base.py PRE-CREATION 
>   src/cli_new/lib/mesos/util.py PRE-CREATION 
>   src/cli_new/mesos.bash_completion PRE-CREATION 
>   src/cli_new/pip-requirements.txt PRE-CREATION 
>   src/cli_new/pylint.config PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50912/diff/
> 
> 
> Testing
> ---
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos --help
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52347: Send last `TaskStatus` in `TaskUpdated`.

[Zhitao Li]

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

(Updated Oct. 6, 2016, 12:35 a.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Vinod Kone.


Changes
---

Add `TaskState` back.


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


Repository: mesos


Description (updated)
---

Review: https://reviews.apache.org/r/52347


Diffs (updated)
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 
  src/common/protobuf_utils.hpp c5e5a9ad10134bcf87b59460bec7152e4916c75d 
  src/common/protobuf_utils.cpp 3ff37f498d6c830eb2f4b3d9395f9416cb01f5d3 
  src/master/master.cpp 66a672f6d16233e96b29e330a9e6c474546fa851 
  src/tests/api_tests.cpp e857b17cfe5f05d59859263c025564d33700a26c 

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


Testing
---

unit test.


Thanks,

Zhitao Li

Re: Review Request 52569: Updated the nested container launch to correctly determine the user.

[Vinod Kone]

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


Fix it, then Ship it!





src/slave/http.cpp (line 1954)


typo: executor



src/slave/slave.hpp (line 916)


This and the corresponding changes in slave.cpp could've been done in a 
separate review?



src/tests/api_tests.cpp (lines 3355 - 3369)


could have used the LaunchTask action?



src/tests/api_tests.cpp (lines 3490 - 3512)


ditto.


- Vinod Kone


On Oct. 5, 2016, 8:45 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52569/
> ---
> 
> (Updated Oct. 5, 2016, 8:45 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Vinod Kone.
> 
> 
> Bugs: MESOS-6289
> https://issues.apache.org/jira/browse/MESOS-6289
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This locates the executor that corresponds to the parent container,
> and uses the user that was used to launch the executor.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp bdafe1db9f1b22ac0761f5119e73f87b531c6a3c 
>   src/slave/slave.hpp 9beefa12283e936684d82c97ebd3ac6506475edf 
>   src/slave/slave.cpp fba089506850eec87f67a46694a1ef645cdf4ad5 
>   src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 
> 
> Diff: https://reviews.apache.org/r/52569/diff/
> 
> 
> Testing
> ---
> 
> Now that the executor is looked up, the tests have been updated to
> launch an executor and use it's container id as the parent.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 52587: Allow CREATE of shared volumes based on capability of framework.

[Anindya Sinha]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

When a framework issues a CREATE for a shared volume, we allow that
operation only if the framework has opted in for the capability of
SHARED_RESOURCES. However, for HTTP operator (/create-volumes), we
do not check as the operator API is not related to a specific
framework.


Diffs
-

  src/master/master.cpp 02a2fb29bdd8484fc90e5cb033ac29b49a141860 
  src/master/validation.hpp 035f721c610ae566c89a1cf0e65ff0df11679f15 
  src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
  src/tests/master_validation_tests.cpp 
99e350e0587e73e9ee25ef20dd369cd146bd446a 
  src/tests/persistent_volume_tests.cpp 
e10a79e9662530e143ccaa2aa2506a4d25158364 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52347: Send last `TaskStatus` in `TaskUpdated`.

[Zhitao Li]

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

(Updated Oct. 6, 2016, 12:18 a.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Send last `TaskStatus` in `TaskUpdated`.


Diffs
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 
  src/common/protobuf_utils.hpp c5e5a9ad10134bcf87b59460bec7152e4916c75d 
  src/common/protobuf_utils.cpp 3ff37f498d6c830eb2f4b3d9395f9416cb01f5d3 
  src/master/master.cpp 66a672f6d16233e96b29e330a9e6c474546fa851 
  src/tests/api_tests.cpp e857b17cfe5f05d59859263c025564d33700a26c 

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


Testing
---

unit test.


Thanks,

Zhitao Li

Re: Review Request 52347: Send last `TaskStatus` in `TaskUpdated`.

[Zhitao Li]

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

(Updated Oct. 6, 2016, 12:17 a.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Vinod Kone.


Summary (updated)
-

Send last `TaskStatus` in `TaskUpdated`.


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


Repository: mesos


Description (updated)
---

Review: https://reviews.apache.org/r/52347


Diffs (updated)
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 
  src/common/protobuf_utils.hpp c5e5a9ad10134bcf87b59460bec7152e4916c75d 
  src/common/protobuf_utils.cpp 3ff37f498d6c830eb2f4b3d9395f9416cb01f5d3 
  src/master/master.cpp 66a672f6d16233e96b29e330a9e6c474546fa851 
  src/tests/api_tests.cpp e857b17cfe5f05d59859263c025564d33700a26c 

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


Testing
---

unit test.


Thanks,

Zhitao Li

Re: Review Request 52568: Added support for nested containers to the test containerizer.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/containerizer.hpp (line 134)


not yours, but new line here?



src/tests/containerizer.cpp (lines 125 - 126)


why do you use `at` here but `[]` up above in the constructors?


- Vinod Kone


On Oct. 5, 2016, 8:45 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52568/
> ---
> 
> (Updated Oct. 5, 2016, 8:45 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the process, the maps that were stored were simplified.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer.hpp 3cf418a040dfedad2eb6d771e7cc9cc179bcf55c 
>   src/tests/containerizer.cpp 89c5400d3a2e771278ba370a939a6c68bc57a31f 
> 
> Diff: https://reviews.apache.org/r/52568/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 52465: Fixed the race in master update slave.

[Benjamin Mahler]

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


Fix it, then Ship it!




Thanks for fixing this! Glad that you added a second test for the decreasing 
case.

Can you file a bug for this issue?


src/master/master.cpp (lines 5554 - 5573)


How about moving this up to above the update to the agent's resources?

Also I think we can make this a bit more succinct:

```
// NOTE: We must *first* update the agent's resources before we
// recover the resources. If we recovered the resources first,
// an allocation could trigger between recovering resources and
// updating the agent in the allocator. This would lead us to
// re-send out the stale oversubscribed resources!
```



src/tests/oversubscription_tests.cpp (line 443)


This says "if" but we should say something like "In this test the 
oversubscribed resources are increased, so the master will send out ...".



src/tests/oversubscription_tests.cpp (line 515)


Do you need this WillRepeatedly?



src/tests/oversubscription_tests.cpp (line 523)


Not yours, but can we move this pause to the top?



src/tests/oversubscription_tests.cpp (lines 533 - 535)


Mind lining this up against the parenthesis?

```
EXPECT_EQ(createRevocableResources("cpus", "2"),
  Resources(offers3.get()[0].resources()));
```



src/tests/oversubscription_tests.cpp (lines 542 - 544)


```
  // Advance the clock to trigger a batch allocation, this will
  // allocate the oversubscribed resources that were rescinded.
```



src/tests/oversubscription_tests.cpp (lines 558 - 563)


How about s/shrinked/decreased/ ?



src/tests/oversubscription_tests.cpp (lines 560 - 562)


Same comment here as above about the use of "if".



src/tests/oversubscription_tests.cpp (line 631)


Do you need this WillRepeatedly?



src/tests/oversubscription_tests.cpp (lines 633 - 634)


s/shrinked/decreased/



src/tests/oversubscription_tests.cpp (line 639)


Can this be moved up to the beginning of the test? 

(Eventually we want to force a paused clock for all tests to eliminate 
implicit reliance on time progressing, leads to too many flaky tests).



src/tests/oversubscription_tests.cpp (lines 646 - 648)


Ditto comment from the test above.


- Benjamin Mahler


On Oct. 1, 2016, 9:34 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52465/
> ---
> 
> (Updated Oct. 1, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The reason that we need `updateSlave` first and then rescind offer
> is because of a race condition case: there may be a batch allocation
> triggered between rescind offer and `updateSlave`. In this case, the
> order will be rescind offer -> batch allocation -> update slave. This
> order will cause some issues when the oversubscribed resources was
> shrinked.
> 
> Suppose the oversubscribed resources was shrinked from 2 to 1, then
> after rescind offer finished, the batch allocation will allocate the
> old 2 oversubscribed resources again, then update slave will update
> the total oversubscribed resources to 1. This will cause the agent
> host have some time overcommitted due to the tasks can still use 2
> oversubscribed resources but not 1 oversubscribed resources, once
> the tasks using the 2 oversubscribed resources finished, everything
> goes back.
> 
> If we update slave first then rescind offer, the order will be update
> slave -> batch allocation -> rescind offer, this order will have no
> problem when shrinking resources. Suppose the oversubscribed resources
> was shrinked from 2 to 1, then update slave will update total
> oversubscribed resources to 1 directly, then the batch allocation will
> not allocate any oversubscribed resources since there are more
> allocated than total oversubscribed resources, then rescind offer
> will rescind all offers using oversubscribed resources. This will
> not lead the age

Re: Review Request 52537: Added unit test for provisioner 'RecoverNestedContainerNoParentImage'.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Oct. 5, 2016, 12:50 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52537/
> ---
> 
> (Updated Oct. 5, 2016, 12:50 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-6302
> https://issues.apache.org/jira/browse/MESOS-6302
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit test for provisioner 'RecoverNestedContainerNoParentImage'.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 6ef1c926d7aa942e241a24d4d3838a5f2d7c4bd1 
> 
> Diff: https://reviews.apache.org/r/52537/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Verified that this test would fail if we removed the fix in 
> https://reviews.apache.org/r/52480/
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52567: Added a TODO for cleaning up the `Mesos` library lifetime issues.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 5, 2016, 8:44 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52567/
> ---
> 
> (Updated Oct. 5, 2016, 8:44 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `Mesos` library may invoke the provided callbacks after `Mesos`
> is destructed. This has necessitated callers to bind a shared pointer
> to the executor when creating the callbacks. However, this should not
> be necessary, and this adds a TODO to describe how to eliminate this
> issue.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp a05e3e0fad92169dc550f1a1a8b704074716d99e 
> 
> Diff: https://reviews.apache.org/r/52567/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 51946: Updated test for BadACLNoPrincipal and BadACLDropCreateAndDestroy.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Oct. 3, 2016, 9:53 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51946/
> ---
> 
> (Updated Oct. 3, 2016, 9:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Greg Mann, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6181
> https://issues.apache.org/jira/browse/MESOS-6181
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If destroy volume failed, we should get the latest offer to make
> sure that the latest offer contain the volume resource.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 2ab44d11d159162dfcac9d0791b651ed059b8164 
> 
> Diff: https://reviews.apache.org/r/51946/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh --gtest_filter="*PersistentVolumeTest.BadACL*/*"  
> --gtest_repeat=20
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 52516: Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.

[Zhitao Li]

> On Oct. 5, 2016, 6:14 p.m., Anand Mazumdar wrote:
> > src/tests/api_tests.cpp, lines 1458-1459
> > 
> >
> > Do you need this? Why can't we just wait on the `event` from the 
> > decoder.

I'm using this to capture `AgentID` now so that I can compare with later events.


- Zhitao


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


On Oct. 5, 2016, 11:48 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52516/
> ---
> 
> (Updated Oct. 5, 2016, 11:48 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-6102
> https://issues.apache.org/jira/browse/MESOS-6102
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 
> 
> Diff: https://reviews.apache.org/r/52516/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Benjamin Mahler]

> On Oct. 5, 2016, 1:02 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1313-1318
> > 
> >
> > The intention of making allocate asynchronous and returning a Future is 
> > that the caller could tell when the asynchronous allocation completes and 
> > could set up a continuation based on this.
> > 
> > As it stands this function just returns Nothing always? Seems it should 
> > return to the caller once the caller's requested allocation completes.
> 
> Guangya Liu wrote:
> Here we want to enqueue only one `allocate(slaves)` request and then 
> update the `allocationCandidates` for other event trigger allocation requests 
> before the `allocate(slaves)` request was processed, after the 
> `allocate(slaves)` request was processed, another `allocate(slaves)` request 
> will be enqueued again, so seems we do not need to wait till the allocation 
> completes, but just check the allocation status for each `allocate(slaves)` 
> request, comments?

By "return to the caller once the allocation completes" I mean "return a Future 
that is satisfied once the allocation completes".


- Benjamin


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


On Oct. 4, 2016, 11:31 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Oct. 4, 2016, 11:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Triggered allocations dispatch allocate() only
>   if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
>   cleared when enqueued allocations are processed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 52162: Fixed Python bindings build error caused by custom SVN location.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52162]

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 Oct. 5, 2016, 6:31 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52162/
> ---
> 
> (Updated Oct. 5, 2016, 6:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Kapil Arya, and Till Toenshoff.
> 
> 
> Bugs: MESOS-6207
> https://issues.apache.org/jira/browse/MESOS-6207
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved Python related configuration steps down below all others. Now 
> PYTHON_LDFLAGS variable captures all required LDFLAGS.
> 
> 
> Diffs
> -
> 
>   configure.ac 8f971c96bfea0afd7462ea9b91c69458324435b6 
> 
> Diff: https://reviews.apache.org/r/52162/diff/
> 
> 
> Testing
> ---
> 
> ```
> ../configure --with-svn=$HOME/svn && make && make check
> ```
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 52516: Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.

[Zhitao Li]

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

(Updated Oct. 5, 2016, 11:48 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


Changes
---

Review comments.


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


Repository: mesos


Description
---

Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.


Diffs (updated)
-

  src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 52515: Implement `AGENT_ADDED` and `AGENT_REMOVED` events.

[Zhitao Li]

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

(Updated Oct. 5, 2016, 11:47 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


Changes
---

Review comments.


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


Repository: mesos


Description
---

Implement `AGENT_ADDED` and `AGENT_REMOVED` events.


Diffs (updated)
-

  src/common/protobuf_utils.hpp 0c6405f6cfc5a69fa835428fa330d8d63778f469 
  src/common/protobuf_utils.cpp 1f088684c566f89c85dd72eaece295c5e3d50b23 
  src/master/http.cpp e9f9d16de803175d4f07e0486674e6150c632b02 
  src/master/master.cpp 756ab546851952bc1de24b1f469d232237b1c01c 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 52584: Create helper function `createAgentResponse`.

[Zhitao Li]

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

(Updated Oct. 5, 2016, 11:46 p.m.)


Review request for mesos, Anand Mazumdar and Xiaojian Huang.


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


Repository: mesos


Description
---

Create helper function `createAgentResponse`.


Diffs
-

  src/common/protobuf_utils.hpp 0c6405f6cfc5a69fa835428fa330d8d63778f469 
  src/common/protobuf_utils.cpp 1f088684c566f89c85dd72eaece295c5e3d50b23 
  src/master/http.cpp e9f9d16de803175d4f07e0486674e6150c632b02 

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


Testing
---


Thanks,

Zhitao Li

Review Request 52584: Create helper function `createAgentResponse`.

[Zhitao Li]

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

Review request for mesos.


Repository: mesos


Description
---

Create helper function `createAgentResponse`.


Diffs
-

  src/common/protobuf_utils.hpp 0c6405f6cfc5a69fa835428fa330d8d63778f469 
  src/common/protobuf_utils.cpp 1f088684c566f89c85dd72eaece295c5e3d50b23 
  src/master/http.cpp e9f9d16de803175d4f07e0486674e6150c632b02 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 51774: Add `AGENT_ADDED` and `AGENT_REMOVED` master events.

[Zhitao Li]

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

(Updated Oct. 5, 2016, 11:45 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


Changes
---

Review comments.


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


Repository: mesos


Description
---

Add `AGENT_ADDED` and `AGENT_REMOVED` master events.


Diffs (updated)
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 

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


Testing
---

New unit test.


Thanks,

Zhitao Li

Re: Review Request 52563: Minor clean ups in `health_check_tests.cpp` for consistency.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52250, 52251, 52560, 52561, 52563]

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 Oct. 5, 2016, 6:11 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52563/
> ---
> 
> (Updated Oct. 5, 2016, 6:11 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * Replace `Testing` to `Tests` in comments.
> * Remove redundant `.Times(1)`.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 
> 
> Diff: https://reviews.apache.org/r/52563/diff/
> 
> 
> Testing
> ---
> 
> ```
> sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="*HealthCheckTest.*" 
> --gtest_also_run_disabled_tests
> [--] Global test environment tear-down
> [==] 19 tests from 1 test case ran. (35264 ms total)
> [  PASSED  ] 19 tests.
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52536: Refactored the provisioner recover test.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Oct. 5, 2016, 12:50 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52536/
> ---
> 
> (Updated Oct. 5, 2016, 12:50 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored the provisioner recover test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 6ef1c926d7aa942e241a24d4d3838a5f2d7c4bd1 
> 
> Diff: https://reviews.apache.org/r/52536/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52576: Added MESOS-6269 to 1.0.2 CHANGELOG.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On Oct. 5, 2016, 9:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52576/
> ---
> 
> (Updated Oct. 5, 2016, 9:36 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Vinod Kone.
> 
> 
> Bugs: MESOS-6269
> https://issues.apache.org/jira/browse/MESOS-6269
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added MESOS-6269 to 1.0.2 CHANGELOG.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 8312e85014a1f764c82b91e704517dfac595d5c9 
> 
> Diff: https://reviews.apache.org/r/52576/diff/
> 
> 
> Testing
> ---
> 
> trivial.
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52576: Added MESOS-6269 to 1.0.2 CHANGELOG.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 5, 2016, 9:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52576/
> ---
> 
> (Updated Oct. 5, 2016, 9:36 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Vinod Kone.
> 
> 
> Bugs: MESOS-6269
> https://issues.apache.org/jira/browse/MESOS-6269
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added MESOS-6269 to 1.0.2 CHANGELOG.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 8312e85014a1f764c82b91e704517dfac595d5c9 
> 
> Diff: https://reviews.apache.org/r/52576/diff/
> 
> 
> Testing
> ---
> 
> trivial.
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 52576: Added MESOS-6269 to 1.0.2 CHANGELOG.

[Jie Yu]

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

Review request for mesos, Avinash sridharan and Vinod Kone.


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


Repository: mesos


Description
---

Added MESOS-6269 to 1.0.2 CHANGELOG.


Diffs
-

  CHANGELOG 8312e85014a1f764c82b91e704517dfac595d5c9 

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


Testing
---

trivial.


Thanks,

Jie Yu

Review Request 52572: Used unmountAll to replace the original shell script.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Gilbert Song.


Repository: mesos


Description
---

Used unmountAll to replace the original shell script.


Diffs
-

  src/tests/environment.cpp fba94f6e3c810a7907ff180e19a770c36d569374 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 51879: Autodetect value of resource when not specified in static resources.

[Jiang Yan Xu]

> On Sept. 28, 2016, 3:37 p.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 587-618
> > 
> >
> > No need for the helpers. Just the following is sufficient.
> > 
> > ```
> > Resource resource;
> > resource.set_name(name);
> > resource.set_role(role);
> > 
> > // Return a Resource with missing value.
> > if (_value.isNone()) {
> >   return resource;
> > }
> > 
> > Value _value = result.get();
> > 
> > if (_value.type() == Value::SCALAR) {
> >   resource.set_type(Value::SCALAR);
> >   resource.mutable_scalar()->CopyFrom(_value.scalar());
> > } else if (_value.type() == Value::RANGES) {
> >   resource.set_type(Value::RANGES);
> >   resource.mutable_ranges()->CopyFrom(_value.ranges());
> > } else if (_value.type() == Value::SET) {
> >   resource.set_type(Value::SET);
> >   resource.mutable_set()->CopyFrom(_value.set());
> > } else {
> >   return Error(
> >   "Bad type for resource " + name + " value " + value +
> >   " type " + Value::Type_Name(_value.type()));
> > }
> > ```
> 
> Guangya Liu wrote:
> Seems we still need the `type` for resource when `_value.isNone()`, so 
> the logic could be:
> 
> ```
> Resource resource;
> resource.set_name(name);
> resource.set_role(role);
> 
> // Return a Resource with missing value.
> if (_value.isNone()) {
>   Try _type = Resources::valueType(name);
>   if (_type.isError()) {
> return Error(_type.error());
>   }
>   
>   resource.set_type(_type.get());
>   return resource;
> }
> 
> Value _value = result.get();
> 
> if (_value.type() == Value::SCALAR) {
>   resource.set_type(Value::SCALAR);
>   resource.mutable_scalar()->CopyFrom(_value.scalar());
> } else if (_value.type() == Value::RANGES) {
>   resource.set_type(Value::RANGES);
>   resource.mutable_ranges()->CopyFrom(_value.ranges());
> } else if (_value.type() == Value::SET) {
>   resource.set_type(Value::SET);
>   resource.mutable_set()->CopyFrom(_value.set());
> } else {
>   return Error(
>   "Bad type for resource " + name + " value " + value +
>   " type " + Value::Type_Name(_value.type()));
> }
> ```

I don't think so. The type is based on the value. No value, no type. After we 
detect the value, we'll have the type, no?


- Jiang Yan


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


On Oct. 3, 2016, 4:46 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> ---
> 
> (Updated Oct. 3, 2016, 4:46 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 resources with a positive size, we use
> that for the resources on the agent. However, --resources can include
> resources with no size, which indicates that mesos agent determine the
> size of those resources from the agent and uses that information. For
> disk resources, 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 disk 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 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.hpp 
> b2eabfebef99ccebef427d144bb816adc0175ecf 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
> 2e722691475c84afae14009014ea70cc0fdd0e65 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51879/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Review Request 52573: Removed some redundant cleanup code in linux filesystem isolator.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Gilbert Song.


Repository: mesos


Description
---

Removed some redundant cleanup code in linux filesystem isolator.


Diffs
-

  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
5b6a1d409293e8d16f6bd78bc9c6f5a524c5e0fe 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Review Request 52574: Parameterized the image volume tests for nested containers.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Gilbert Song.


Repository: mesos


Description
---

Parameterized the image volume tests for nested containers.


Diffs
-

  src/tests/containerizer/volume_image_isolator_tests.cpp PRE-CREATION 
  src/tests/mesos.hpp a05e3e0fad92169dc550f1a1a8b704074716d99e 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 52347: Send entire updated task in `TaskUpdated`.

[Anand Mazumdar]

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



Some high level comments. Would take a detailed look again after another 
iteration.


include/mesos/master/master.proto (lines 464 - 469)


Since the API is experimental, it should be fine to not worry about 
backward compatibility for now. Let's remove these fields except `framework_id` 
that is not present in `TaskStatus`.



include/mesos/master/master.proto (line 470)


s/optional/required?



include/mesos/master/master.proto (line 472)


1. s/optional/required

2. s/Task/TaskStatus

We don't need to send the entire `Task` to the user again. Just 
`TaskStatus` should suffice since it's just an update. Were you using any other 
field from the `Task` message?

3. Can you also add a comment that this `TaskStatus` update is for the last 
update acknowledged by the scheduler?



src/master/master.cpp (line 7484)


nice cleanup!



src/master/master.cpp (line 7536)


Can fit in one line?


- Anand Mazumdar


On Sept. 28, 2016, 1:48 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52347/
> ---
> 
> (Updated Sept. 28, 2016, 1:48 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5936
> https://issues.apache.org/jira/browse/MESOS-5936
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Send entire updated task in `TaskUpdated`.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
>   include/mesos/v1/master/master.proto 
> 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 
>   src/common/protobuf_utils.hpp c5e5a9ad10134bcf87b59460bec7152e4916c75d 
>   src/common/protobuf_utils.cpp 3ff37f498d6c830eb2f4b3d9395f9416cb01f5d3 
>   src/master/master.cpp 66a672f6d16233e96b29e330a9e6c474546fa851 
>   src/tests/api_tests.cpp e857b17cfe5f05d59859263c025564d33700a26c 
> 
> Diff: https://reviews.apache.org/r/52347/diff/
> 
> 
> Testing
> ---
> 
> unit test.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Jiang Yan Xu]

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




include/mesos/resources.hpp (lines 278 - 280)


I still have reservations about this method.

As we discussed, a predicate returning a `bool` is consistent with other 
similar methods and can be used more generally, e.g.,

```
resources.filter([](const Resource& resource) { return 
isMountDisk(resource); });
```

Your main concern (from our conversations) is that it's possible that we 
add more disk types and it would be unwieldy if we have 17 similar calls in the 
form of `bool Resources::isXYZ()`.

My take is that, we currently have only a handful of predefined first class 
resource types (Mesos has special logic for them) and it's not unreasonble to 
have first class support (such as having an `isXYZ()` dedicated for it) for all 
of them. If in the future we have a lot of them, we should still provide first 
class support for **all** of them, but before this list gets too large we 
should probably improve the abstraction to support it in a different, more 
structured way, e.g., a `Disk` class for disks.

---

The problems of adding this method right now include:

## 1. We shouldn't use Try::error when it's not really an error.

Looking at

```
static Try getDiskSource(const Resource& 
resource);
```

I wouldn't know how to deal with the error, is it because the Resource is 
invalid? Is it not a disk? It being a valid disk but without a `Source::Type` 
is not one of the top things spring to mind.

To get the disk type, it may be  (slightly) better to have:

```
static Option getDiskSourceType(const 
Resource& resource);
```

With None meaning "there's no source type". But the problem is that with a 
generic `resource` argument, what if the resource is valid but not a disk? What 
if the resource is invalid?

## 2. The real problem is a flat Resource type with methods that make sense 
only to a special kind of Resource.

Due to the different shapes of pre-defined resources, I find it 
unrealistic/difficult to use/implement methods that handle/expose all 
error/edge cases in one call.

It would have made sense to have

```
Disk::Type Disk::getType();
```

where we know disk is valid. `Disk` can be a subclass of C++ (non-protobuf) 
`Resource` which is guaranteed to be valid.

---

For now, I think we should aim for consistency by sticking to the `isXYZ()` 
methods. It makes using and understanding of the methods easier both for the 
current users and for refactor in the future. (After all I think manging the 
complexity of current use and complexity of change is all that we are doing 
here). In most cases the client has to know if the Resource represents a 
(valid) disk first, they can do that and then use the set of `isXYZ()` to 
determine the type.

---

Finally, we may not even need `isMountDisk()` or `isPathDisk` given how 
/r/51879/ is going. It may make sense to not specially treat PATH disk 
differently but let's disucss that in /r/51879/. With this review we can just 
have a `bool isRootDisk()` first if you like.



src/common/resources.cpp (line 848)


Don't do the CHECK here. Let's follow the `isPersistentVolume` example. 
However I agree we should check the resource name (which I overlooked).

We can just embed it into the return:

```
  return resource.name() == "disk" &&
 (!resource.has_disk() || !resource.disk().has_source());
```

Speaking of `isPersistentVolume()`, it makes sense to add `resource.name() 
== "disk" &&` to it as well.


- Jiang Yan Xu


On Oct. 3, 2016, 4:46 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52002/
> ---
> 
> (Updated Oct. 3, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper methods to determine types of disk resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://

Review Request 52569: Updated the nested container launch to correctly determine the user.

[Benjamin Mahler]

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

Review request for mesos, Gilbert Song and Vinod Kone.


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


Repository: mesos


Description
---

This locates the executor that corresponds to the parent container,
and uses the user that was used to launch the executor.


Diffs
-

  src/slave/http.cpp bdafe1db9f1b22ac0761f5119e73f87b531c6a3c 
  src/slave/slave.hpp 9beefa12283e936684d82c97ebd3ac6506475edf 
  src/slave/slave.cpp fba089506850eec87f67a46694a1ef645cdf4ad5 
  src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 

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


Testing
---

Now that the executor is looked up, the tests have been updated to
launch an executor and use it's container id as the parent.


Thanks,

Benjamin Mahler

Review Request 52568: Added support for nested containers to the test containerizer.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Repository: mesos


Description
---

In the process, the maps that were stored were simplified.


Diffs
-

  src/tests/containerizer.hpp 3cf418a040dfedad2eb6d771e7cc9cc179bcf55c 
  src/tests/containerizer.cpp 89c5400d3a2e771278ba370a939a6c68bc57a31f 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 52567: Added a TODO for cleaning up the `Mesos` library lifetime issues.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Repository: mesos


Description
---

The `Mesos` library may invoke the provided callbacks after `Mesos`
is destructed. This has necessitated callers to bind a shared pointer
to the executor when creating the callbacks. However, this should not
be necessary, and this adds a TODO to describe how to eliminate this
issue.


Diffs
-

  src/tests/mesos.hpp a05e3e0fad92169dc550f1a1a8b704074716d99e 

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


Testing
---

N/A


Thanks,

Benjamin Mahler

Re: Review Request 52516: Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51774, 52515, 52516]

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 Oct. 5, 2016, 3:30 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52516/
> ---
> 
> (Updated Oct. 5, 2016, 3:30 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-6102
> https://issues.apache.org/jira/browse/MESOS-6102
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 
> 
> Diff: https://reviews.apache.org/r/52516/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52162: Fixed Python bindings build error caused by custom SVN location.

[Ilya Pronin]

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

(Updated Oct. 5, 2016, 7:31 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, and Till Toenshoff.


Changes
---

Updated accidently reverted description.


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


Repository: mesos


Description (updated)
---

Moved Python related configuration steps down below all others. Now 
PYTHON_LDFLAGS variable captures all required LDFLAGS.


Diffs
-

  configure.ac 8f971c96bfea0afd7462ea9b91c69458324435b6 

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


Testing
---

```
../configure --with-svn=$HOME/svn && make && make check
```


Thanks,

Ilya Pronin

Re: Review Request 52162: Fixed Python bindings build error caused by custom SVN location.

[Ilya Pronin]

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

(Updated Oct. 5, 2016, 7:29 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, and Till Toenshoff.


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


Repository: mesos


Description (updated)
---

Now PYTHON_LDFLAGS variable captures all required LDFLAGS.


Diffs (updated)
-

  configure.ac 8f971c96bfea0afd7462ea9b91c69458324435b6 

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


Testing
---

```
../configure --with-svn=$HOME/svn && make && make check
```


Thanks,

Ilya Pronin

Re: Review Request 52162: Fixed Python bindings build error caused by custom SVN location.

[Ilya Pronin]

> On Oct. 5, 2016, 5:20 p.m., Till Toenshoff wrote:
> > configure.ac, line 1988
> > 
> >
> > Can we please add one additional comment here; something like:
> > 
> > ```
> > # NOTE: Do not update any compiler or linker settings (e.g. CXXFLAGS,
> > # LDFLAGS, ...) beyond this line.
> > ```
> > 
> > Feel free to change the wording as I am not a native speaker :)

Done. I'm not a native speaker either so I used your wording :)


- Ilya


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


On Oct. 5, 2016, 2:45 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52162/
> ---
> 
> (Updated Oct. 5, 2016, 2:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Kapil Arya, and Till Toenshoff.
> 
> 
> Bugs: MESOS-6207
> https://issues.apache.org/jira/browse/MESOS-6207
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved Python related configuration steps down below all others. Now 
> PYTHON_LDFLAGS variable captures all required LDFLAGS.
> 
> 
> Diffs
> -
> 
>   configure.ac 57482d39db1f83e92e75fca959cd6df329a1c24f 
> 
> Diff: https://reviews.apache.org/r/52162/diff/
> 
> 
> Testing
> ---
> 
> ```
> ../configure --with-svn=$HOME/svn && make && make check
> ```
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 52516: Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.

[Anand Mazumdar]

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



Nice Test!


src/tests/api_tests.cpp (line 1407)


s/that/that a



src/tests/api_tests.cpp (lines 1416 - 1417)


hmm, this does not align with the expectations on L1452 i.e., we check for 
0 frameworks/agents?

Also, we don't seem to be waiting on any offers either. Copy/Paste error? 
:-)



src/tests/api_tests.cpp (lines 1458 - 1459)


Do you need this? Why can't we just wait on the `event` from the decoder.



src/tests/api_tests.cpp (line 1485)


Nit: s/so/so that



src/tests/api_tests.cpp (line 1497)


This needs to change to just compare for `AgentID`.


- Anand Mazumdar


On Oct. 5, 2016, 3:30 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52516/
> ---
> 
> (Updated Oct. 5, 2016, 3:30 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-6102
> https://issues.apache.org/jira/browse/MESOS-6102
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 
> 
> Diff: https://reviews.apache.org/r/52516/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52563: Minor clean ups in `health_check_tests.cpp` for consistency.

[haosdent huang]

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

(Updated Oct. 5, 2016, 6:11 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Rebase.


Repository: mesos


Description
---

* Replace `Testing` to `Tests` in comments.
* Remove redundant `.Times(1)`.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---

```
sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="*HealthCheckTest.*" 
--gtest_also_run_disabled_tests
[--] Global test environment tear-down
[==] 19 tests from 1 test case ran. (35264 ms total)
[  PASSED  ] 19 tests.
```


Thanks,

haosdent huang

Re: Review Request 52560: Avoided temporary `MockDocker` pointers in health check test cases.

[haosdent huang]

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

(Updated Oct. 5, 2016, 6:11 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Rebase.


Repository: mesos


Description
---

Avoided temporary `MockDocker` pointers in health check test cases.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52251: Added test cases for TCP health check.

[haosdent huang]

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

(Updated Oct. 5, 2016, 6:10 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added test cases for TCP health check.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52561: Renamed `flags` to `agentFlags` in health check test cases.

[haosdent huang]

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

(Updated Oct. 5, 2016, 6:11 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Rebase.


Repository: mesos


Description
---

Renamed `flags` to `agentFlags` in health check test cases.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52250: Added test cases for HTTP health check.

[haosdent huang]

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

(Updated Oct. 5, 2016, 6:10 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added test cases for HTTP health check.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52563: Minor clean ups in `health_check_tests.cpp` for consistency.

[haosdent huang]

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

(Updated Oct. 5, 2016, 6:07 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Repository: mesos


Description
---

* Replace `Testing` to `Tests` in comments.
* Remove redundant `.Times(1)`.


Diffs
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing (updated)
---

```
sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="*HealthCheckTest.*" 
--gtest_also_run_disabled_tests
[--] Global test environment tear-down
[==] 19 tests from 1 test case ran. (35264 ms total)
[  PASSED  ] 19 tests.
```


Thanks,

haosdent huang

Re: Review Request 51999: Refactor parsing of resources.

[Jiang Yan Xu]

> On Oct. 5, 2016, 11:06 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 167
> > 
> >
> > "If that fails" gave me the wrong idea of the condition under which the 
> > method would then call `fromSimpleString()` (so I suggested 
> > `fromJSONString(text, role)`).
> > 
> > 
> > We are basically just replying on the input type and not "failures" to 
> > choose the parsing mechanism so would the following be more clear?
> > 
> > 
> > Parses Resources from text in the form of a JSON array or as a simple 
> > string in the form of "name(role):value;name:value;...". i.e, this
> > method calls `fromJSONArray()` or `fromSimpleString()` and validates 
> > the resulting `vector` before converting it to a `Resources` 
> > object.
> > ```
> > 
> > The last part of the paragraph may be implied but I think it makes it 
> > more clear.

s/replying/relying/


- Jiang Yan


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


On Oct. 3, 2016, 4:46 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Oct. 3, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try>`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Overall LGTM!


include/mesos/resources.hpp (line 157)


"If that fails" gave me the wrong idea of the condition under which the 
method would then call `fromSimpleString()` (so I suggested 
`fromJSONString(text, role)`).

We are basically just replying on the input type and not "failures" to 
choose the parsing mechanism so would the following be more clear?


Parses Resources from text in the form of a JSON array or as a simple 
string in the form of "name(role):value;name:value;...". i.e, this
method calls `fromJSONArray()` or `fromSimpleString()` and validates the 
resulting `vector` before converting it to a `Resources` object.
```

The last part of the paragraph may be implied but I think it makes it more 
clear.



include/mesos/resources.hpp (line 158)


s/i.e,/i.e.,/



include/mesos/resources.hpp (line 159)


s/fromJSONArray/fromJSON/ 

See the comment at the method declaration below.



include/mesos/resources.hpp (line 180)


Should we add a usage note?

```
NOTE: The `Resource` objects in the result vector may not be semantically 
valid (i.e., they may not pass `Resources::validate()`). This is to allow 
additional handling of the parsing results in certain cases.
```



include/mesos/resources.hpp (line 186)


s/fromJSONArray/fromJSON/

There should be no ambiguity and we gained brevity. 

The info about the argument being `JSON::Array` is already baked into the 
method signture so we don't have to say it again.



include/mesos/resources.hpp (line 197)


Should we add a usage note?

```
NOTE: The `Resource` objects in the result vector may not be semantically 
valid (i.e., they may not pass `Resources::validate()`). This is to allow 
additional handling of the parsing results in certain cases.
```



src/common/resources.cpp (lines 532 - 550)


Would the following be simpler?

```
Try> _result = resourcesJSON.isSome()
  ? fromJSONArray(resourcesJSON.get(), defaultRole),
  : fromSimpleString(text, defaultRole);

if (_result.isError()) {
  return Error(_result.error());
}

resources = _result.get();
```

Also s/resourcesJSON/json/? (because it literally just checks whether it's 
valid JSON and it
is shorter)



src/common/resources.cpp (line 598)


```
// However, those checks are done at the respective call sites. 
```

I suggested above that we move this over to be a usage note for this 
method. If we do that then here the implementation really doesn't care about it 
one way or the other (as a lower level parsing function) so we can kill it.



src/common/resources.cpp (lines 646 - 647)


Ditto.


- Jiang Yan Xu


On Oct. 3, 2016, 4:46 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Oct. 3, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try>`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 

Review Request 52563: Minor clean ups in `health_check_tests.cpp` for consistency.

[haosdent huang]

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

Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Repository: mesos


Description
---

* Replace `Testing` to `Tests` in comments.
* Remove redundant `.Times(1)`.


Diffs
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 50910: Added a python linter to mesos-style.cpp.

[Kevin Klues]

> On Oct. 5, 2016, 12:18 a.m., Joseph Wu wrote:
> > support/mesos-style.py, lines 270-272
> > 
> >
> > It may help to change the order of reviews (no rebase necessary, just 
> > the "depends on" field).  
> > 
> > Because you add a linter in this review, that depends on the next one ( 
> > https://reviews.apache.org/r/50912/ ), this review fails to lint itself :)
> 
> Kevin Klues wrote:
> I'm confused. The order of these patches (and their dependencies) should 
> be correct. This patch "depends on" 50907, not 50912. Also, when I 
> cherry-pick each of these patches onto master individually, they all get 
> linted just as expected.
> 
> Kevin Klues wrote:
> For example, from master:
> ```
> klueska@c99:~/projects/mesos$ support/apply-reviews.py -r 50907
> 2016-10-04 20:31:30 URL:https://reviews.apache.org/r/50907/diff/raw/ 
> [16055/16055] -> "50907.patch" [1]
> No C++ files to lint
> [cli-test d536361] Abstracted mesos-style.py to wrap the cpp linter in a 
> class.
>  1 file changed, 249 insertions(+), 169 deletions(-)
>  rewrite support/mesos-style.py (95%)
> klueska@c99:~/projects/mesos$ support/apply-reviews.py -r 50910
> 2016-10-04 20:31:39 URL:https://reviews.apache.org/r/50910/diff/raw/ 
> [2997/2997] -> "50910.patch" [1]
> No C++ files to lint
> No Python files to lint
> [cli-test 2b9502e] Added a python linter to mesos-style.cpp.
>  Author: Haris Choudhary 
>  1 file changed, 67 insertions(+), 2 deletions(-)
> klueska@c99:~/projects/mesos$ support/apply-reviews.py -r 50912
> 2016-10-04 20:31:51 URL:https://reviews.apache.org/r/50912/diff/raw/ 
> [40930/40930] -> "50912.patch" [1]
> No C++ files to lint
> Checking 8 Python files
> Total errors found: 0
> [cli-test 88e5726] Added the infrastructure for a new python-based CLI.
>  20 files changed, 1019 insertions(+), 3 deletions(-)
>  create mode 100644 src/cli_new/.gitignore
>  create mode 100644 src/cli_new/README.md
>  create mode 100644 src/cli_new/activate
>  create mode 100644 src/cli_new/bin/config.py
>  create mode 100644 src/cli_new/bin/main.py
>  create mode 100755 src/cli_new/bin/mesos
>  create mode 100755 src/cli_new/bootstrap
>  create mode 100644 src/cli_new/deactivate
>  create mode 100644 src/cli_new/lib/mesos/__init__.py
>  create mode 100644 src/cli_new/lib/mesos/docopt.py
>  create mode 100644 src/cli_new/lib/mesos/exceptions.py
>  create mode 100644 src/cli_new/lib/mesos/plugins/__init__.py
>  create mode 100644 src/cli_new/lib/mesos/plugins/base.py
>  create mode 100644 src/cli_new/lib/mesos/util.py
>  create mode 100644 src/cli_new/mesos.bash_completion
>  create mode 100644 src/cli_new/pip-requirements.txt
>  create mode 100644 src/cli_new/pylint.config
> ```
> 
> Joseph Wu wrote:
> Presumably, that's because your source directory contains leftover 
> directories/files (like `*.pyc` files, which prevent git from deleting the 
> `cli_new` folder):
> ```
> $ support/apply-reviews.py -n -r 50910 -c
> 2016-10-05 10:49:31 URL:https://reviews.apache.org/r/50907/diff/raw/ 
> [16055/16055] -> "50907.patch" [1]
> No C++ files to lint
> [detached HEAD faed65f] Abstracted mesos-style.py to wrap the cpp linter 
> in a class.
>  Author: Kevin Klues 
>  1 file changed, 249 insertions(+), 169 deletions(-)
>  rewrite support/mesos-style.py (95%)
> 2016-10-05 10:49:32 URL:https://reviews.apache.org/r/50910/diff/raw/ 
> [2997/2997] -> "50910.patch" [1]
> No C++ files to lint
> Could not find 'src/cli_new'
> Please run from the root of the mesos source directory
> ```
> 
> If I do:
> ```
> $ support/apply-reviews.py -n -r 50907
> $ support/apply-reviews.py -n -r 50912 # <- reordered this one.
> $ support/apply-reviews.py -n -r 50910
> ```
> 
> It applies cleanly.

I see. I think we should then add the linter in this commit (but with an emty 
array of folders to check). And then introduce this folder into the array in 
the next commit.  Otherwise we miss out on linting the entire base patch for 
the CLI.


- Kevin


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


On Aug. 11, 2016, 9:54 p.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50910/
> ---
> 
> (Updated Aug. 11, 2016, 9:54 p.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: Mesos-6006
> https://issues.apache.org/jira/browse/Mesos-6006
> 
> 
> Repos

Re: Review Request 50910: Added a python linter to mesos-style.cpp.

[Joseph Wu]

> On Oct. 4, 2016, 5:18 p.m., Joseph Wu wrote:
> > support/mesos-style.py, lines 270-272
> > 
> >
> > It may help to change the order of reviews (no rebase necessary, just 
> > the "depends on" field).  
> > 
> > Because you add a linter in this review, that depends on the next one ( 
> > https://reviews.apache.org/r/50912/ ), this review fails to lint itself :)
> 
> Kevin Klues wrote:
> I'm confused. The order of these patches (and their dependencies) should 
> be correct. This patch "depends on" 50907, not 50912. Also, when I 
> cherry-pick each of these patches onto master individually, they all get 
> linted just as expected.
> 
> Kevin Klues wrote:
> For example, from master:
> ```
> klueska@c99:~/projects/mesos$ support/apply-reviews.py -r 50907
> 2016-10-04 20:31:30 URL:https://reviews.apache.org/r/50907/diff/raw/ 
> [16055/16055] -> "50907.patch" [1]
> No C++ files to lint
> [cli-test d536361] Abstracted mesos-style.py to wrap the cpp linter in a 
> class.
>  1 file changed, 249 insertions(+), 169 deletions(-)
>  rewrite support/mesos-style.py (95%)
> klueska@c99:~/projects/mesos$ support/apply-reviews.py -r 50910
> 2016-10-04 20:31:39 URL:https://reviews.apache.org/r/50910/diff/raw/ 
> [2997/2997] -> "50910.patch" [1]
> No C++ files to lint
> No Python files to lint
> [cli-test 2b9502e] Added a python linter to mesos-style.cpp.
>  Author: Haris Choudhary 
>  1 file changed, 67 insertions(+), 2 deletions(-)
> klueska@c99:~/projects/mesos$ support/apply-reviews.py -r 50912
> 2016-10-04 20:31:51 URL:https://reviews.apache.org/r/50912/diff/raw/ 
> [40930/40930] -> "50912.patch" [1]
> No C++ files to lint
> Checking 8 Python files
> Total errors found: 0
> [cli-test 88e5726] Added the infrastructure for a new python-based CLI.
>  20 files changed, 1019 insertions(+), 3 deletions(-)
>  create mode 100644 src/cli_new/.gitignore
>  create mode 100644 src/cli_new/README.md
>  create mode 100644 src/cli_new/activate
>  create mode 100644 src/cli_new/bin/config.py
>  create mode 100644 src/cli_new/bin/main.py
>  create mode 100755 src/cli_new/bin/mesos
>  create mode 100755 src/cli_new/bootstrap
>  create mode 100644 src/cli_new/deactivate
>  create mode 100644 src/cli_new/lib/mesos/__init__.py
>  create mode 100644 src/cli_new/lib/mesos/docopt.py
>  create mode 100644 src/cli_new/lib/mesos/exceptions.py
>  create mode 100644 src/cli_new/lib/mesos/plugins/__init__.py
>  create mode 100644 src/cli_new/lib/mesos/plugins/base.py
>  create mode 100644 src/cli_new/lib/mesos/util.py
>  create mode 100644 src/cli_new/mesos.bash_completion
>  create mode 100644 src/cli_new/pip-requirements.txt
>  create mode 100644 src/cli_new/pylint.config
> ```

Presumably, that's because your source directory contains leftover 
directories/files (like `*.pyc` files, which prevent git from deleting the 
`cli_new` folder):
```
$ support/apply-reviews.py -n -r 50910 -c
2016-10-05 10:49:31 URL:https://reviews.apache.org/r/50907/diff/raw/ 
[16055/16055] -> "50907.patch" [1]
No C++ files to lint
[detached HEAD faed65f] Abstracted mesos-style.py to wrap the cpp linter in a 
class.
 Author: Kevin Klues 
 1 file changed, 249 insertions(+), 169 deletions(-)
 rewrite support/mesos-style.py (95%)
2016-10-05 10:49:32 URL:https://reviews.apache.org/r/50910/diff/raw/ 
[2997/2997] -> "50910.patch" [1]
No C++ files to lint
Could not find 'src/cli_new'
Please run from the root of the mesos source directory
```

If I do:
```
$ support/apply-reviews.py -n -r 50907
$ support/apply-reviews.py -n -r 50912 # <- reordered this one.
$ support/apply-reviews.py -n -r 50910
```

It applies cleanly.


- Joseph


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


On Aug. 11, 2016, 2:54 p.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50910/
> ---
> 
> (Updated Aug. 11, 2016, 2:54 p.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: Mesos-6006
> https://issues.apache.org/jira/browse/Mesos-6006
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It currently doesn't run over any files in the code base, but we will
> be adding the new python CLI in a subsequent commit, which will use
> this new linter.
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py 061cbe711bd9407b7341fa146f8c723eaa9fc438 
> 
> Diff: https://reviews.apache.org/r/50910/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Haris Choudhar

Re: Review Request 52252: Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaHTTP`.

[haosdent huang]

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

(Updated Oct. 5, 2016, 5:46 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


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


Repository: mesos


Description
---

Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaHTTP`.


Diffs
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

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


Testing
---


Thanks,

haosdent huang

Review Request 52561: Renamed `flags` to `agentFlags` in health check test cases.

[haosdent huang]

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

Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Repository: mesos


Description
---

Renamed `flags` to `agentFlags` in health check test cases.


Diffs
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---


Thanks,

haosdent huang

Review Request 52560: Avoided temporary `MockDocker` pointers in health check test cases.

[haosdent huang]

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

Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Repository: mesos


Description
---

Avoided temporary `MockDocker` pointers in health check test cases.


Diffs
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52252: Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaHTTP`.

[haosdent huang]

> On Oct. 5, 2016, 4:53 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, line 1547
> > 
> >
> > Could you please rename all relevant s/flags/agentFlags in this file? 
> > As a separate patch, please!

Posted at https://reviews.apache.org/r/52561/


- haosdent


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


On Oct. 5, 2016, 4:10 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52252/
> ---
> 
> (Updated Oct. 5, 2016, 4:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6278
> https://issues.apache.org/jira/browse/MESOS-6278
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaHTTP`.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52252/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52252: Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaHTTP`.

[haosdent huang]

> On Oct. 5, 2016, 4:53 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, lines 1540-1541
> > 
> >
> > Let's do it as a separate patch in the whole file at once.

Posted at https://reviews.apache.org/r/52560/


- haosdent


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


On Oct. 5, 2016, 4:10 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52252/
> ---
> 
> (Updated Oct. 5, 2016, 4:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6278
> https://issues.apache.org/jira/browse/MESOS-6278
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaHTTP`.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52252/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Oct. 5, 2016, 5:44 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
ad2070246b7fdb90aa6ed02326b5a7eb95365997 

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


Testing
---

make check on linux and macos


Thanks,

Megha Sharma

Re: Review Request 52515: Implement `AGENT_ADDED` and `AGENT_REMOVED` events.

[Anand Mazumdar]

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




src/common/protobuf_utils.hpp (line 188)


2 empty lines please.



src/common/protobuf_utils.hpp (line 192)


Ditto



src/common/protobuf_utils.cpp (line 498)


hmm, this is a bit inconsistent. We have been using `model()` functions 
historically to convert our internal protos into `JSON` based responses. I 
guess you did it like this since the logic was previously common to both the 
helper functions that you had added?

How do you feel about adding a helper function `createAgentResponse()` 
instead in a separate review and make `_getAgents()` invoke it? This would 
avoid duplicating this code at both the places. Then in this review you can 
make `createAgentAdded()` invoke it?



src/common/protobuf_utils.cpp (line 547)


s/Slave/SlaveID?

Looks like you are using `AgentInfo` in the `AGENT_REMOVED` event. We need 
to *change* it to `AgentID` as per my review comment in the previous review.


- Anand Mazumdar


On Oct. 5, 2016, 3:30 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52515/
> ---
> 
> (Updated Oct. 5, 2016, 3:30 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-6102
> https://issues.apache.org/jira/browse/MESOS-6102
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement `AGENT_ADDED` and `AGENT_REMOVED` events.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 0c6405f6cfc5a69fa835428fa330d8d63778f469 
>   src/common/protobuf_utils.cpp 1f088684c566f89c85dd72eaece295c5e3d50b23 
>   src/master/master.cpp 756ab546851952bc1de24b1f469d232237b1c01c 
> 
> Diff: https://reviews.apache.org/r/52515/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Oct. 5, 2016, 5:40 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
ad2070246b7fdb90aa6ed02326b5a7eb95365997 

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


Testing
---

make check on linux and macos


Thanks,

Megha Sharma

Re: Review Request 52251: Added test cases for TCP health check.

[haosdent huang]

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

(Updated Oct. 5, 2016, 5:28 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Address @alexr's comments.


Summary (updated)
-

Added test cases for TCP health check.


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


Repository: mesos


Description (updated)
---

Added test cases for TCP health check.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52250: Added test cases for HTTP health check.

[haosdent huang]

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

(Updated Oct. 5, 2016, 5:27 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Rebase.


Summary (updated)
-

Added test cases for HTTP health check.


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


Repository: mesos


Description (updated)
---

Added test cases for HTTP health check.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51999: Refactor parsing of resources.

[Jiang Yan Xu]

> On Sept. 27, 2016, 10:28 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 199
> > 
> >
> > s/JSON::Array& resourcesJSON/std::string text/
> > 
> > Looking the current and potential call-sites, it doesn't look like we 
> > ever directly call this method with a `JSON::Array` not parsed from text.
> > 
> > Considering this, I think it would be more consistent to have the 
> > following three methods which take the same argument list.
> > 
> > ```
> > static Try parse(
> > const std::string& text,
> > const std::string& defaultRole = "*");
> > 
> > static Try> fromJSONString(
> > const std::string& text,
> > const std::string& defaultRole = "*");
> > 
> > static Try> fromSimpleString(
> > const std::string& text,
> > const std::string& defaultRole = "*")
> > ```
> 
> Anindya Sinha wrote:
> For JSON handling, we do the following in sequence:
> (1) `JSON::parse()` converts input string to JSON array. 
> (2) `protobuf::parse>(resourcesJSON)` converts 
> the JSON array to protobuf.
> 
> If we call both of these in `fromJSONString()`, then in 
> `Resources::parse()`, we would not be able to deduce which of the above 2 
> functions failed. This is needed since if it failed in (1), it means we parse 
> assuming text; whereas if it failed in (2), that is an error since protobuf 
> conversion failed for the JSON (we should not assume it is text).
> 
> So, I think this should work:
> In `Resources::parse()`:
> - Convert to JSON array (`JSON::parse()`).
> - If `JSON::parse()` succeeds, process JSON array in 
> `fromJSONString()`. Any failure here is an error condition.
> - If `JSON::parse()` fails, assume it is text and process in 
> `fromSimpleString()`.

OK, makes sense. Some of my other comments were based on the assumption of this 
method taking a string. I see that Guangya has already made comments about 
renaming it to `fromJSONArray()`. I'll add some additional comments.


> On Sept. 27, 2016, 10:28 a.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, lines 630-657
> > 
> >
> > With out refactor, the result of `Resources::parse()` doesn't change 
> > right?
> 
> Jiang Yan Xu wrote:
> s/out/our/
> 
> Anindya Sinha wrote:
> Im original code: If a resource in text format is encountered with value 
> of 0, it is dropped; but in JSON, it flags an error. This patch makes the 
> behavior same, ie. just drop if a `Resource` has a value of 0. Hence, this 
> test would fail for value of 0. So I modified this test to test for negative 
> values which is obviously an Error.

OK, I don't think parse should return Error for empty values either.


- Jiang Yan


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


On Oct. 3, 2016, 4:46 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Oct. 3, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try>`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52252: Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaHTTP`.

[Alexander Rukletsov]

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




src/tests/health_check_tests.cpp (lines 1540 - 1541)


Let's do it as a separate patch in the whole file at once.



src/tests/health_check_tests.cpp (lines 1543 - 1545)


Either put allocation interval back or remove the flags ; )



src/tests/health_check_tests.cpp (line 1547)


Could you please rename all relevant s/flags/agentFlags in this file? As a 
separate patch, please!


- Alexander Rukletsov


On Oct. 5, 2016, 4:10 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52252/
> ---
> 
> (Updated Oct. 5, 2016, 4:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6278
> https://issues.apache.org/jira/browse/MESOS-6278
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaHTTP`.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52252/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52253: Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaTCP`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52250, 52251, 52252, 52253]

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 Oct. 5, 2016, 4:10 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52253/
> ---
> 
> (Updated Oct. 5, 2016, 4:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6279
> https://issues.apache.org/jira/browse/MESOS-6279
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaTCP`.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52253/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52541: Renamed the filesystem isolator tests file name.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Oct. 4, 2016, 6:16 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52541/
> ---
> 
> (Updated Oct. 4, 2016, 6:16 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed the filesystem isolator tests file name.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 5b6a1d409293e8d16f6bd78bc9c6f5a524c5e0fe 
> 
> Diff: https://reviews.apache.org/r/52541/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52434: Improved handling of tmp file creation in health check test.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Oct. 4, 2016, 4:14 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52434/
> ---
> 
> (Updated Oct. 4, 2016, 4:14 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The tmp file used in the HealthStatusChange test is now created inside
> the tmp directory created for the test suite. This ensures that it will
> be cleaned up on tear down.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52434/diff/
> 
> 
> Testing
> ---
> 
> make check GTEST_FILTER="HealthCheckTest.*" V=0
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52433: Improved the naming of variables in HealthCheckTest.HealthStatusChange.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Oct. 5, 2016, 2:37 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52433/
> ---
> 
> (Updated Oct. 5, 2016, 2:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved the naming of variables in HealthCheckTest.HealthStatusChange.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52433/diff/
> 
> 
> Testing
> ---
> 
> make check GTEST_FILTER="HealthCheckTest.*" V=0
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52432: Updated and reenabled 'HealthCheckTest.GracePeriod'.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Oct. 5, 2016, 2:10 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52432/
> ---
> 
> (Updated Oct. 5, 2016, 2:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-1653
> https://issues.apache.org/jira/browse/MESOS-1653
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test was disabled in 2014 because it was considered flaky. I
> updated it and ran it 1000 times without a single failure.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52432/diff/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --verbose --gtest_filter="HealthCheckTest.GracePeriod" 
> --break-on-error --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52162: Fixed Python bindings build error caused by custom SVN location.

[Till Toenshoff]

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


Fix it, then Ship it!




Thanks Ilya. Would love to see one little addition to make sure this issues is 
less likely to occur again - see below.

Really confusing how reviewboard displays your patch. So for other reviewers: 
this really just moves lines 1472-1789 to 1845 - effectively making sure that 
all mutations towards the `LDFLAGS` (and others) are indeed captured by the 
disttools invocations needed for those eggs.


configure.ac (line 1845)


Can we please add one additional comment here; something like:

```
# NOTE: Do not update any compiler or linker settings (e.g. CXXFLAGS,
# LDFLAGS, ...) beyond this line.
```

Feel free to change the wording as I am not a native speaker :)


- Till Toenshoff


On Oct. 5, 2016, 1:45 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52162/
> ---
> 
> (Updated Oct. 5, 2016, 1:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Kapil Arya, and Till Toenshoff.
> 
> 
> Bugs: MESOS-6207
> https://issues.apache.org/jira/browse/MESOS-6207
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved Python related configuration steps down below all others. Now 
> PYTHON_LDFLAGS variable captures all required LDFLAGS.
> 
> 
> Diffs
> -
> 
>   configure.ac 57482d39db1f83e92e75fca959cd6df329a1c24f 
> 
> Diff: https://reviews.apache.org/r/52162/diff/
> 
> 
> Testing
> ---
> 
> ```
> ../configure --with-svn=$HOME/svn && make && make check
> ```
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Review Request 52558: Added test case `ROOT_HealthyTaskViaTCPWithContainerImage`.

[haosdent huang]

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

Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


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


Repository: mesos


Description
---

Added test case `ROOT_HealthyTaskViaTCPWithContainerImage`.


Diffs
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

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


Testing
---


Thanks,

haosdent huang

Review Request 52557: Added test case `ROOT_HealthyTaskViaHTTPWithContainerImage`.

[haosdent huang]

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

Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


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


Repository: mesos


Description
---

Added test case `ROOT_HealthyTaskViaHTTPWithContainerImage`.


Diffs
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52253: Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaTCP`.

[haosdent huang]

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

(Updated Oct. 5, 2016, 4:10 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaTCP`.


Diffs (updated)
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52252: Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaHTTP`.

[haosdent huang]

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

(Updated Oct. 5, 2016, 4:10 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaHTTP`.


Diffs (updated)
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52251: Added test case `HealthCheckTest.HealthyTaskViaTCP`.

[haosdent huang]

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

(Updated Oct. 5, 2016, 4:09 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added test case `HealthCheckTest.HealthyTaskViaTCP`.


Diffs (updated)
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52250: Added test case `HealthCheckTest.HealthyTaskViaHTTP`.

[haosdent huang]

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

(Updated Oct. 5, 2016, 4:09 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added test case `HealthCheckTest.HealthyTaskViaHTTP`.


Diffs (updated)
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52516: Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.

[Zhitao Li]

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

(Updated Oct. 5, 2016, 3:30 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


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


Repository: mesos


Description
---

Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.


Diffs (updated)
-

  src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 52515: Implement `AGENT_ADDED` and `AGENT_REMOVED` events.

[Zhitao Li]

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

(Updated Oct. 5, 2016, 3:30 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


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


Repository: mesos


Description
---

Implement `AGENT_ADDED` and `AGENT_REMOVED` events.


Diffs (updated)
-

  src/common/protobuf_utils.hpp 0c6405f6cfc5a69fa835428fa330d8d63778f469 
  src/common/protobuf_utils.cpp 1f088684c566f89c85dd72eaece295c5e3d50b23 
  src/master/master.cpp 756ab546851952bc1de24b1f469d232237b1c01c 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 51774: Add `AGENT_ADDED` and `AGENT_REMOVED` master events.

[Zhitao Li]

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

(Updated Oct. 5, 2016, 3:30 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


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


Repository: mesos


Description
---

Add `AGENT_ADDED` and `AGENT_REMOVED` master events.


Diffs (updated)
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 

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


Testing
---

New unit test.


Thanks,

Zhitao Li

Re: Review Request 52253: Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaTCP`.

[haosdent huang]

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

(Updated Oct. 5, 2016, 2:39 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaTCP`.


Diffs (updated)
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52252: Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaHTTP`.

[haosdent huang]

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

(Updated Oct. 5, 2016, 2:39 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added test case `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaHTTP`.


Diffs (updated)
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52433: Improved the naming of variables in HealthCheckTest.HealthStatusChange.

[Gastón Kleiman]

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

(Updated Oct. 5, 2016, 2:37 p.m.)


Review request for mesos, Alexander Rukletsov and haosdent huang.


Changes
---

Addressed comments.


Repository: mesos


Description
---

Improved the naming of variables in HealthCheckTest.HealthStatusChange.


Diffs (updated)
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

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


Testing
---

make check GTEST_FILTER="HealthCheckTest.*" V=0


Thanks,

Gastón Kleiman