Re: Review Request 65541: WIP: Experimented with the MessageDifferencer.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['65540', '65541']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
   To stop - type Control-C.
-a Resolve addresses to hostnames.
-n count   Number of echo requests to send.
-l sizeSend buffer size.
-f Set Don't Fragment flag in packet (IPv4-only).
-i TTL Time To Live.
-v TOS Type Of Service (IPv4-only. This setting has been deprecated
   and has no effect on the type of service field in the IP
   Header).
-r count   Record route for count hops (IPv4-only).
-s count   Timestamp for count hops (IPv4-only).
-j host-list   Loose source route along host-list (IPv4-only).
-k host-list   Strict source route along host-list (IPv4-only).
-w timeout Timeout in milliseconds to wait for each reply.
-R Use routing header to test reverse route also (IPv6-only).
   Per RFC 5095 the use of this routing header has been
   deprecated. Some systems may drop echo requests if
   this header is used.
-S srcaddr Source address to use.
-c compartment Routing compartment identifier.
-p Ping a Hyper-V Network Virtualization provider address.
-4 Force using IPv4.
-6 Force using IPv6.

Note: Google Test filter = 
*-FetcherTest.UNZIP_ExtractFile:FetcherTest.UNZIP_ExtractInvalidFile:FetcherTest.UNZIP_ExtractFileWithDuplicatedEntries:HealthCheckTest.DISABLED_ROOT_DOCKER_DockerHealthyTask:HealthCheckTest.DISABLED_ROOT_DOCKER_DockerHealthStatusChange:HealthCheckTest.ROOT_INTERNET_CURL_HealthyTaskViaHTTPWithContainerImage:HealthCheckTest.ROOT_INTERNET_CURL_HealthyTaskViaHTTPSWithContainerImage:HealthCheckTest.ROOT_INTERNET_CURL_HealthyTaskViaTCPWithContainerImage:HookTest.DISABLED_ROOT_DOCKER_VerifySlavePreLaunchDockerTaskExecutorDecorator:HookTest.DISABLED_ROOT_DOCKER_VerifySlavePreLaunchDockerValidator:HookTest.DISABLED_ROOT_DOCKER_VerifySlavePreLaunchDockerHook:HookTest.DISABLED_ROOT_DOCKER_VerifySlavePostFetchHook:Resources_Filter_BENCHMARK_Test.Filters:DockerFetcherPluginTest.INTERNET_CURL_FetchManifest:DockerFetcherPluginTest.INTERNET_CURL_FetchBlob:DockerFetcherPluginTest.INTERNET_CURL_FetchImage:DockerFetcherPluginTest.INTERNET_CURL_InvokeFetchByName:DockerTest.DI
 
SABLED_ROOT_DOCKER_interface:DockerTest.DISABLED_ROOT_DOCKER_kill:DockerTest.ROOT_DOCKER_Version:DockerTest.DISABLED_ROOT_DOCKER_CheckCommandWithShell:DockerTest.DISABLED_ROOT_DOCKER_CheckPortResource:DockerTest.DISABLED_ROOT_DOCKER_CancelPull:DockerTest.DISABLED_ROOT_DOCKER_MountRelativeHostPath:DockerTest.DISABLED_ROOT_DOCKER_MountAbsoluteHostPath:DockerTest.DISABLED_ROOT_DOCKER_MountRelativeContainerPath:DockerTest.DISABLED_ROOT_DOCKER_MountRelativeHostPathRelativeContainerPath:DockerTest.DISABLED_ROOT_DOCKER_NVIDIA_GPU_DeviceAllow:DockerTest.DISABLED_ROOT_DOCKER_NVIDIA_GPU_InspectDevices:DockerTest.DISABLED_ROOT_DOCKER_ConflictingVolumeDriversInMultipleVolumes:DockerTest.DISABLED_ROOT_DOCKER_ConflictingVolumeDrivers:ParentChildContainerTypeAndContentType/AgentContainerAPITest.DISABLED_NestedContainerLaunch/1:ParentChildContainerTypeAndContentType/AgentContainerAPITest.DISABLED_NestedContainerLaunch/3:ParentChildContainerTypeAndContentType/AgentContainerAPITest.DISABLED_NestedCon
 
tainerLaunch/5:ParentChildContainerTypeAndContentType/AgentContainerAPITest.DISABLED_NestedContainerLaunch/7:ParentChildContainerTypeAndContentType/AgentContainerAPITest.DISABLED_NestedContainerLaunch/9:ParentChildContainerTypeAndContentType/AgentContainerAPITest.DISABLED_NestedContainerLaunch/11:ParentChildContainerTypeAndContentType/AgentContainerAPITest.DISABLED_NestedContainerLaunch/13:ParentChildContainerTypeAndContentType/AgentContainerAPITest.DISABLED_NestedContainerLaunch/15:ParentChildContainerTypeAndContentType/AgentContainerAPITest.DISABLED_RecoverNestedContainer/1:ParentChildContainerTypeAndContentType/AgentContainerAPITest.DISABLED_RecoverNestedContainer/3:ParentChildContainerTypeAndContentType/AgentContainerAPITest.DISABLED_RecoverNestedContainer/5:ParentChildContainerTypeAndContentType/AgentContainerAPITest.DISABLED_RecoverNestedContainer/7:ParentChildContainerTypeAndContentType/AgentContainerAPITest.DISABLED_RecoverNestedContainer/9:ParentChildContainerTypeAndContent
 

Review Request 65541: WIP: Experimented with the MessageDifferencer.

[Joseph Wu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This is an attempt to use the MessageDifferencer class (introduced in 
protobuf 3.0) to replace our handwritten protobuf equality operators.

This patch currently:
  * Ignores style rules (mostly).
  * Has TODOs to deal with the Resources and Attributes classes,
which have their own wrappers and may need to be special-cased.
  * Only deals with non-versioned protobufs at the moment.
  * Can be made faster by making the MessageDifferencer into a
`static thread_local` object guarded by a Once.


Diffs
-

  include/mesos/type_utils.hpp af2b187b9b59552e4ba515ad640fd4419eaf5075 
  src/common/type_utils.cpp a4d5dcb4e4445e307356d9b0c16dd39f00f6a8e2 


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


Testing
---


Thanks,

Joseph Wu

Review Request 65540: WIP: Removed most equality operators.

[Joseph Wu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This is a cleanup patch so that the next patch's diff looks cleaner.

Only deals with non-versioned protobufs at the moment.


Diffs
-

  include/mesos/type_utils.hpp af2b187b9b59552e4ba515ad640fd4419eaf5075 
  src/common/type_utils.cpp a4d5dcb4e4445e307356d9b0c16dd39f00f6a8e2 


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


Testing
---

make check


Thanks,

Joseph Wu

Re: Review Request 65538: WIP: Added provider assignments into `disk_profile.proto`.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['65032', '65471', '65472', '65060', '65499', '65520', 
'65534', '65523', '65521', '65061', '65522', '65538']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
 D:\DCOS\mesos\mesos\src\java\jni\convert.cpp(415): warning C4267: 
'argument': conversion from 'size_t' to 'jsize', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\java\jni\convert.cpp(437): warning C4267: 
'argument': conversion from 'size_t' to 'jsize', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\java\jni\convert.cpp(438): warning C4267: 
'argument': conversion from 'size_t' to 'jsize', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\java\jni\convert.cpp(460): warning C4267: 
'argument': conversion from 'size_t' to 'jsize', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\java\jni\convert.cpp(461): warning C4267: 
'argument': conversion from 'size_t' to 'jsize', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\java\jni\convert.cpp(500): warning C4267: 
'argument': conversion from 'size_t' to 'jsize', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\java\jni\convert.cpp(501): warning C4267: 
'argument': conversion from 'size_t' to 'jsize', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_state_Variable.cpp(44): 
warning C4267: 'argument': conversion from 'size_t' to 'jsize', possible loss 
of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_state_Variable.cpp(45): 
warning C4267: 'argument': conversion from 'size_t' to 'jsize', possible loss 
of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(95): warning C4267: '=': 
conversion from 'size_t' to 'jint', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(463): warning C4244: 
'initializing': conversion from 'jchar' to 'char', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(493): warning C4244: 
'initializing': conversion from 'jlong' to 'long', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(654): warning C4244: 
'initializing': conversion from 'jchar' to 'char', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(693): warning C4244: 
'initializing': conversion from 'jlong' to 'long', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(770): warning C4244: 
'initializing': conversion from 'jchar' to 'char', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(809): warning C4244: 
'initializing': conversion from 'jlong' to 'long', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]


   "D:\DCOS\mesos\src\tests\mesos-tests.vcxproj" (default target) (1) ->
   (Link target) -> 
 cluster.obj : error LNK2019: unresolved external symbol "class 
Try,class std::allocator >,class std::allocator,class std::allocator 
> > >,class Error> __cdecl mesos::csi::paths::getContainerPaths(class 
std::basic_string const &,class std::basic_string const &,class std::basic_string const &)" 
(?getContainerPaths@paths@csi@mesos@@YA?AV?$Try@V?$list@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V?$allocator@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@2@@std@@VErrorAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@00@Z)
 referenced in function "public: void __cdecl ::operator()(void)const " 
(??R@@QEBAXXZ) 
[D:\DCOS\mesos\src\tests\mesos-tests.vcxproj]
 cluster.obj : error LNK2019: unresolved external symbol "class 
Try __cdecl 
mesos::csi::paths::parseContainerPath(class std::basic_string const 

Re: Review Request 65538: WIP: Added provider assignments into `disk_profile.proto`.

[James DeFelice]

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




src/resource_provider/storage/disk_profile.proto
Lines 39 (patched)


maybe add a comment that specifying a `selector` is REQUIRED?


- James DeFelice


On Feb. 7, 2018, 1:12 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65538/
> ---
> 
> (Updated Feb. 7, 2018, 1:12 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8510
> https://issues.apache.org/jira/browse/MESOS-8510
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A new `assignments` field is added to the `DiskProfileMapping` protobuf
> so that the URI disk profile adaptor can be customized to notify each
> resource provider a different set of profiles.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/disk_profile.proto 
> 6cf1f8abcd24e45a292efc95a395f90bb2140da2 
> 
> 
> Diff: https://reviews.apache.org/r/65538/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65538: WIP: Added provider assignments into `disk_profile.proto`.

[Chun-Hung Hsiao]

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

(Updated Feb. 7, 2018, 1:12 a.m.)


Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.


Changes
---

Addressed James' comment.


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


Repository: mesos


Description
---

A new `assignments` field is added to the `DiskProfileMapping` protobuf
so that the URI disk profile adaptor can be customized to notify each
resource provider a different set of profiles.


Diffs (updated)
-

  src/resource_provider/storage/disk_profile.proto 
6cf1f8abcd24e45a292efc95a395f90bb2140da2 


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

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


Testing
---


Thanks,

Chun-Hung Hsiao

Re: Review Request 65448: Added a test to ensure master removes executors that never launched.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65448 was successfully built and tested.

Reviews applied: `['65445', '65504', '65446', '65449', '65448']`

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

- Mesos Reviewbot Windows


On Feb. 1, 2018, 2:04 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65448/
> ---
> 
> (Updated Feb. 1, 2018, 2:04 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
> 
> 
> Bugs: MESOS-1720
> https://issues.apache.org/jira/browse/MESOS-1720
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test ensures that agent send exited executor message when the
> executor is never launched. So that master's executor bookkeeping
> entry is removed. See MESOS-1720.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 7112bb4efcc312dc4f68bdc44fb685c7624002b1 
>   src/tests/slave_tests.cpp f393a8433a984267adc4db307ef07fcbafd1e62f 
> 
> 
> Diff: https://reviews.apache.org/r/65448/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65465: Windows: Fixed recovery of Mesos containerizer.

[Joseph Wu]

> On Feb. 1, 2018, 2:32 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/main.cpp
> > Lines 40-50 (patched)
> > 
> >
> > Flying by. Why this logic is not in launch.cpp? Sounds to me it's 
> > unrelated to, for example, Mount below?
> 
> Andrew Schwartzmeyer wrote:
> Where in `launch.cpp` would you put it? The handle needs to exist for 
> exactly as long as the process exists (or as close as we can get, which 
> putting it here gets it really close).
> 
> Jie Yu wrote:
> well, i don't think putting here or in launch.cpp has any noticible 
> difference in terms of "closeness" (probably a dozen of instructions?).
> 
> my question is: is this logic only related to the launch of a container 
> or not? If yes, this should be moved to launch.cpp (i.e., 
> `MesosContainerizerLaunch::execute()`).

This is not exactly related to launching a Windows job object, as the reason 
for adding this code is for recovering the job object later.

Having it in `main.cpp` vs. `launch.cpp` doesn't make too much of a difference 
regarding the lifetime of the Handle (as `main.cpp` calls methods in 
`launch.cpp`), but it is safer to do this in `launch.cpp` as the `main.cpp` 
contains up to 3 subcommands (only one of which is long-lived).


- Joseph


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


On Feb. 1, 2018, 11:57 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65465/
> ---
> 
> (Updated Feb. 1, 2018, 11:57 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8519
> https://issues.apache.org/jira/browse/MESOS-8519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Windows OS deletes the job object created in the agent process when
> the agent dies, because no other process holds a handle to it (despite
> processes being assigned to the job object). While this is
> counter-intuitive, it is the observed behavior. So in order for recovery
> to succeed, the containerizer must also hold an otherwise unused handle
> to its job object to keep it alive in the kernel, and available for
> recovery to find.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/main.cpp 
> a53ccd68bf975d919f9d1f920cf3fa74d4e43f24 
> 
> 
> Diff: https://reviews.apache.org/r/65465/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> [--] Global test environment tear-down
> [==] 874 tests from 85 test cases ran. (253311 ms total)
> [  PASSED  ] 874 tests.
> 
> I0201 12:46:58.159368  3116 slave.cpp:6921] Recovering framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
> I0201 12:46:58.159368  3116 slave.cpp:8543] Recovering executor 
> 'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
> I0201 12:46:58.162847  9456 task_status_update_manager.cpp:207] Recovering 
> task status update manager
> I0201 12:46:58.162847  9456 task_status_update_manager.cpp:215] Recovering 
> executor 'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
> I0201 12:46:58.166851  7344 containerizer.cpp:674] Recovering containerizer
> I0201 12:46:58.167351  7344 containerizer.cpp:731] Recovering container 
> 69cefa53-61e0-444b-a808-e38ffb4cb18f for executor 
> 'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
> I0201 12:46:58.183379 17088 provisioner.cpp:493] Provisioner recovery complete
> I0201 12:46:58.186367 16792 slave.cpp:6695] Sending reconnect request to 
> executor 'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf- at executor(1)@10.123.7.41:52591
> I0201 12:46:58.194370  7344 slave.cpp:4519] Received re-registration message 
> from executor 'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
> I0201 12:47:00.193958 16792 slave.cpp:4737] Cleaning up un-reregistered 
> executors
> I0201 12:47:00.193958 16792 slave.cpp:6824] Finished recovery
> I0201 12:47:00.200943  9456 task_status_update_manager.cpp:181] Pausing 
> sending task status updates
> I0201 12:47:00.200943  3116 slave.cpp:1146] New master detected at 
> master@10.123.6.78:5050
> I0201 12:47:00.200943  3116 slave.cpp:1190] No credentials provided. 
> Attempting to register without authentication
> I0201 12:47:00.200943  3116 slave.cpp:1201] Detecting new master
> I0201 12:47:00.214944 16792 slave.cpp:1471] Re-registered with master 
> master@10.123.6.78:5050
> I0201 12:47:00.214944 13180 

Re: Review Request 65408: Windows: Ported `slave_recovery_tests.cpp`.

[Joseph Wu]

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


Ship it!




Couple of comment tweaks for you to do before committing.


src/tests/slave_recovery_tests.cpp
Lines 3258 (patched)


s/no/not/



src/tests/slave_recovery_tests.cpp
Lines 4449 (patched)


We stopped putting the extra `!` in this `#endif`s a while ago.

(There are only a few places left which haven't been cleaned up yet).



src/tests/slave_recovery_tests.cpp
Lines 4496 (patched)


Ditto about the `!`.



src/tests/slave_recovery_tests.cpp
Lines 4729-4730 (patched)


s/default Windows memory/default Windows isolators/


- Joseph Wu


On Feb. 1, 2018, 4:14 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65408/
> ---
> 
> (Updated Feb. 1, 2018, 4:14 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-6713
> https://issues.apache.org/jira/browse/MESOS-6713
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit enables the unit tests found in `slave_recovery_tests.cpp`
> to test agent recovery on Windows.
> 
> 
> Diffs
> -
> 
>   src/tests/CMakeLists.txt 6f1c67a6b4003ada1a0635a68fb7471895a2a6f5 
>   src/tests/slave_recovery_tests.cpp 77aa60c953bd0769eaba05f001755e4cec9ba028 
> 
> 
> Diff: https://reviews.apache.org/r/65408/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65409: Fixed `SlaveRecoveryTest.ReconcileTasksMissingFromSlave`.

[Joseph Wu]

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




src/tests/slave_recovery_tests.cpp
Line 3832 (original), 3832 (patched)


The reset should ideally go right below this line.  I believe the test 
should not be reliant on any data structures of the agent existing after 
termination.

You can probably move the comment up here too.


- Joseph Wu


On Feb. 1, 2018, 4:15 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65409/
> ---
> 
> (Updated Feb. 1, 2018, 4:15 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-6713
> https://issues.apache.org/jira/browse/MESOS-6713
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Because it is not possible to delete a file (or a folder recursively)
> with open handles on Windows, we have to explicitly `reset()` the agent
> before removing the framework meta directory. Otherwise, the task status
> update manager will be destructed too late, and so an open handle for
> `task.updates` will cause the `os::rmdir` to fail.
> 
> This is safe because we previously destructed the agent anyway, just
> later in the test when it was reassigned.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 77aa60c953bd0769eaba05f001755e4cec9ba028 
> 
> 
> Diff: https://reviews.apache.org/r/65409/diff/2/
> 
> 
> Testing
> ---
> 
> make check on CentOS 7, all passed
> ctest on Windows, all passed including new SlaveRecoveryTests
> 
> Note that while this chain enables recovery of Docker tasks on Windows, it 
> explicitly does not fix MESOS-8519 (recovery of job object tasks).
> 
> ```
> I0131 11:52:01.545505  8316 docker.cpp:898] Recovering Docker containers
> I0131 11:52:01.546005   660 containerizer.cpp:674] Recovering containerizer
> I0131 11:52:01.546505   660 containerizer.cpp:725] Skipping recovery of 
> executor 'iis.feae9d12-06ba-11e8-8f77-02421c3bc93c' of framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf- because it was not launched from 
> mesos containerizer
> I0131 11:52:01.557006 11272 provisioner.cpp:493] Provisioner recovery complete
> I0131 11:52:02.521003  8720 docker.cpp:1008] Recovering container 
> 'f7978e90-32f5-458d-ad4e-3ffa25a7b190' for executor 
> 'iis.feae9d12-06ba-11e8-8f77-02421c3bc93c' of framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
> I0131 11:52:02.530527  8316 slave.cpp:6695] Sending reconnect request to 
> executor 'iis.feae9d12-06ba-11e8-8f77-02421c3bc93c' of framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf- at executor(1)@10.123.7.41:63903
> I0131 11:52:02.549062  8720 slave.cpp:4519] Received re-registration message 
> from executor 'iis.feae9d12-06ba-11e8-8f77-02421c3bc93c' of framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
> I0131 11:52:04.548064 10556 slave.cpp:4737] Cleaning up un-reregistered 
> executors
> I0131 11:52:04.548064 10556 slave.cpp:6824] Finished recovery
> I0131 11:52:04.566066   660 task_status_update_manager.cpp:181] Pausing 
> sending task status updates
> I0131 11:52:04.567059 14636 slave.cpp:1146] New master detected at 
> master@10.123.6.78:5050
> I0131 11:52:04.567059 14636 slave.cpp:1190] No credentials provided. 
> Attempting to register without authentication
> I0131 11:52:04.568047 14636 slave.cpp:1201] Detecting new master
> I0131 11:52:04.604035  8720 slave.cpp:1471] Re-registered with master 
> master@10.123.6.78:5050
> I0131 11:52:04.605060   660 task_status_update_manager.cpp:188] Resuming 
> sending task status updates
> I0131 11:52:04.606036  8720 slave.cpp:1516] Forwarding agent update 
> {"operations":{},"resource_version_uuid":{"value":"mzwol7M6SrGxOml4zYlA8Q=="},"slave_id":{"value":"7dc02270-a4e1-4f59-9ad7-56bad5182ea4-S0"},"update_oversubscribed_resource
> s":true}
> I0131 11:52:04.612036  8720 slave.cpp:3625] Updating info for framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf- with pid updated to 
> scheduler-aaa62980-8b1b-4775-b8bb-c6890b41941e@10.123.6.78:45907
> I0131 11:52:04.636543 13468 task_status_update_manager.cpp:188] Resuming 
> sending task status updates
> ```
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65406: Added `TYPED_TEST_TEMP_DISABLED_ON_WINDOWS` macro.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Jan. 29, 2018, 12:39 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65406/
> ---
> 
> (Updated Jan. 29, 2018, 12:39 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This lets us temporarily disable `TYPED_TEST` unit tests on Windows but
> not elsewhere.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/gtest.hpp 
> a8ae3d90519aae7788e0874a22776cff682173ba 
> 
> 
> Diff: https://reviews.apache.org/r/65406/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65407: Windows: Enabled tests that were blocked by MESOS-7604.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Feb. 1, 2018, 4:03 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65407/
> ---
> 
> (Updated Feb. 1, 2018, 4:03 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7604
> https://issues.apache.org/jira/browse/MESOS-7604
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit enables tests that were previously disabled on Windows due
> to MESOS-7604.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp b639a4bafbf69d3ea53aa0573e9f0489c3a1a57f 
>   src/tests/gc_tests.cpp 0e34c017f2c629b527915218f517b88d8392aa4b 
>   src/tests/master_tests.cpp 7112bb4efcc312dc4f68bdc44fb685c7624002b1 
>   src/tests/slave_tests.cpp f393a8433a984267adc4db307ef07fcbafd1e62f 
> 
> 
> Diff: https://reviews.apache.org/r/65407/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65407: Windows: Enabled tests that were blocked by MESOS-7604.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Feb. 1, 2018, 4:03 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65407/
> ---
> 
> (Updated Feb. 1, 2018, 4:03 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7604
> https://issues.apache.org/jira/browse/MESOS-7604
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit enables tests that were previously disabled on Windows due
> to MESOS-7604.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp b639a4bafbf69d3ea53aa0573e9f0489c3a1a57f 
>   src/tests/gc_tests.cpp 0e34c017f2c629b527915218f517b88d8392aa4b 
>   src/tests/master_tests.cpp 7112bb4efcc312dc4f68bdc44fb685c7624002b1 
>   src/tests/slave_tests.cpp f393a8433a984267adc4db307ef07fcbafd1e62f 
> 
> 
> Diff: https://reviews.apache.org/r/65407/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65538: WIP: Added provider assignments into `disk_profile.proto`.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['65032', '65471', '65472', '65060', '65499', '65520', 
'65534', '65523', '65521', '65061', '65522', '65538']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
 D:\DCOS\mesos\mesos\src\java\jni\convert.cpp(415): warning C4267: 
'argument': conversion from 'size_t' to 'jsize', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\java\jni\convert.cpp(437): warning C4267: 
'argument': conversion from 'size_t' to 'jsize', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\java\jni\convert.cpp(438): warning C4267: 
'argument': conversion from 'size_t' to 'jsize', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\java\jni\convert.cpp(460): warning C4267: 
'argument': conversion from 'size_t' to 'jsize', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\java\jni\convert.cpp(461): warning C4267: 
'argument': conversion from 'size_t' to 'jsize', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\java\jni\convert.cpp(500): warning C4267: 
'argument': conversion from 'size_t' to 'jsize', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\java\jni\convert.cpp(501): warning C4267: 
'argument': conversion from 'size_t' to 'jsize', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_state_Variable.cpp(44): 
warning C4267: 'argument': conversion from 'size_t' to 'jsize', possible loss 
of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_state_Variable.cpp(45): 
warning C4267: 'argument': conversion from 'size_t' to 'jsize', possible loss 
of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(95): warning C4267: '=': 
conversion from 'size_t' to 'jint', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(463): warning C4244: 
'initializing': conversion from 'jchar' to 'char', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(493): warning C4244: 
'initializing': conversion from 'jlong' to 'long', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(654): warning C4244: 
'initializing': conversion from 'jchar' to 'char', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(693): warning C4244: 
'initializing': conversion from 'jlong' to 'long', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(770): warning C4244: 
'initializing': conversion from 'jchar' to 'char', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
 D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(809): warning C4244: 
'initializing': conversion from 'jlong' to 'long', possible loss of data 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]


   "D:\DCOS\mesos\src\tests\mesos-tests.vcxproj" (default target) (1) ->
   (Link target) -> 
 cluster.obj : error LNK2019: unresolved external symbol "class 
Try,class std::allocator >,class std::allocator,class std::allocator 
> > >,class Error> __cdecl mesos::csi::paths::getContainerPaths(class 
std::basic_string const &,class std::basic_string const &,class std::basic_string const &)" 
(?getContainerPaths@paths@csi@mesos@@YA?AV?$Try@V?$list@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V?$allocator@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@2@@std@@VErrorAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@00@Z)
 referenced in function "public: void __cdecl ::operator()(void)const " 
(??R@@QEBAXXZ) 
[D:\DCOS\mesos\src\tests\mesos-tests.vcxproj]
 cluster.obj : error LNK2019: unresolved external symbol "class 
Try __cdecl 
mesos::csi::paths::parseContainerPath(class std::basic_string const 

Re: Review Request 65522: Fixed some log messages in `slave.cpp`.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 6, 2018, 4:17 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65522/
> ---
> 
> (Updated Feb. 6, 2018, 4:17 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed some log messages in `slave.cpp`.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 
> 
> 
> Diff: https://reviews.apache.org/r/65522/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65405: Implemented `net::socket()` for Windows using `WSASocket`.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Feb. 1, 2018, 3:30 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65405/
> ---
> 
> (Updated Feb. 1, 2018, 3:30 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-5882
> https://issues.apache.org/jira/browse/MESOS-5882
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than reusing the existing `net::socket()` wrapper for Windows,
> the original wrapper was moved to `posix/socket.hpp`, and a new wrapper
> was written in `windows/socket.hpp` which uses `::WSASocket()` instead
> of `::socket()` and takes an optional set of Windows socket flags.
> 
> We enable `WSA_FLAG_OVERLAPPED` so that the socket supports overlapped
> I/O operations (the default behavior of `::socket()`; MSDN states that
> most sockets should be created with this flag set, so it is a reasonable
> default.
> 
> Furthermore, we enable `WSA_FLAG_NO_HANDLE_INHERIT`, which provides the
> semantics of close-on-exec. This is set by default because (a) it is not
> possible to change afterwards, and (b) we never use inheritable sockets.
> Therefore it reasonable to also disable inheritance.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/socket.hpp 
> bab0b808f53abd1314a7d13fc0cba75e5717f96f 
>   3rdparty/stout/include/stout/os/socket.hpp 
> 2ca4465ca23c9ca59239947c9babf8dd0212fafd 
>   3rdparty/stout/include/stout/os/windows/socket.hpp 
> 020c5e2367217cd2b4439ae5e2d5be4b31b4226b 
> 
> 
> Diff: https://reviews.apache.org/r/65405/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Review Request 65538: WIP: Added provider assignments into `disk_profile.proto`.

[Chun-Hung Hsiao]

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

Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.


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


Repository: mesos


Description
---

A new `assignments` field is added to the `DiskProfileMapping` protobuf
so that the URI disk profile adaptor can be customized to notify each
resource provider a different set of profiles.


Diffs
-

  src/resource_provider/storage/disk_profile.proto 
6cf1f8abcd24e45a292efc95a395f90bb2140da2 


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


Testing
---


Thanks,

Chun-Hung Hsiao

Re: Review Request 65061: Speeded up tests for resource provider config agent API.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 6, 2018, 4:14 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65061/
> ---
> 
> (Updated Feb. 6, 2018, 4:14 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Jie Yu.
> 
> 
> Bugs: MESOS-8426
> https://issues.apache.org/jira/browse/MESOS-8426
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speeded up tests for resource provider config agent API.
> 
> 
> Diffs
> -
> 
>   src/tests/agent_resource_provider_config_api_tests.cpp 
> 7cbee5cceef2963b736f3e6ce3f52fcf0b3c029c 
> 
> 
> Diff: https://reviews.apache.org/r/65061/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65499: Fixed the flakiness of the ROOT_ConvertPreExistingVolume test.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 3, 2018, 1:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65499/
> ---
> 
> (Updated Feb. 3, 2018, 1:40 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-8474
> https://issues.apache.org/jira/browse/MESOS-8474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This unit test is flaky because the master might send out an offer
> before both offer operations of a previous `ACCEPT` call are finished.
> This is fixed by manipulating the clock such that no offer won't be
> sent out until the master receives status updates for both operations.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> ce241617ee555e5a67e552dd30b0e7afee161386 
> 
> 
> Diff: https://reviews.apache.org/r/65499/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> Ran in repetition.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65060: Cleaned up endpoint directories after SLRP tests.

[Jie Yu]

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




src/tests/cluster.cpp
Lines 685 (patched)


Can we move to the SLRP test fixture? I don't think it's maintainable to 
have all cleanup code in a single place.

For instance, CNI test cleanup code is very networking related, better to 
put in CNI test fixtures, not in a global location.


- Jie Yu


On Feb. 3, 2018, 1:37 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65060/
> ---
> 
> (Updated Feb. 3, 2018, 1:37 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8427
> https://issues.apache.org/jira/browse/MESOS-8427
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up endpoint directories after SLRP tests.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp 19a41c7c1c303ad806daa4e5e3765a1e0b55933b 
> 
> 
> Diff: https://reviews.apache.org/r/65060/diff/5/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65472: Added `csi::paths::parseContainerPath` helper.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 2, 2018, 5:23 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65472/
> ---
> 
> (Updated Feb. 2, 2018, 5:23 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `csi::paths::parseContainerPath` helper.
> 
> 
> Diffs
> -
> 
>   src/csi/paths.hpp 37f65f85af2791c02a96885014db1de64a1c853f 
>   src/csi/paths.cpp 60fca882dab5c599ef81b3005f031c54fc155d75 
>   src/resource_provider/storage/provider.cpp 
> 604cadff20a6156639db10a28a180e53a959ff29 
> 
> 
> Diff: https://reviews.apache.org/r/65472/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65471: Made the SLRP tests more consistent in coding style.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 2, 2018, 5:08 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65471/
> ---
> 
> (Updated Feb. 2, 2018, 5:08 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the SLRP tests more consistent in coding style.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> ce241617ee555e5a67e552dd30b0e7afee161386 
> 
> 
> Diff: https://reviews.apache.org/r/65471/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65032: Added a SLRP unit test for agent reboot.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 2, 2018, 5:07 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65032/
> ---
> 
> (Updated Feb. 2, 2018, 5:07 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8415
> https://issues.apache.org/jira/browse/MESOS-8415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a SLRP unit test for agent reboot.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> ce241617ee555e5a67e552dd30b0e7afee161386 
> 
> 
> Diff: https://reviews.apache.org/r/65032/diff/5/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65467: Windows: Added `internal::windows::enable_inherit(WindowsFD)`.

[Andrew Schwartzmeyer]

> On Feb. 6, 2018, 1:55 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/internal/windows/inherit.hpp
> > Lines 31-43 (patched)
> > 
> >
> > This is basically what the ChildHook `UNSET_CLOEXEC` 
> > (libprocess/src/subprocess.cpp) should be doing.

Pretty much, but we don't have any child hook support on Windows whatsoever. 
Adding support for child hooks would be a non-trivial undertaking (we don't 
have a pseudo-program written to run child hooks and launch a process like on 
Linux, we're just launching the process directly).


- Andrew


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


On Feb. 2, 2018, 12:13 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65467/
> ---
> 
> (Updated Feb. 2, 2018, 12:13 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8512
> https://issues.apache.org/jira/browse/MESOS-8512
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This function is used on Windows to explicitly enable inheritance on a
> handle.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65467/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65467: Windows: Added `internal::windows::enable_inherit(WindowsFD)`.

[Joseph Wu]

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




3rdparty/stout/include/stout/internal/windows/inherit.hpp
Lines 31-43 (patched)


This is basically what the ChildHook `UNSET_CLOEXEC` 
(libprocess/src/subprocess.cpp) should be doing.


- Joseph Wu


On Feb. 2, 2018, 12:13 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65467/
> ---
> 
> (Updated Feb. 2, 2018, 12:13 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8512
> https://issues.apache.org/jira/browse/MESOS-8512
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This function is used on Windows to explicitly enable inheritance on a
> handle.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65467/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65202: Adopted `DEFAULT_TEST_TIMEOUT` in Mesos tests.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65201, 65343, 65298, 65202]

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

- Mesos Reviewbot


On Feb. 5, 2018, 10:50 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65202/
> ---
> 
> (Updated Feb. 5, 2018, 10:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gaston Kleiman.
> 
> 
> Bugs: MESOS-7016
> https://issues.apache.org/jira/browse/MESOS-7016
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than hard-coding a 15sec test timeout, use the libprocess
> `DEFAULT_TEST_TIMEOUT` variable so that this can be globally tuned.
> Added a test flag `--default_await_timeout` that can be used to set
> the `DEFAULT_TEST_TIMEOUT` on a test run.
> 
> 
> Diffs
> -
> 
>   src/tests/agent_container_api_tests.cpp 
> 618569277545205017320aaf1f3a70e540d35e30 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 5412a7eccaa836a7f182772f1eb33b10298c96a1 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> d761ea46fd4f12c6bed74090bf5ead7587f16811 
>   src/tests/disk_quota_tests.cpp 31cf4c44b1494afd3501046abffd303c7a1307f8 
>   src/tests/fetcher_cache_tests.cpp 7db7b7dcef27b1686ccae5a7408ff2811a3b9255 
>   src/tests/flags.hpp df4d4588f405ea884bb89021264e70218e054fae 
>   src/tests/flags.cpp 4542f3443830474edd97e8b695af0c61685e947c 
>   src/tests/gc_tests.cpp 0e34c017f2c629b527915218f517b88d8392aa4b 
>   src/tests/main.cpp f945ac9e8858998174b4c2785f6a5612bae66a41 
>   src/tests/master_contender_detector_tests.cpp 
> 015d310b72a152829641d436f7cb558f72dfd77c 
>   src/tests/registrar_tests.cpp cbb58c1cd72dc13ed16b5465452d17526255c763 
>   src/tests/slave_recovery_tests.cpp 77aa60c953bd0769eaba05f001755e4cec9ba028 
>   src/tests/slave_tests.cpp f393a8433a984267adc4db307ef07fcbafd1e62f 
> 
> 
> Diff: https://reviews.apache.org/r/65202/diff/8/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 65403: Windows: Disabled `O_CLOEXEC` semantic mapping on Windows.

[Joseph Wu]

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


Ship it!




- Joseph Wu


On Feb. 1, 2018, 3:23 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65403/
> ---
> 
> (Updated Feb. 1, 2018, 3:23 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-5882
> https://issues.apache.org/jira/browse/MESOS-5882
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously we mapped `O_CLOEXEC` to `O_NOINHERIT`, which is close to the
> intended semantics, but not close enough. Instead, we want all Windows
> handles by default to be non-inheritable, and not tie any Windows
> inheritance semantics to the close-on-exec semantics. So, we define
> `O_CLOEXEC` on Windows to be `0`, and remove the logging from the
> `os::cloexec` family of functions because do not ever intend to
> implement them.
> 
> We achieve the "close" part of the semantics by making all handles
> non-inheritable, as the intent is to avoid leaking handles.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/open.hpp 
> 6711f22d44b0e7c13da3abd51f9fb7b71779e868 
>   3rdparty/stout/include/stout/os/windows/fcntl.hpp 
> 5800ec92f85401a80cb813afd880be2e5a24a3af 
> 
> 
> Diff: https://reviews.apache.org/r/65403/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65402: Windows: Use Unicode API to duplicate sockets.

[Joseph Wu]

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


Ship it!





3rdparty/stout/include/stout/os/windows/dup.hpp
Line 49 (original), 43-44 (patched)


Extra newline here.


- Joseph Wu


On Jan. 29, 2018, 12:35 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65402/
> ---
> 
> (Updated Jan. 29, 2018, 12:35 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The CRT API for duplicating file handles is left as-is because
> `WindowsFD` currently gets a CRT `int` file descriptor, which
> necessitates using `_dup` instead of `DuplicateHandle`. Furthermore, the
> latter API does not appear compatible with handles used to redirect
> stdout/stderr/stdin for subprocesses.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/dup.hpp 
> b37aaa6429e0e7b4340e86c078f5c543e443bdcd 
> 
> 
> Diff: https://reviews.apache.org/r/65402/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65401: Windows: Enabled `Flags::runtime_directory` for checkpointing.

[Joseph Wu]

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


Ship it!




- Joseph Wu


On Feb. 1, 2018, 3:22 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65401/
> ---
> 
> (Updated Feb. 1, 2018, 3:22 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-6713
> https://issues.apache.org/jira/browse/MESOS-6713
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This had previously been compiled out on Windows, but we bring it back
> in order to support checkpointing.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.hpp 
> 49eb9128ae5a1e4ec02e2b9d9e3cb67d7a8f7663 
>   src/slave/containerizer/mesos/launch.cpp 
> 91016ed417428e3a5b21a132a96b9d7760d13aa3 
> 
> 
> Diff: https://reviews.apache.org/r/65401/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65400: Windows: Tied task lifetimes to executors.

[Joseph Wu]

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


Ship it!




- Joseph Wu


On Jan. 29, 2018, 12:34 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65400/
> ---
> 
> (Updated Jan. 29, 2018, 12:34 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-6713
> https://issues.apache.org/jira/browse/MESOS-6713
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To enable recovery of checkpointed tasks, the agent must be able to die
> without also killing the executors and tasks, thus we cannot set the
> "job object kill on close" limit unconditionally. However, the executors
> must still be able to kill their tasks when they die, so we explicitly
> enable this limit through a parent hook when launching the container for
> the task. In this way, the agent can be restarted (e.g. for an upgrade)
> without killing the executors, but the executors are still capable of
> killing their tasks on catastrophic death.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 050f5a057f360873e2b4738b126289bcd1bd0c7f 
> 
> 
> Diff: https://reviews.apache.org/r/65400/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65399: Windows: Moved "kill on close" job object flag to own function.

[Joseph Wu]

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


Fix it, then Ship it!




Style nits only.


3rdparty/stout/include/stout/windows/os.hpp
Lines 766-767 (patched)


You could move this right above the `SetInformationJobObject` call which 
uses it.



3rdparty/stout/include/stout/windows/os.hpp
Lines 772-773 (patched)


Insert a newline after indented lines like this.



3rdparty/stout/include/stout/windows/os.hpp
Lines 778-781 (patched)


4 space indent



3rdparty/stout/include/stout/windows/os.hpp
Lines 781-782 (patched)


Newline ditto.



3rdparty/stout/include/stout/windows/os.hpp
Lines 784 (patched)


4 space indent


- Joseph Wu


On Jan. 29, 2018, 12:34 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65399/
> ---
> 
> (Updated Jan. 29, 2018, 12:34 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-6713
> https://issues.apache.org/jira/browse/MESOS-6713
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of setting this flag unconditionally when we create a job
> object, this patch abstracts it to the the function
> `set_job_kill_on_close_limit(pid_t)` so that users can choose how to tie
> job object lifetimes together.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> ec935209fc1532ebe087ac20933c99eed393506d 
> 
> 
> Diff: https://reviews.apache.org/r/65399/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65398: Removed workaround in ZooKeeper test.

[Joseph Wu]

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


Ship it!




- Joseph Wu


On Jan. 29, 2018, 12:32 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65398/
> ---
> 
> (Updated Jan. 29, 2018, 12:32 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7803
> https://issues.apache.org/jira/browse/MESOS-7803
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now that `fs::list` returns the full path, this code can be fixed.
> 
> 
> Diffs
> -
> 
>   src/tests/zookeeper.cpp 54cf9c63878b5b6d30df27838244eec46312d917 
> 
> 
> Diff: https://reviews.apache.org/r/65398/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65397: Windows: Fixed `fs::list` to return full paths.

[Joseph Wu]

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


Ship it!




LGTM.

- Joseph Wu


On Jan. 29, 2018, 12:32 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65397/
> ---
> 
> (Updated Jan. 29, 2018, 12:32 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7803
> https://issues.apache.org/jira/browse/MESOS-7803
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This resolves MESOS-7803, where `fs::list` only returned the name of the
> found files, dropping the initial path components. That is, for a
> directory `foo` with the file `bar` in it, and the pattern
> `fs::list("foo/*")`, it was incorrectly returning just `bar`, not
> `foo/bar`. This was fixed by getting the `dirname` of the pattern, and
> rejoining it with each found file name before it is returned.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/fs.hpp 
> bc7cf025c9228a65f2ae0fab9ba27b386d189b46 
> 
> 
> Diff: https://reviews.apache.org/r/65397/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65445: Added new protobuf field `launch_executor` in RunTask(Group)Message.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Feb. 5, 2018, 2:44 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65445/
> ---
> 
> (Updated Feb. 5, 2018, 2:44 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
> 
> 
> Bugs: MESOS-1720
> https://issues.apache.org/jira/browse/MESOS-1720
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This boolean flag is used for the master to specify whether a
> new executor should be launched for the task or taskGroup (with
> the exception of command executor). This will let the master
> to control executor creation on the agent.
> 
> Also updated the relevant message handlers and mock functions.
> 
> 
> Diffs
> -
> 
>   src/messages/messages.proto 0db44a24979cfdc748cd7fa3acd9e0346b14cfd3 
>   src/slave/slave.hpp 09c01ebd1b5e8008ba9e7d412042f1db76a2c5a5 
>   src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 
>   src/tests/mock_slave.hpp 29ce7140501888d95d5f2d6c26b752ad276b484a 
>   src/tests/mock_slave.cpp 8357edc7b3a35624c813eccb9ecca9d3b5dbe07c 
>   src/tests/slave_tests.cpp f393a8433a984267adc4db307ef07fcbafd1e62f 
> 
> 
> Diff: https://reviews.apache.org/r/65445/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65520: Reverted plugin name randomization in SLRP tests.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 6, 2018, 4:08 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65520/
> ---
> 
> (Updated Feb. 6, 2018, 4:08 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The plugin name is no longer randomized since it is no longer needed
> for test isolation. Also removed an unnecessary member variable in the
> SLRP test class.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> ce241617ee555e5a67e552dd30b0e7afee161386 
> 
> 
> Diff: https://reviews.apache.org/r/65520/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65534: Improved logging for container daemon.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 6, 2018, 7:11 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65534/
> ---
> 
> (Updated Feb. 6, 2018, 7:11 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved logging for container daemon.
> 
> 
> Diffs
> -
> 
>   src/slave/container_daemon.cpp 6458d1ff1006f321363dc934ae0483ccdbbdc742 
> 
> 
> Diff: https://reviews.apache.org/r/65534/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65521: Improved isolation for agent RP API config tests.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 6, 2018, 4:11 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65521/
> ---
> 
> (Updated Feb. 6, 2018, 4:11 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the tests to use a unique cgroup for each run. It also
> changes `CHECK_*` macros to `ASSERT_*` so the tests can be torn down
> properly upon assertion failures.
> 
> 
> Diffs
> -
> 
>   src/tests/agent_resource_provider_config_api_tests.cpp 
> 7cbee5cceef2963b736f3e6ce3f52fcf0b3c029c 
> 
> 
> Diff: https://reviews.apache.org/r/65521/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65523: Fixed the flakiness in the SLRP metrics test.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 6, 2018, 7:12 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65523/
> ---
> 
> (Updated Feb. 6, 2018, 7:12 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Jie Yu.
> 
> 
> Bugs: MESOS-8548
> https://issues.apache.org/jira/browse/MESOS-8548
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The metrics test unnecessarily set up the disk profile module without
> providing the profile config file. This may cause the profile module to
> pull a non-existent file. Also, it could kill the plugin before SLRP
> connecting to the endpoint, making SLRP wait for 1 minute.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> ce241617ee555e5a67e552dd30b0e7afee161386 
> 
> 
> Diff: https://reviews.apache.org/r/65523/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65246: Added download button for master and agent logs in Web UI.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65246 was successfully built and tested.

Reviews applied: `['65246']`

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

- Mesos Reviewbot Windows


On Feb. 6, 2018, 6:43 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65246/
> ---
> 
> (Updated Feb. 6, 2018, 6:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-8454
> https://issues.apache.org/jira/browse/MESOS-8454
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
>   src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
>   src/webui/master/static/js/controllers.js 
> 59665869dbea77c00740e42f2590473181dfe2fe 
> 
> 
> Diff: https://reviews.apache.org/r/65246/diff/4/
> 
> 
> Testing
> ---
> 
> Tested using Google Chrome 64.0.3282.119.
> 
> Created an High Availability Mode Mesos cluster locally:
> ```
> $ zkServer start
> $ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' 
> --work_dir='/tmp/master1' --quorum=1 
> --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' 
> --log_dir='/tmp/master1-log'
> $ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' 
> --work_dir='/tmp/master2' --quorum=1 
> --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' 
> --log_dir='/tmp/master2-log'
> $ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' 
> --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
> ```
> Tested the download and streaming features on the home and agents Web UI 
> endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.
> 
> New UI (masters):
> ![New logs button master](https://i.imgur.com/Uz8aj1H.png)
> 
> New UI (agents):
> ![New logs button](https://i.imgur.com/tmGavCL.png)
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 65522: Fixed some log messages in `slave.cpp`.

[Gaston Kleiman]

> On Feb. 6, 2018, 10:29 a.m., Gaston Kleiman wrote:
> > src/slave/slave.cpp
> > Line 9530 (original), 9529 (patched)
> > 
> >
> > I noticed that if a single task is passed, we surround its ID in single 
> > quotes, but we don't do that if a task groups is passed.
> > 
> > We might want to do this consistently.
> 
> Chun-Hung Hsiao wrote:
> Would you prefer iterating throuch each ID and qouting them, or add a 
> pair of brackets surrounding them?

I'd vote for the former.


- Gaston


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


On Feb. 5, 2018, 8:17 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65522/
> ---
> 
> (Updated Feb. 5, 2018, 8:17 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed some log messages in `slave.cpp`.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 
> 
> 
> Diff: https://reviews.apache.org/r/65522/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65522: Fixed some log messages in `slave.cpp`.

[Chun-Hung Hsiao]

> On Feb. 6, 2018, 6:29 p.m., Gaston Kleiman wrote:
> > src/slave/slave.cpp
> > Line 9530 (original), 9529 (patched)
> > 
> >
> > I noticed that if a single task is passed, we surround its ID in single 
> > quotes, but we don't do that if a task groups is passed.
> > 
> > We might want to do this consistently.

Would you prefer iterating throuch each ID and qouting them, or add a pair of 
brackets surrounding them?


- Chun-Hung


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


On Feb. 6, 2018, 4:17 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65522/
> ---
> 
> (Updated Feb. 6, 2018, 4:17 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed some log messages in `slave.cpp`.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 
> 
> 
> Diff: https://reviews.apache.org/r/65522/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65510: Enable 'curl' uri_fetcher tests on Windows platform.

[Andrew Schwartzmeyer]

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


Ship it!




Nit: Fix the summary and description.

- Andrew Schwartzmeyer


On Feb. 5, 2018, 11:54 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65510/
> ---
> 
> (Updated Feb. 5, 2018, 11:54 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enable 'curl' uri_fetcher tests on Windows platform.
> 
> 
> Diffs
> -
> 
>   src/tests/uri_fetcher_tests.cpp 8a40f46b215bb1f267a59a9edfca83445f86b430 
> 
> 
> Diff: https://reviews.apache.org/r/65510/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 65509: The CURL_ prefix in unit test name should always succeed on Windows.

[Andrew Schwartzmeyer]

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


Fix it, then Ship it!




We should also note in the description that `curl` on Windows is built 
_unconditionally_, and furthermore, is placed in a location such that it can 
always be found (as part of the fix for health checks).

That is, it is not incidental that we build `curl` on Windows and can find it. 
It's a requirement of `mesos-agent` that it is built and findable.


src/tests/environment.cpp
Lines 223 (patched)


Nit: this should be `false` instead of `0` to match the above return type 
(boolean).


- Andrew Schwartzmeyer


On Feb. 5, 2018, 11:53 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65509/
> ---
> 
> (Updated Feb. 5, 2018, 11:53 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the 'curl.exe' executable is built on part of Windows (it's a
> build target), it will always exist for test purposes.
> 
> 
> Diffs
> -
> 
>   src/tests/environment.cpp 13a4c9514fcd3016fe623c597decd067457e86cd 
> 
> 
> Diff: https://reviews.apache.org/r/65509/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 65508: Allow curl program to be properly executed on Windows platform.

[Andrew Schwartzmeyer]

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


Ship it!




Nit: Fix summary to be in past tense, and fix description to not be a copy of 
the summary.

- Andrew Schwartzmeyer


On Feb. 5, 2018, 10:12 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65508/
> ---
> 
> (Updated Feb. 5, 2018, 10:12 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow curl program to be properly executed on Windows platform.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/curl.cpp f34daf2bc1f3fef74548e04ad332e3a627636bfd 
> 
> 
> Diff: https://reviews.apache.org/r/65508/diff/1/
> 
> 
> Testing
> ---
> 
> Full build, unit tests on both Linux and Windows platform.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 65523: Fixed the flakiness in the SLRP metrics test.

[Chun-Hung Hsiao]

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

(Updated Feb. 6, 2018, 7:12 p.m.)


Review request for mesos, Greg Mann and Jie Yu.


Summary (updated)
-

Fixed the flakiness in the SLRP metrics test.


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


Repository: mesos


Description (updated)
---

The metrics test unnecessarily set up the disk profile module without
providing the profile config file. This may cause the profile module to
pull a non-existent file. Also, it could kill the plugin before SLRP
connecting to the endpoint, making SLRP wait for 1 minute.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
ce241617ee555e5a67e552dd30b0e7afee161386 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65508: Allow curl program to be properly executed on Windows platform.

[Andrew Schwartzmeyer]

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




src/uri/fetchers/curl.cpp
Line 99 (original), 99-104 (patched)


Nit: Run `clang-format` please; line 99 is >80 chars.


- Andrew Schwartzmeyer


On Feb. 5, 2018, 10:12 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65508/
> ---
> 
> (Updated Feb. 5, 2018, 10:12 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow curl program to be properly executed on Windows platform.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/curl.cpp f34daf2bc1f3fef74548e04ad332e3a627636bfd 
> 
> 
> Diff: https://reviews.apache.org/r/65508/diff/1/
> 
> 
> Testing
> ---
> 
> Full build, unit tests on both Linux and Windows platform.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Review Request 65534: Improved logging for container daemon.

[Chun-Hung Hsiao]

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

Review request for mesos, Greg Mann and Jie Yu.


Repository: mesos


Description
---

Improved logging for container daemon.


Diffs
-

  src/slave/container_daemon.cpp 6458d1ff1006f321363dc934ae0483ccdbbdc742 


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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65246: Added download button for master and agent logs in Web UI.

[Armand Grillet]

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

(Updated Feb. 6, 2018, 6:43 p.m.)


Review request for mesos, Benjamin Mahler and Vinod Kone.


Changes
---

Fixed last issue.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
  src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
  src/webui/master/static/js/controllers.js 
59665869dbea77c00740e42f2590473181dfe2fe 


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

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


Testing
---

Tested using Google Chrome 64.0.3282.119.

Created an High Availability Mode Mesos cluster locally:
```
$ zkServer start
$ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' 
--work_dir='/tmp/master1' --quorum=1 
--webui_dir='/Users/Armand/Code/apache-mesos/src/webui' 
--log_dir='/tmp/master1-log'
$ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' 
--work_dir='/tmp/master2' --quorum=1 
--webui_dir='/Users/Armand/Code/apache-mesos/src/webui' 
--log_dir='/tmp/master2-log'
$ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' 
--work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
```
Tested the download and streaming features on the home and agents Web UI 
endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.

New UI (masters):
![New logs button master](https://i.imgur.com/Uz8aj1H.png)

New UI (agents):
![New logs button](https://i.imgur.com/tmGavCL.png)


Thanks,

Armand Grillet

Re: Review Request 65497: Added test to verify task-less executor is shutdown when re-subscribing.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65497 was successfully built and tested.

Reviews applied: `['65109', '65110', '65111', '65369', '65496', '65497']`

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

- Mesos Reviewbot Windows


On Feb. 3, 2018, 1:18 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65497/
> ---
> 
> (Updated Feb. 3, 2018, 1:18 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8411
> https://issues.apache.org/jira/browse/MESOS-8411
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the v1 executor is shutdown if all of its
> initial tasks could not be delivered when re-subscribing with
> a recovered agent. See MESOS-8411.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp a35c68e8645384f6244d17e37cad71373aba6893 
>   src/tests/mesos.cpp d751b2e9c635eb6a5039678de426467176cda908 
>   src/tests/mock_slave.cpp 8357edc7b3a35624c813eccb9ecca9d3b5dbe07c 
>   src/tests/slave_tests.cpp f393a8433a984267adc4db307ef07fcbafd1e62f 
> 
> 
> Diff: https://reviews.apache.org/r/65497/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65522: Fixed some log messages in `slave.cpp`.

[Gaston Kleiman]

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


Fix it, then Ship it!




I noticed a minor inconsistency in `taskOrTaskGroup` that we might want to 
address in a different patch.


src/slave/slave.cpp
Line 2091 (original), 2091 (patched)


nit: s/was killed partially/was partially killed/ here and below.



src/slave/slave.cpp
Line 9530 (original), 9529 (patched)


I noticed that if a single task is passed, we surround its ID in single 
quotes, but we don't do that if a task groups is passed.

We might want to do this consistently.


- Gaston Kleiman


On Feb. 5, 2018, 8:17 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65522/
> ---
> 
> (Updated Feb. 5, 2018, 8:17 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed some log messages in `slave.cpp`.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 
> 
> 
> Diff: https://reviews.apache.org/r/65522/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 63368: Added MemoryProfiler class to Libprocess.

[Alexander Rukletsov]

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




3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 57 (patched)


When describing what a function does or returns, we use 3rd person present 
simple tense: activates, returns, generates,... Please adjust here and 
everywhere.



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 87 (patched)


Why? What is this id about?

Also please mention that `id` is optional and the last know will be 
returned if `id` is omitted.



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 115 (patched)


Period at the end.



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 132-133 (patched)


FYI: Fits one line. However, the comment seems incorrect: this function 
does not return any path, rather, it is a side effect on success.



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 148 (patched)


If you have private fields and getters, make it a class.



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 157 (patched)


backtick type and variable names and put a period at the end.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 75 (patched)


I'd kill these blank lines.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 197-240 (patched)


I would still merge these two functions. Since it is for internal use only, 
it is fine to expect the user to understand when to expect a previous value 
(looks like there is one such instance in the current code).

How about this?
```
template
Result writeJemallocSetting(const char* name, const T& value, bool 
requestPrevious)
{
  if (!detectJemalloc()) {
return Error(JEMALLOC_NOT_DETECTED_MESSAGE);
  }

  T previous;
  T* pprevious = requestPrevious ?  : nullptr;
  size_t size = sizeof(previous);
  size_t* pszie = requestPrevious ?  : nullptr;
  
  int error = mallctl(
  name, pprevious, psize, const_cast(), sizeof(value));

  if (error) {
return Error(strings::format(
"Couldn't write value %s for option %s: %s",
value, name, ::strerror(error)).get());
  }

  return requestPrevious ? previous : None();
}
```

And if you make `value` optional, you can have a single function plus maybe 
three syntactic sugar functions with descriptive names that call into the basic 
one.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 243-249 (patched)


I think it is safe to use this function because it is only accessed from 
`MemoryProfiler::DiskArtifact::path()` which is in turn only accessed from 
`MemoryProfiler` methods, which are always serialized. You should call this in 
a comment or even employ `process::Once` now to make sure it is thread-safe and 
hence future-proof.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 264 (patched)


Say "using std::string" in the beginning of the file and save hundreds of 
characters!



3rdparty/libprocess/src/memory_profiler.cpp
Lines 301 (patched)


I'm not sure newlines in `Error` is good idea. You can easily do without.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 311 (patched)


I'd avoid defaults in general. Given the ordering error below, I'd probably 
reorder the arguments and remove the default here.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 342 (patched)


No periods in error messages, please!



3rdparty/libprocess/src/memory_profiler.cpp
Lines 381-382 (patched)


Instead of trailing spaces, use leading spaces. This way it is easier to 
spot when a space is missing. Here and below.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 393-395 (patched)


Looks like you would need a multiline message here. Use commas to separate 
lines. Ditto for other longer help messages you have below.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 437-470 (patched)


Aint't it nice and tidy now?

Re: Review Request 63368: Added MemoryProfiler class to Libprocess.

[Benno Evers]

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

(Updated Feb. 6, 2018, 5:45 p.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Changes
---

Moved changes to CMakeLists into this commit.


Repository: mesos


Description
---

This class exposes profiling functionality of jemalloc memory allocator
when it is detected to be the memory allocator of the current process.

In particular, it gives developers an easy method to collect and
access heap profiles which report which pieces of code were
responsible for allocating memory.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 3071b7ce2b82a4ce0ea62e4d2b3518a6f5114803 
  3rdparty/libprocess/include/process/memory_profiler.hpp PRE-CREATION 
  3rdparty/libprocess/include/process/process.hpp 
8661706cb058efb26f5bfbcc84972f9930d3670f 
  3rdparty/libprocess/src/CMakeLists.txt 
f002c157dc2ca64da66bc4e61f5095f2b533ae1f 
  3rdparty/libprocess/src/memory_profiler.cpp PRE-CREATION 
  3rdparty/libprocess/src/process.cpp ba9bc291bb6741e32b3a066fe90771311d21852a 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 63366: Add jemalloc release tarball and build rules.

[Benno Evers]

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

(Updated Feb. 6, 2018, 5:45 p.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Changes
---

Removed changes to CMakeLists in libprocess.


Summary (updated)
-

Add jemalloc release tarball and build rules.


Repository: mesos


Description (updated)
---

Add jemalloc release tarball and build rules.


Diffs (updated)
-

  3rdparty/CMakeLists.txt da605707b89bbe9b3db9e60bc0b0a26dac46e56e 
  3rdparty/Makefile.am f4cb731198e271b967b920235ed2785e9296f9e1 
  3rdparty/cmake/Versions.cmake 94b0d8048412e00e2480f45e7ce4e6494da4bd5d 
  3rdparty/jemalloc-5.0.1.tar.bz2 PRE-CREATION 
  3rdparty/versions.am 3e008d5a7bdc029de23950463c295b968b3280c8 
  cmake/CompilationConfigure.cmake 50cddf9476c8c5196c4824a7b060c2680a96b277 
  configure.ac 30fbadc32d1d96f719d45fa8067f975283c25507 
  src/Makefile.am e444b8981093ba6f4e94acb7a060b998f75b542a 
  src/master/CMakeLists.txt ec552110509d17628107f4ad98f2d04675ed3ce3 
  src/slave/CMakeLists.txt 943e8f523f867e3dd4030f78eca7830500578faf 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 65109: Fixed a bug relating to lingering executors [1/2].

[Meng Zhu]

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

(Updated Feb. 6, 2018, 9:45 a.m.)


Review request for mesos, Benjamin Mahler and Vinod Kone.


Changes
---

Patch summary updated to refect that it is the first half of a fix.


Summary (updated)
-

Fixed a bug relating to lingering executors [1/2].


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


Repository: mesos


Description
---

An executor should be shutdown if all of its tasks are
killed while the executor is launching.

This patch fixes and issue where the executor is left
running when the task(s) get killed between the executor
registration/subscription and `Slave::___run()`. See
MESOS-8411 for more details. There is an additional race
in the agent failover case that is addressed in this patch.

The fix here is to fix the race by checking an executor's various
tasks queues during task kill and executor (re-)registration,
and shutting down executors that had never received any tasks.


Diffs (updated)
-

  src/slave/constants.hpp e6cb7cc0ccdaaf981eb66defa21b38720f4e1de9 
  src/slave/slave.hpp 09c01ebd1b5e8008ba9e7d412042f1db76a2c5a5 
  src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 


Diff: https://reviews.apache.org/r/65109/diff/7/

Changes: https://reviews.apache.org/r/65109/diff/6-7/


Testing
---

make check
new tests in #65111


Thanks,

Meng Zhu

Re: Review Request 65496: Fixed a bug relating to lingering executors [2/2].

[Meng Zhu]

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

(Updated Feb. 6, 2018, 9:42 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Patch summary updated to reflect it is the second half of a fix.


Summary (updated)
-

Fixed a bug relating to lingering executors [2/2].


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


Repository: mesos


Description
---

A task-less v1 executor could linger if the agent fails before
any of the executor's initial tasks got delivered.

This patch fixes this issue by checking if an executor is
task-less during the executor `subscribe()` and shutdown
the executor accordingly.


Diffs (updated)
-

  src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 


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

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


Testing
---

make check.
Dedicated test #65497


Thanks,

Meng Zhu

Re: Review Request 65461: Provide new overload of 'ProcessBase::route()'.

[Benno Evers]

> On Feb. 3, 2018, 12:06 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/process.hpp
> > Lines 325-352 (original), 325-352 (patched)
> > 
> >
> > These look obviated by your new one? Why do we still need these?

The overload taking an `AuthenticatedHttpRequestHandler` is still required when 
we pass a lambda to `route()`, e.g. from `master.cpp`. I've removed the other 
one.


- Benno


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


On Feb. 6, 2018, 5:40 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65461/
> ---
> 
> (Updated Feb. 6, 2018, 5:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Provide new overload of 'ProcessBase::route()'.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/logging.hpp 
> f9997677d69d54f5723d4fc0a495008d3ce11cc5 
>   3rdparty/libprocess/include/process/process.hpp 
> 8661706cb058efb26f5bfbcc84972f9930d3670f 
>   3rdparty/libprocess/include/process/profiler.hpp 
> 2991dd2033d68802a813de91babb47679c807aa0 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> f79541853b4c826014ee969633345c3d51520ecf 
> 
> 
> Diff: https://reviews.apache.org/r/65461/diff/2/
> 
> 
> Testing
> ---
> 
> It is a common pattern in libprocess to have a single function serving as 
> endpoint that handles both authenticated and non-authenticated requests, 
> providing `None()` as a default parameter in non-authenticated contexts.
> 
> This patch adds an overload to `ProcessBase::route()` implementing that so 
> that it does not need to be re-implemented in each individual process.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 65461: Provide new overload of 'ProcessBase::route()'.

[Benno Evers]

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

(Updated Feb. 6, 2018, 5:40 p.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Repository: mesos


Description
---

Provide new overload of 'ProcessBase::route()'.


Diffs (updated)
-

  3rdparty/libprocess/include/process/logging.hpp 
f9997677d69d54f5723d4fc0a495008d3ce11cc5 
  3rdparty/libprocess/include/process/process.hpp 
8661706cb058efb26f5bfbcc84972f9930d3670f 
  3rdparty/libprocess/include/process/profiler.hpp 
2991dd2033d68802a813de91babb47679c807aa0 
  3rdparty/libprocess/src/metrics/metrics.cpp 
f79541853b4c826014ee969633345c3d51520ecf 


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

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


Testing
---

It is a common pattern in libprocess to have a single function serving as 
endpoint that handles both authenticated and non-authenticated requests, 
providing `None()` as a default parameter in non-authenticated contexts.

This patch adds an overload to `ProcessBase::route()` implementing that so that 
it does not need to be re-implemented in each individual process.


Thanks,

Benno Evers

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65518 was successfully built and tested.

Reviews applied: `['65518']`

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

- Mesos Reviewbot Windows


On Feb. 6, 2018, 1:45 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 6, 2018, 1:45 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-8488
> https://issues.apache.org/jira/browse/MESOS-8488
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to a Docker issue (https://github.com/moby/moby/issues/33820),
> Docker daemon will fail to catch a container exit, i.e., the container
> process has already exited but the command `docker ps` shows the
> container still running, this will lead to the "docker run" command
> that we execute in Docker executor never returns, and it will also
> cause the `docker stop` command takes no effect, i.e., it will return
> without error but `docker ps` shows the container still running, so
> the task will stuck in `TASK_KILLING` state.
> 
> To workaround this Docker issue, in this patch we made Docker executor
> reaps the container process directly so Docker executor will be notified
> once the container process exits.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
> 
> 
> Diff: https://reviews.apache.org/r/65518/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65462: Use the new 'route()' overload from libprocess.

[Benno Evers]

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

(Updated Feb. 6, 2018, 2:11 p.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Changes
---

Limit description to 72 chars per line.


Repository: mesos


Description (updated)
---

Make use of the new `ProcessBase::route()` overload introduced in the
previous commit to reduce the amount of duplicated logic.


Diffs
-

  src/files/files.cpp 324b4bcf0ef53931fa7ef9b7a891b160564705ff 
  src/master/registrar.cpp aabce22e4b06826e4452c61c27ebdd702b1e47bc 


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


Testing
---


Thanks,

Benno Evers

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Qian Zhang]

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

(Updated Feb. 6, 2018, 9:45 p.m.)


Review request for mesos, Greg Mann and Vinod Kone.


Changes
---

Replaced `onAny` with `then`.


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


Repository: mesos


Description
---

Due to a Docker issue (https://github.com/moby/moby/issues/33820),
Docker daemon will fail to catch a container exit, i.e., the container
process has already exited but the command `docker ps` shows the
container still running, this will lead to the "docker run" command
that we execute in Docker executor never returns, and it will also
cause the `docker stop` command takes no effect, i.e., it will return
without error but `docker ps` shows the container still running, so
the task will stuck in `TASK_KILLING` state.

To workaround this Docker issue, in this patch we made Docker executor
reaps the container process directly so Docker executor will be notified
once the container process exits.


Diffs (updated)
-

  src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 


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

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


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 65529: Added --clean flag to bootstrap script for Mesos CLI.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65529 was successfully built and tested.

Reviews applied: `['65529']`

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

- Mesos Reviewbot Windows


On Feb. 6, 2018, 11:11 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65529/
> ---
> 
> (Updated Feb. 6, 2018, 11:11 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-8240
> https://issues.apache.org/jira/browse/MESOS-8240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Without it, the bootstrap script compares the modification date
> of the virtual environment compared to the pip-requirements
> and bootstrap script. If the virtualenv is newer, the bootstrap
> script will stop instead of building the environment again.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
> 
> 
> Diff: https://reviews.apache.org/r/65529/diff/1/
> 
> 
> Testing
> ---
> 
> Run bootstrap script three times. With no `.virtualenv`:
> ```
> $ ./bootstrap
> ```
> Creates the virtual environment.
> ```
> $ ./bootstrap
> The virtual environment is more recent than its dependencies, no need
> to rebuild it. Use 'bootstrap --clean' to rebuild the virtualenv
> without comparing its last modification date with its dependencies.
> $ ./bootstrap --clean
> ```
> The virtual environment will then be rebuilt again with this third command.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 65044: Added the v1 API 'GET_OPERATIONS' call for master and agent.

[Benjamin Bannier]

> On Feb. 1, 2018, 7:46 a.m., Greg Mann wrote:
> > src/tests/api_tests.cpp
> > Lines 6349-6352 (patched)
> > 
> >
> > Are these necessary? I think that framework registration may be 
> > sufficient to produce an offer?
> 
> Jan Schlicht wrote:
> `settle` isn't necessary, but `advance` is, see the similar comment in 
> #65045.

@greggomann: To me a `Clock::advance` without following `Clock::settle` looks 
unusual and potentially suspicious. I'd rather see a `settle` turning into a 
following `AWAIT_READY` into just an `ASSERT_READY` than having to follow the 
code to make sure it is safe.

Can we agree on a pattern of potentially redundant `settle` after `advance` 
here and elsewhere?


- Benjamin


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


On Feb. 6, 2018, 11:10 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65044/
> ---
> 
> (Updated Feb. 6, 2018, 11:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, Jie 
> Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-8424
> https://issues.apache.org/jira/browse/MESOS-8424
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The 'GET_OPERATIONS' call lists all operations known to master or agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 315820041990d28e5cdae71c8385d4551d56558c 
>   include/mesos/master/master.proto 3e34634f3b864222d07374936c0d9a18269c2fbd 
>   include/mesos/v1/agent/agent.proto 9e8b49defb83ffd4dd240ebb3a09b2dd3610ab2a 
>   include/mesos/v1/master/master.proto 
> 6759c3004e7e04c0044360b0c54f4438c2f6ec8a 
>   src/master/http.cpp c489b6f525f157811549b2cc84addd9d85e87990 
>   src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2 
>   src/master/validation.cpp f0b86775b7919ba6aa4a73038edb78a0adca68f4 
>   src/slave/http.hpp 1619bb77e3e7d5a6d387e4d4bb071d83ce914967 
>   src/slave/http.cpp 77e711ceeb0e2613d629b5e21fd686f85dfad11a 
>   src/slave/validation.cpp 0c2ccda177734cf3c47c0346ed34d20d58e7d932 
>   src/tests/api_tests.cpp b639a4bafbf69d3ea53aa0573e9f0489c3a1a57f 
> 
> 
> Diff: https://reviews.apache.org/r/65044/diff/10/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 65529: Added --clean flag to bootstrap script for Mesos CLI.

[Armand Grillet]

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

Review request for mesos and Kevin Klues.


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


Repository: mesos


Description
---

Without it, the bootstrap script compares the modification date
of the virtual environment compared to the pip-requirements
and bootstrap script. If the virtualenv is newer, the bootstrap
script will stop instead of building the environment again.


Diffs
-

  src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 


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


Testing
---

Run bootstrap script three times. With no `.virtualenv`:
```
$ ./bootstrap
```
Creates the virtual environment.
```
$ ./bootstrap
The virtual environment is more recent than its dependencies, no need
to rebuild it. Use 'bootstrap --clean' to rebuild the virtualenv
without comparing its last modification date with its dependencies.
$ ./bootstrap --clean
```
The virtual environment will then be rebuilt again with this third command.


Thanks,

Armand Grillet

Re: Review Request 65045: Tested correct operation handling during master failover.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65045 was successfully built and tested.

Reviews applied: `['65044', '65045']`

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

- Mesos Reviewbot Windows


On Feb. 6, 2018, 11:11 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65045/
> ---
> 
> (Updated Feb. 6, 2018, 11:11 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8424
> https://issues.apache.org/jira/browse/MESOS-8424
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tested correct operation handling during master failover.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 7112bb4efcc312dc4f68bdc44fb685c7624002b1 
> 
> 
> Diff: https://reviews.apache.org/r/65045/diff/7/
> 
> 
> Testing
> ---
> 
> make check
> 
> This test may trigger MESOS-8524, we might want to commit this as a disabled 
> test.
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 65202: Adopted `DEFAULT_TEST_TIMEOUT` in Mesos tests.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/tests/flags.hpp
Lines 49 (patched)


Let's rename this to `test_await_timeout`.



src/tests/flags.cpp
Lines 170 (patched)


`test_await_timeout`.



src/tests/main.cpp
Lines 21 (patched)


If we move the test await timeout to a file of constants we would include 
its header file here.

Here and below.


- Benjamin Bannier


On Feb. 5, 2018, 11:50 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65202/
> ---
> 
> (Updated Feb. 5, 2018, 11:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gaston Kleiman.
> 
> 
> Bugs: MESOS-7016
> https://issues.apache.org/jira/browse/MESOS-7016
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than hard-coding a 15sec test timeout, use the libprocess
> `DEFAULT_TEST_TIMEOUT` variable so that this can be globally tuned.
> Added a test flag `--default_await_timeout` that can be used to set
> the `DEFAULT_TEST_TIMEOUT` on a test run.
> 
> 
> Diffs
> -
> 
>   src/tests/agent_container_api_tests.cpp 
> 618569277545205017320aaf1f3a70e540d35e30 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 5412a7eccaa836a7f182772f1eb33b10298c96a1 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> d761ea46fd4f12c6bed74090bf5ead7587f16811 
>   src/tests/disk_quota_tests.cpp 31cf4c44b1494afd3501046abffd303c7a1307f8 
>   src/tests/fetcher_cache_tests.cpp 7db7b7dcef27b1686ccae5a7408ff2811a3b9255 
>   src/tests/flags.hpp df4d4588f405ea884bb89021264e70218e054fae 
>   src/tests/flags.cpp 4542f3443830474edd97e8b695af0c61685e947c 
>   src/tests/gc_tests.cpp 0e34c017f2c629b527915218f517b88d8392aa4b 
>   src/tests/main.cpp f945ac9e8858998174b4c2785f6a5612bae66a41 
>   src/tests/master_contender_detector_tests.cpp 
> 015d310b72a152829641d436f7cb558f72dfd77c 
>   src/tests/registrar_tests.cpp cbb58c1cd72dc13ed16b5465452d17526255c763 
>   src/tests/slave_recovery_tests.cpp 77aa60c953bd0769eaba05f001755e4cec9ba028 
>   src/tests/slave_tests.cpp f393a8433a984267adc4db307ef07fcbafd1e62f 
> 
> 
> Diff: https://reviews.apache.org/r/65202/diff/8/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 65343: Adopted `DEFAULT_TEST_TIMEOUT` in libprocess tests.

[Benjamin Bannier]

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


Fix it, then Ship it!





3rdparty/libprocess/src/tests/main.cpp
Lines 63 (patched)


Let's rename this to `test_await_timeout`.



3rdparty/libprocess/src/tests/main.cpp
Lines 68 (patched)


Rename to `test_await_timeout`.


- Benjamin Bannier


On Jan. 25, 2018, 8:15 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65343/
> ---
> 
> (Updated Jan. 25, 2018, 8:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gaston Kleiman.
> 
> 
> Bugs: MESOS-7016
> https://issues.apache.org/jira/browse/MESOS-7016
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than hard-coding a 15sec test timeout, use the libprocess
> `DEFAULT_TEST_TIMEOUT` variable so that this can be globally tuned.
> Added a test flag `--default_await_timeout` that can be used to set
> the `DEFAULT_TEST_TIMEOUT` on a test run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/main.cpp 
> 4633bea4caad3ddda809b7b5aa0c5220f8ad82c2 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> 8211ae63e75359705af2bb9ac02a7cbdb28e3252 
> 
> 
> Diff: https://reviews.apache.org/r/65343/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 65201: Added a global `DEFAULT_TEST_TIMEOUT` variable.

[Benjamin Bannier]

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


Fix it, then Ship it!





3rdparty/libprocess/Makefile.am
Lines 200 (patched)


If we move the constants to a separate source file we could include on that 
one here.



3rdparty/libprocess/include/process/gtest.hpp
Lines 37 (patched)


I'd suggest to move this to an extra header file, e.g., 
`process/tests/constants.hpp`. We could then include that header file here.

Let's also rename the constant to `TEST_AWAIT_TIMEOUT` as `DEFAULT` is 
inconsequential.



3rdparty/libprocess/src/gtest.cpp
Lines 1-23 (patched)


Let's move this to its own file, e.g., `process/tests/constants.cpp`. This 
implementation file could always include its header file without issues.


- Benjamin Bannier


On Jan. 23, 2018, 10:57 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65201/
> ---
> 
> (Updated Jan. 23, 2018, 10:57 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gaston Kleiman.
> 
> 
> Bugs: MESOS-7016
> https://issues.apache.org/jira/browse/MESOS-7016
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a global `DEFAULT_TEST_TIMEOUT` variable that applications
> can use to globally tune the default timeout in the `AWAIT_READY`
> family of test macros. The default remains 15sec.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 3071b7ce2b82a4ce0ea62e4d2b3518a6f5114803 
>   3rdparty/libprocess/include/process/gtest.hpp 
> eee726653d52af2e4a148819e420ebd22e5123a9 
>   3rdparty/libprocess/src/CMakeLists.txt 
> f002c157dc2ca64da66bc4e61f5095f2b533ae1f 
>   3rdparty/libprocess/src/gtest.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65201/diff/7/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 65298: Split test flags into header and source files.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/tests/flags.hpp
Line 25 (original), 24 (patched)


This is not needed.



src/tests/flags.cpp
Line 17 (original), 28 (patched)


This one needs to be included first.


- Benjamin Bannier


On Jan. 25, 2018, 8:29 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65298/
> ---
> 
> (Updated Jan. 25, 2018, 8:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gaston Kleiman.
> 
> 
> Bugs: MESOS-7016
> https://issues.apache.org/jira/browse/MESOS-7016
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Split the test flags into header and source files to reduce the amount
> of recompilation necessary when the implementation changes.
> 
> 
> Diffs
> -
> 
>   src/tests/flags.hpp df4d4588f405ea884bb89021264e70218e054fae 
>   src/tests/flags.cpp 4542f3443830474edd97e8b695af0c61685e947c 
> 
> 
> Diff: https://reviews.apache.org/r/65298/diff/6/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 65045: Tested correct operation handling during master failover.

[Jan Schlicht]

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

(Updated Feb. 6, 2018, 11:11 a.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Re-added authentication flags (which is important for builds with enabled SSL)


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


Repository: mesos


Description
---

Tested correct operation handling during master failover.


Diffs (updated)
-

  src/tests/master_tests.cpp 7112bb4efcc312dc4f68bdc44fb685c7624002b1 


Diff: https://reviews.apache.org/r/65045/diff/7/

Changes: https://reviews.apache.org/r/65045/diff/6-7/


Testing
---

make check

This test may trigger MESOS-8524, we might want to commit this as a disabled 
test.


Thanks,

Jan Schlicht

Re: Review Request 65044: Added the v1 API 'GET_OPERATIONS' call for master and agent.

[Jan Schlicht]

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

(Updated Feb. 6, 2018, 11:10 a.m.)


Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, Jie Yu, 
and Vinod Kone.


Changes
---

Re-added authentication flags.


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


Repository: mesos


Description
---

The 'GET_OPERATIONS' call lists all operations known to master or agent.


Diffs (updated)
-

  include/mesos/agent/agent.proto 315820041990d28e5cdae71c8385d4551d56558c 
  include/mesos/master/master.proto 3e34634f3b864222d07374936c0d9a18269c2fbd 
  include/mesos/v1/agent/agent.proto 9e8b49defb83ffd4dd240ebb3a09b2dd3610ab2a 
  include/mesos/v1/master/master.proto 6759c3004e7e04c0044360b0c54f4438c2f6ec8a 
  src/master/http.cpp c489b6f525f157811549b2cc84addd9d85e87990 
  src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2 
  src/master/validation.cpp f0b86775b7919ba6aa4a73038edb78a0adca68f4 
  src/slave/http.hpp 1619bb77e3e7d5a6d387e4d4bb071d83ce914967 
  src/slave/http.cpp 77e711ceeb0e2613d629b5e21fd686f85dfad11a 
  src/slave/validation.cpp 0c2ccda177734cf3c47c0346ed34d20d58e7d932 
  src/tests/api_tests.cpp b639a4bafbf69d3ea53aa0573e9f0489c3a1a57f 


Diff: https://reviews.apache.org/r/65044/diff/10/

Changes: https://reviews.apache.org/r/65044/diff/9-10/


Testing
---

make check


Thanks,

Jan Schlicht