Re: Review Request 69188: Fixed a wrong way to install callback for OOM notifier.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69188 was successfully built and tested.

Reviews applied: `['69123', '69188']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2536/mesos-review-69188

- Mesos Reviewbot Windows


On Oct. 26, 2018, 3:26 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69188/
> ---
> 
> (Updated Oct. 26, 2018, 3:26 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9334
> https://issues.apache.org/jira/browse/MESOS-9334
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a wrong way to install callback for OOM notifier.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
> f4ed8337253995250cb533fa0a232909cb1a824b 
> 
> 
> Diff: https://reviews.apache.org/r/69188/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 69123: Fixed an early fd close in the cgroups event notifier.

[Qian Zhang]

> On Oct. 25, 2018, 4:07 p.m., Qian Zhang wrote:
> > Not yours, I see in `MemorySubsystemProcess::oomListen` we have the these 
> > code:
> > ```
> >   info->oomNotifier.onReady(
> >   defer(PID(this),
> > ::oomWaited,
> > containerId,
> > cgroup,
> > lambda::_1));
> > ```
> > Here the `onReady` seems not correct to me, I think it should be `onAny`.
> 
> Benjamin Mahler wrote:
> Hm.. ok, this warrants a separate patch, do you want to send one?

Posted one: https://reviews.apache.org/r/69188/


- Qian


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


On Oct. 26, 2018, 11:25 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69123/
> ---
> 
> (Updated Oct. 26, 2018, 11:25 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-9334
> https://issues.apache.org/jira/browse/MESOS-9334
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The cgroups event notifier was closing the eventfd while an
> `io::read()` operation may be in progress. This can lead to
> bugs where the fd gets re-used and read from a stale io::read.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 95efc1def3318dd93d30d91e9c8a91376cc610b8 
> 
> 
> Diff: https://reviews.apache.org/r/69123/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 69188: Fixed a wrong way to install callback for OOM notifier.

[Qian Zhang]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Fixed a wrong way to install callback for OOM notifier.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
f4ed8337253995250cb533fa0a232909cb1a824b 


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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 69123: Fixed an early fd close in the cgroups event notifier.

[Qian Zhang]

> On Oct. 25, 2018, 4:07 p.m., Qian Zhang wrote:
> > src/linux/cgroups.cpp
> > Lines 1071-1080 (original), 1071-1089 (patched)
> > 
> >
> > I see we already have an onAny callback `_listen`, can we close the fd 
> > at the end of that method (i.e., when we are sure that we will not listen 
> > anymore)? And then we could change the code here like below:
> > ```
> > if (reading.isSome()) {
> >   reading->discard();
> > } else if (eventfd.isSome()) {
> >   Try unregister = unregisterNotifier(eventfd.get());
> >   if (unregister.isError()) {
> > LOG(ERROR) << "Failed to unregister eventfd: " << 
> > unregister.error();
> >   }
> > }
> > 
> > ```
> 
> Benjamin Mahler wrote:
> I originally tried to simplify the lifecycle of the listener, but it was 
> implemented this way because some callers call listen multiple times.
> 
> create listener
> listen
> listen
> listen
> terminate

Yeah, I know we support that. So that's why I suggested to close the fd 
(`unregisterNotifier(eventfd.get())`) at the end of `_listen`, and then even 
the caller call `listen` again, we will actually do nothing but just return 
`error` (since `error` has been set in `_listen`). I think if we are sure that 
we will not listen again (i.e., the first time when `_listen` is call with 
`error` set), we should close the fd immediately rather than closing it until 
`finalize` is called.


- Qian


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


On Oct. 23, 2018, 11:08 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69123/
> ---
> 
> (Updated Oct. 23, 2018, 11:08 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The cgroups event notifier was closing the eventfd while an
> `io::read()` operation may be in progress. This can lead to
> bugs where the fd gets re-used and read from a stale io::read.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 95efc1def3318dd93d30d91e9c8a91376cc610b8 
> 
> 
> Diff: https://reviews.apache.org/r/69123/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 69123: Fixed an early fd close in the cgroups event notifier.

[Benjamin Mahler]

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

(Updated Oct. 26, 2018, 11:25 a.m.)


Review request for mesos and Qian Zhang.


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


Repository: mesos


Description
---

The cgroups event notifier was closing the eventfd while an
`io::read()` operation may be in progress. This can lead to
bugs where the fd gets re-used and read from a stale io::read.


Diffs
-

  src/linux/cgroups.cpp 95efc1def3318dd93d30d91e9c8a91376cc610b8 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 69185: Added streaming header support for /api/v1 SUBSCRIBE.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['69180', '69181', '69182', '69183', '69184', '69185']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2535/mesos-review-69185

Relevant logs:

- 
[mesos-tests-cmake.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2535/mesos-review-69185/logs/mesos-tests-cmake.log):

```
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3501):
 warning C4996: 'sprintf': This function or variable may be unsafe. Consider 
using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3479):
 warning C4101: 'addrstr': unreferenced local variable 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\recordio.c(170):
 warning C4267: '=': conversion from 'size_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496):
 warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256):
 warning C4090: 'function': different 'const' qualifiers 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(512):
 warning 

Review Request 69184: Marked internal::recordio::Reader as non-copyable/assignable.

[Joseph Wu]

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

Review request for mesos, Gastón Kleiman and Greg Mann.


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


Repository: mesos


Description
---

This helper object is basically a wrapper around a libprocess actor.
In the current implementation, if the helper is destructed, the
underlying libprocess actor will be terminated as well.  This means
an errorneous copy of this helper object may result in a dead PID,
which is unguarded and may result in CHECK failures in some scenarios.


Diffs
-

  src/common/recordio.hpp 8cb2e73f16c753a85074fbf2ae9fd92ae0d2aeea 


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


Testing
---

make

See later in chain.


Thanks,

Joseph Wu

Review Request 69185: Added streaming header support for /api/v1 SUBSCRIBE.

[Joseph Wu]

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

Review request for mesos, Gastón Kleiman and Greg Mann.


Bugs: MESOS-7974 and MESOS-9258
https://issues.apache.org/jira/browse/MESOS-7974
https://issues.apache.org/jira/browse/MESOS-9258


Repository: mesos


Description
---

The existing API allowed the client to send 'Accept' and 'Content-Type'
headers of 'application/json' or 'application/protobuf'.

The SUBSCRIBE endpoint would also take a 'Content-Type' of those two
types, but always return content with 'application/recordio' (because
this is a streaming response).  This maintains that behavior while
also allowing the user to use streaming headers correctly.


Diffs
-

  src/master/http.cpp 75ab6ea422a77e25049cd6afe007507d11650a06 
  src/master/master.hpp ea7e9242b62fe6c2cc0e717f9a9f2f0c1cc0a390 


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


Testing
---

make

See later in chain.


Thanks,

Joseph Wu

Review Request 69183: Added optional heartbeat monitoring to the master /api/v1.

[Joseph Wu]

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

Review request for mesos, Gastón Kleiman and Greg Mann.


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


Repository: mesos


Description
---

When a client of the master SUBSCRIBE /api/v1 makes a streaming request,
this logic will check that the client sends a heartbeat at least
once every 30 seconds (2x the default heartbeat interval).  If the
heartbeat times out, the connection breaks, or a malformed message is
sent as a heartbeat, the master will actively break the connection.


Diffs
-

  src/master/master.hpp ea7e9242b62fe6c2cc0e717f9a9f2f0c1cc0a390 
  src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a 


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


Testing
---

make

See later in chain.


Thanks,

Joseph Wu

Review Request 69182: Moved Master::Subscribers::Subscriber constructor into .cpp file.

[Joseph Wu]

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

Review request for mesos, Gastón Kleiman and Greg Mann.


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


Repository: mesos


Description
---

The logic inside this class's constructor was originally in the master's
header file.  With the introduction of streaming request support (to be
added in subsequent commit(s)), the logic inside the constructor will be
increased; and as such would be better placed outside the header file.

This could potentially be squashed with:
https://reviews.apache.org/r/69181/


Diffs
-

  src/master/master.hpp ea7e9242b62fe6c2cc0e717f9a9f2f0c1cc0a390 
  src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a 


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


Testing
---

make

See later in chain.


Thanks,

Joseph Wu

Review Request 69181: Passed streaming decoder from master /api/v1 to master actor.

[Joseph Wu]

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

Review request for mesos, Gastón Kleiman and Greg Mann.


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


Repository: mesos


Description
---

This intermediate change passes the incoming streaming handle of a
master SUBSCRIBE request from the Http helpers into the
Master::Subscribers::Subscriber object.

The Subscriber object will be in charge of reading from the streaming
handle and acting on the incoming data.  This logic will be implemented
in a subsequent commit.


Diffs
-

  src/master/http.cpp 75ab6ea422a77e25049cd6afe007507d11650a06 
  src/master/master.hpp ea7e9242b62fe6c2cc0e717f9a9f2f0c1cc0a390 
  src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a 


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


Testing
---

make

See later in chain.


Thanks,

Joseph Wu

Review Request 69180: Enabled streaming requests on the master /api/v1 endpoint.

[Joseph Wu]

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

Review request for mesos, Gastón Kleiman and Greg Mann.


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


Repository: mesos


Description
---

This change allows clients of the /api/v1 to specify the HTTP header
'Content-Type: application/recordio' to the master's /api/v1 endpoint.
For now, the only master API that will support streaming requests
is the SUBSCRIBE call.  Other APIs will disallow streaming.

This change makes the `Master::Http::api()` function closely resemble
the `Slave::Http::api()` function.  The main difference is that the
incoming stream reader is stored as a shared_ptr rather than a
Owned object, which to prevent the stream reader from going out of
scope while being passed around via lambda captures.


Diffs
-

  src/master/http.cpp 75ab6ea422a77e25049cd6afe007507d11650a06 
  src/master/master.hpp ea7e9242b62fe6c2cc0e717f9a9f2f0c1cc0a390 
  src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a 


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


Testing
---

make

See later in chain.


Thanks,

Joseph Wu

Re: Review Request 69178: Fixed test `MasterTestPrePostReservationRefinement.LaunchGroup`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69178 was successfully built and tested.

Reviews applied: `['69178']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2533/mesos-review-69178

- Mesos Reviewbot Windows


On Oct. 25, 2018, 4:11 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69178/
> ---
> 
> (Updated Oct. 25, 2018, 4:11 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-9079
> https://issues.apache.org/jira/browse/MESOS-9079
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the missing scheduler acknowledgment calls so that the
> two expected task status updates can be delivered.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 1db8ed7d81acbcd8bad4b7ca77c501d1d99cc135 
> 
> 
> Diff: https://reviews.apache.org/r/69178/diff/1/
> 
> 
> Testing
> ---
> 
> run `MasterTestPrePostReservationRefinement.LaunchGroup` continously without 
> failure
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69176: Fixed flaky test `SchedulerHttpApiTest.UpdatePidToHttpScheduler`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69176 was successfully built and tested.

Reviews applied: `['69176']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2534/mesos-review-69176

- Mesos Reviewbot Windows


On Oct. 25, 2018, 11:11 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69176/
> ---
> 
> (Updated Oct. 25, 2018, 11:11 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-8343
> https://issues.apache.org/jira/browse/MESOS-8343
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test was flaky due to race between scheduler driver stopping
> during test teardown and the scheduler `error()` invocation.
> Adding the missing synchronization for the expectation should
> eliminate the race.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_http_api_tests.cpp 
> 64c000650acab30647d0c1e7d685bec954aa7118 
> 
> 
> Diff: https://reviews.apache.org/r/69176/diff/1/
> 
> 
> Testing
> ---
> 
> ran `chedulerHttpApiTest.UpdatePidToHttpScheduler` continously without 
> failure.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 69176: Fixed flaky test `SchedulerHttpApiTest.UpdatePidToHttpScheduler`.

[Meng Zhu]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

The test was flaky due to race between scheduler driver stopping
during test teardown and the scheduler `error()` invocation.
Adding the missing synchronization for the expectation should
eliminate the race.


Diffs
-

  src/tests/scheduler_http_api_tests.cpp 
64c000650acab30647d0c1e7d685bec954aa7118 


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


Testing
---

ran `chedulerHttpApiTest.UpdatePidToHttpScheduler` continously without failure.


Thanks,

Meng Zhu

Review Request 69178: Fixed test `MasterTestPrePostReservationRefinement.LaunchGroup`.

[Meng Zhu]

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

Review request for mesos and Benno Evers.


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


Repository: mesos


Description
---

Added the missing scheduler acknowledgment calls so that the
two expected task status updates can be delivered.


Diffs
-

  src/tests/master_tests.cpp 1db8ed7d81acbcd8bad4b7ca77c501d1d99cc135 


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


Testing
---

run `MasterTestPrePostReservationRefinement.LaunchGroup` continously without 
failure


Thanks,

Meng Zhu

Re: Review Request 69172: Added `FetcherCacheTest.LocalCachedMissing` test.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69172 was successfully built and tested.

Reviews applied: `['69171', '69172']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2531/mesos-review-69172

- Mesos Reviewbot Windows


On Oct. 25, 2018, 8:47 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69172/
> ---
> 
> (Updated Oct. 25, 2018, 8:47 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, James Peach, and Joseph Wu.
> 
> 
> Bugs: MESOS-7474
> https://issues.apache.org/jira/browse/MESOS-7474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the fetcher retries downloading URI when the
> cache file is missing.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 22a7c0f36fb070e51b0c02da27016e6b71c297dd 
> 
> 
> Diff: https://reviews.apache.org/r/69172/diff/1/
> 
> 
> Testing
> ---
> 
> 1. sudo make check (Fedora 25)
> 2. internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 69110: Added task health check definitions to master API responses.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69110 was successfully built and tested.

Reviews applied: `['69110']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2532/mesos-review-69110

- Mesos Reviewbot Windows


On Oct. 25, 2018, 9:01 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69110/
> ---
> 
> (Updated Oct. 25, 2018, 9:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Gastón Kleiman, and Vinod Kone.
> 
> 
> Bugs: MESOS-8780
> https://issues.apache.org/jira/browse/MESOS-8780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Task protobuf message is updated to include the health check
> definition of a task when it is specified. Associated helpers are
> also updated along with a test which verifies that this field is
> reflected in master API responses.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 72966da75bc90d4b5d891e209e786e5326155d17 
>   include/mesos/v1/mesos.proto c6c1dae5494d9270a7a43a2d838769574ae47872 
>   src/common/http.cpp 80848aafe27eb0c35222ad236e671a32c4ab10aa 
>   src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3 
>   src/tests/api_tests.cpp 97fd0bb891235302beda37f2eb8418677317df93 
>   src/tests/health_check_tests.cpp c972b9003196b4b4272d7c34311c84c117b1ae3d 
> 
> 
> Diff: https://reviews.apache.org/r/69110/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 69146: Made fetcher tests more robust by using the test sandbox.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69146 was successfully built and tested.

Reviews applied: `['69146']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2530/mesos-review-69146

- Mesos Reviewbot Windows


On Oct. 25, 2018, 7:34 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> ---
> 
> (Updated Oct. 25, 2018, 7:34 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch makes `FetcherTest` a subclass of
> `MesosTest` which has a `createSlaveFlags()` helper to
> utilize the test sandboxes for related directories to
> avoid interference. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a 
>   src/tests/mesos.cpp c0ab2f7d86c4059f08e730d29456e6cc84c85995 
> 
> 
> Diff: https://reviews.apache.org/r/69146/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69110: Added task health check definitions to master API responses.

[Greg Mann]

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

(Updated Oct. 25, 2018, 9:01 p.m.)


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


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


Repository: mesos


Description
---

The Task protobuf message is updated to include the health check
definition of a task when it is specified. Associated helpers are
also updated along with a test which verifies that this field is
reflected in master API responses.


Diffs (updated)
-

  include/mesos/mesos.proto 72966da75bc90d4b5d891e209e786e5326155d17 
  include/mesos/v1/mesos.proto c6c1dae5494d9270a7a43a2d838769574ae47872 
  src/common/http.cpp 80848aafe27eb0c35222ad236e671a32c4ab10aa 
  src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3 
  src/tests/api_tests.cpp 97fd0bb891235302beda37f2eb8418677317df93 
  src/tests/health_check_tests.cpp c972b9003196b4b4272d7c34311c84c117b1ae3d 


Diff: https://reviews.apache.org/r/69110/diff/5/

Changes: https://reviews.apache.org/r/69110/diff/4-5/


Testing
---

`make check`


Thanks,

Greg Mann

Review Request 69172: Added `FetcherCacheTest.LocalCachedMissing` test.

[Andrei Budnik]

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

Review request for mesos, Gilbert Song, James Peach, and Joseph Wu.


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


Repository: mesos


Description
---

This test verifies that the fetcher retries downloading URI when the
cache file is missing.


Diffs
-

  src/tests/fetcher_cache_tests.cpp 22a7c0f36fb070e51b0c02da27016e6b71c297dd 


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


Testing
---

1. sudo make check (Fedora 25)
2. internal CI


Thanks,

Andrei Budnik

Review Request 69171: Added validation of cache files to the URI Fetcher.

[Andrei Budnik]

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

Review request for mesos, Gilbert Song, James Peach, and Joseph Wu.


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


Repository: mesos


Description
---

Previously, missing cache files could lead to task launch failures.
This patch introduces validation of cache files which happens each
time a downloaded cache entry is requested via the `get()` method.
If validation fails, `get()` returns `None()`, so that the fetcher
retries downloading URI. Currently, we only check the existence of
the cache file during validation.


Diffs
-

  src/slave/containerizer/fetcher.cpp e848c86261b75e5549e80276541e5932162fc012 
  src/slave/containerizer/fetcher_process.hpp 
56ed6e5455b7d23b4ce84873fe6734b4213df691 


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


Testing
---

1. sudo make check (Fedora 25)
2. internal CI


Thanks,

Andrei Budnik

Re: Review Request 69123: Fixed an early fd close in the cgroups event notifier.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [69123]

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 Oct. 23, 2018, 3:08 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69123/
> ---
> 
> (Updated Oct. 23, 2018, 3:08 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The cgroups event notifier was closing the eventfd while an
> `io::read()` operation may be in progress. This can lead to
> bugs where the fd gets re-used and read from a stale io::read.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 95efc1def3318dd93d30d91e9c8a91376cc610b8 
> 
> 
> Diff: https://reviews.apache.org/r/69123/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 69146: Made fetcher tests more robust by using the test sandbox.

[Meng Zhu]

> On Oct. 24, 2018, 11:54 p.m., Benjamin Bannier wrote:
> > src/tests/fetcher_tests.cpp
> > Lines 98-109 (patched)
> > 
> >
> > We can avoid duplication here by making `FetcherTest` a `MesosTest` 
> > (that one would already today take care of `launcher_dir` and 
> > `fetcher_cache_dir`). This function would than at most need to invoke 
> > `MesosTest::CreateSlaveFlags` and make its non-general additions.
> > 
> > It probably even make sense to set `frameworks_home` in 
> > `CreateSlaveFlags` as well. That would remove the need for this function 
> > completely.

Sounds good.


- Meng


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


On Oct. 25, 2018, 12:34 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> ---
> 
> (Updated Oct. 25, 2018, 12:34 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch makes `FetcherTest` a subclass of
> `MesosTest` which has a `createSlaveFlags()` helper to
> utilize the test sandboxes for related directories to
> avoid interference. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a 
>   src/tests/mesos.cpp c0ab2f7d86c4059f08e730d29456e6cc84c85995 
> 
> 
> Diff: https://reviews.apache.org/r/69146/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69146: Made fetcher tests more robust by using the test sandbox.

[Meng Zhu]

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

(Updated Oct. 25, 2018, 12:34 p.m.)


Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and 
Till Toenshoff.


Changes
---

Addressed review comments.


Repository: mesos


Description (updated)
---

Fetcher tests currently rely on some hard-coded paths,
for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
Thus fetcher tests could fail if these directories already
exit. This patch makes `FetcherTest` a subclass of
`MesosTest` which has a `createSlaveFlags()` helper to
utilize the test sandboxes for related directories to
avoid interference. The above paths will be
replaced by `fetch/` and `frameworks/` under the sandbox.


Diffs (updated)
-

  src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a 
  src/tests/mesos.cpp c0ab2f7d86c4059f08e730d29456e6cc84c85995 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 69146: Made fetcher tests more robust by using the test sandbox.

[Meng Zhu]

> On Oct. 24, 2018, 4:31 p.m., Till Toenshoff wrote:
> > src/tests/fetcher_tests.cpp
> > Lines 105-106 (patched)
> > 
> >
> > While this solution appears to be the most efficient it also is not 
> > entirely consistent with how we do it in other places; 
> > Note that `getcwd()` returns the test-sandbox directory after the 
> > fixture did its job.
> > ```
> > path::join(os::getcwd(), "fetcher");
> > path::join(os::getcwd(), "frameworks");
> > ```
> > would be consistent.
> > 
> > Alternatively, we could at least do away with one `CHECK_NOTNONE` and 
> > pull it in front of these two uses of `sandbox`.
> 
> Joseph Wu wrote:
> A good chunk of tests have the following pattern:
> ```
> path::join(sandbox.get(), ...);
> ```
> 
> I've been leaning towards this because it forgoes the syscall.
> 
> Till Toenshoff wrote:
> Definitely more efficient, yes - wish we could "fix" that in one swoop - 
> but am not attached to the first part of my comment due to these reasons.
> 
> Meng Zhu wrote:
> For the second part, I think using two `CHECK_NOTNONE` is more readable 
> to me. Otherwise, we will end up with two unguarded `get()` (I am a little 
> bit allergic to unguarded `get()` :) )
> 
> Till Toenshoff wrote:
> How about 
> ```
> CHECK_NOTNONE(sandbox);
> flags.fetcher_cache_dir = path::join(sandbox.get(), "fetcher");
> flags.frameworks_home = path::join(sandbox.get(), "frameworks");
> ```
> 
> Meng Zhu wrote:
> I am not sure why it is preferred over the current code. I view 
> `CHECK_NOTNONE` more as a safe "get()" operation rather than a pure check.
> 
> If we want to save one check, we can do
> 
> ```
> ASSERT_SOME(sandbox); 
> flags.fetcher_cache_dir = path::join(sandbox.get(), "fetcher");
> flags.frameworks_home = path::join(sandbox.get(), "frameworks");
> 
> ```
> 
> However, the reason I prefer inline `CHECK_NOTNONE` is that it removes 
> order dependencies between statements. Otherwise when we do `get()`, we need 
> to carry the mental burden that it has to be done after the `ASSERT`.
>  
> I feel inline `CHECK_NOTNONE` is both easy to maintain (no order 
> dependency) and read (clearly express the intent of "can't be none" inline).
> 
> I personally prefer to never use naked `get()` given `CHECK_NOTNONE`.
> 
> Chun-Hung Hsiao wrote:
> Note that `Option::get` already has an assertion. The purpose of 
> `CHECK_NOTNONE` to me is just to keep the line number so the check failure 
> can be more easily located.
> 
> In general I'd prefer avoiding the `CHECK*` macros in tests as it would 
> cause the test program to crash, thus no test cleanup would be performed. So 
> an `ASSERT_SOME` is "safer" in this sense. That said, this check do seems a 
> bit unnecessary because it's (practically) guaranteed that it is not none. 
> Also you cannot use `ASSERT_*` in a non-void function. You have to do things 
> like this: 
> https://github.com/apache/mesos/blob/master/src/tests/storage_local_resource_provider_tests.cpp#L299-L301
> And this limitation makes me incline to not do the assertion a bit.

Followed Benjamin's suggestion to use MesosTest which already has a 
`createSlaveFlag()` function. Using naked `get()` for consistency. Dropping 
this issue.

But it has been a good discussion. Unlike std assert, `CHECK_NOTNONE()` 
utilizes the glog interface, so the error log message will be more consistent, 
with timestamp and etc. In addition, I take `CHECK_NOTNONE` as an inline 
expression of intention -- the programmer assumes it cannot be NONE, not that 
she forgot to check.


- Meng


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


On Oct. 25, 2018, 12:34 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> ---
> 
> (Updated Oct. 25, 2018, 12:34 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch makes `FetcherTest` a subclass of
> `MesosTest` which has a `createSlaveFlags()` helper to
> utilize the test sandboxes for related directories to
> avoid interference. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 

Re: Review Request 69092: Added default arguments to `FrameworkProfile` in allocator benchmark.

[Meng Zhu]

> On Oct. 24, 2018, 5:18 p.m., Benjamin Mahler wrote:
> > Hm.. I don't understand what this is doing. Is this setting default 
> > framework behavior if someone writing a benchmark doesn't want to have to 
> > pass the arguments?

That is correct.


- Meng


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


On Oct. 19, 2018, 6:27 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69092/
> ---
> 
> (Updated Oct. 19, 2018, 6:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For frameworks that do not care about launching tasks, we should
> provide some default task launch settings to simplify the benchmark
> settings.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_benchmarks.cpp 
> bf9167b63747f7b8a402d950947028436307082a 
> 
> 
> Diff: https://reviews.apache.org/r/69092/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69163: Set agent and/or resource provider ID in operation status updates.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [68147, 69157, 69158, 69159, 69160, 69161, 69162, 69163]

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 Oct. 25, 2018, 3:54 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69163/
> ---
> 
> (Updated Oct. 25, 2018, 3:54 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, and James DeFelice.
> 
> 
> Bugs: MESOS-9293
> https://issues.apache.org/jira/browse/MESOS-9293
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
> 
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (no agent
> ID, not resource provider ID).
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba 
>   src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3 
>   src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a 
>   src/resource_provider/storage/provider.cpp 
> db783b53558811081fb2671e005e8bbbd9edbede 
>   src/slave/slave.cpp 7bb2b291a7b67563adf88bcedca1392890f17ffe 
>   src/tests/slave_tests.cpp 637bedce20f63dfc1702ef549bcea8f790477be3 
> 
> 
> Diff: https://reviews.apache.org/r/69163/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69123: Fixed an early fd close in the cgroups event notifier.

[Benjamin Mahler]

> On Oct. 25, 2018, 8:07 a.m., Qian Zhang wrote:
> > Not yours, I see in `MemorySubsystemProcess::oomListen` we have the these 
> > code:
> > ```
> >   info->oomNotifier.onReady(
> >   defer(PID(this),
> > ::oomWaited,
> > containerId,
> > cgroup,
> > lambda::_1));
> > ```
> > Here the `onReady` seems not correct to me, I think it should be `onAny`.

Hm.. ok, this warrants a separate patch, do you want to send one?


> On Oct. 25, 2018, 8:07 a.m., Qian Zhang wrote:
> > src/linux/cgroups.cpp
> > Lines 1071-1080 (original), 1071-1089 (patched)
> > 
> >
> > I see we already have an onAny callback `_listen`, can we close the fd 
> > at the end of that method (i.e., when we are sure that we will not listen 
> > anymore)? And then we could change the code here like below:
> > ```
> > if (reading.isSome()) {
> >   reading->discard();
> > } else if (eventfd.isSome()) {
> >   Try unregister = unregisterNotifier(eventfd.get());
> >   if (unregister.isError()) {
> > LOG(ERROR) << "Failed to unregister eventfd: " << 
> > unregister.error();
> >   }
> > }
> > 
> > ```

I originally tried to simplify the lifecycle of the listener, but it was 
implemented this way because some callers call listen multiple times.

create listener
listen
listen
listen
terminate


- Benjamin


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


On Oct. 23, 2018, 3:08 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69123/
> ---
> 
> (Updated Oct. 23, 2018, 3:08 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The cgroups event notifier was closing the eventfd while an
> `io::read()` operation may be in progress. This can lead to
> bugs where the fd gets re-used and read from a stale io::read.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 95efc1def3318dd93d30d91e9c8a91376cc610b8 
> 
> 
> Diff: https://reviews.apache.org/r/69123/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

[GitHub] asfgit closed pull request #315: fix Disarding a Future -> Discarding a Future

[GitBox]

asfgit closed pull request #315: fix Disarding a Future -> Discarding a Future
URL: https://github.com/apache/mesos/pull/315
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/3rdparty/libprocess/README.md b/3rdparty/libprocess/README.md
index 36370440c0..3e3d5e716d 100644
--- a/3rdparty/libprocess/README.md
+++ b/3rdparty/libprocess/README.md
@@ -103,7 +103,7 @@ The following table is meant to capture these transitions:
 
 We use the macros `CHECK_PENDING()`, `CHECK_READY()`, `CHECK_FAILED()`, 
`CHECK_DISCARDED()` throughout our examples. See [`CHECK()` 
Overloads](#futures-and-promises-check-overloads) for more details about these 
macros.
 
-###  Disarding a Future 
(aka Cancellation)
+###  Discarding a 
Future (aka Cancellation)
 
 You can "cancel" the result of some asynchronous operation by discarding a 
future. Unlike doing a discard on a promise, _discarding a future is a request 
that may or may not be be satisfiable_. You discard a future using 
`Future::discard()`. You can determine if a future has a discard request by 
using `Future::hasDiscard()` or set up a callback using `Future::onDiscard()`. 
Here's an example:
 


 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: Review Request 69163: Set agent and/or resource provider ID in operation status updates.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69163 was successfully built and tested.

Reviews applied: `['68147', '69157', '69158', '69159', '69160', '69161', 
'69162', '69163']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2529/mesos-review-69163

- Mesos Reviewbot Windows


On Oct. 25, 2018, 10:54 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69163/
> ---
> 
> (Updated Oct. 25, 2018, 10:54 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, and James DeFelice.
> 
> 
> Bugs: MESOS-9293
> https://issues.apache.org/jira/browse/MESOS-9293
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
> 
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (no agent
> ID, not resource provider ID).
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba 
>   src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3 
>   src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a 
>   src/resource_provider/storage/provider.cpp 
> db783b53558811081fb2671e005e8bbbd9edbede 
>   src/slave/slave.cpp 7bb2b291a7b67563adf88bcedca1392890f17ffe 
>   src/tests/slave_tests.cpp 637bedce20f63dfc1702ef549bcea8f790477be3 
> 
> 
> Diff: https://reviews.apache.org/r/69163/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69085: Stout: Always `fsync` created directories in POSIX `mkdir`.

[Benjamin Bannier]

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


Fix it, then Ship it!




LGTM modulo minor suggestions.

Like I already said, I believe `os::mkdir` is not a great abstraction, but that 
didn't appear with this patch.

* Recursive `mkdir` might fail but still create some directories. Users are 
expected to manually detect themself what failed and possibly roll back.
* Inconsistent error semantics for existing directories between `recursive` 
`true` and `false`.


3rdparty/stout/include/stout/os/posix/mkdir.hpp
Lines 33-35 (patched)


It would be nice to call out the (non-existing) guarantees in case of 
failure here, e.g.,
```
// Make a directory.
//
// If `recursive` is set to true, all intermediate directories
// will be created as required. `fsync()` will be called on the parent of 
each
// created directory to ensure that the result is commited to its 
filesystem.
//
// Note that in case of failures no cleanup is performed by this function
// itself.
```



3rdparty/stout/include/stout/os/posix/mkdir.hpp
Lines 63 (patched)


Just an observation: This function has different error semantics depending 
on whether `recursive` was `true` or `false`, i.e., `mkdir("/tmp", true)` would 
likely error while `mkdir("/tmp", false)` would not.

Nothing we should do something about right now, just another indication 
that this function isn't a great abstraction.



3rdparty/stout/include/stout/os/posix/mkdir.hpp
Lines 67-72 (patched)


Do we want to `fsync` directory entries which existed previously? I guess 
these could only appear unsynced if they were created by a user directly 
calling `::mkdir`, or from a external process.

I'd suggest to just move this block out of the `else` (which could go 
away), and execute it unconditionally.


- Benjamin Bannier


On Oct. 25, 2018, 1:02 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69085/
> ---
> 
> (Updated Oct. 25, 2018, 1:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9281
> https://issues.apache.org/jira/browse/MESOS-9281
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To ensure the directories created by `mkdir` are commited to their
> filesystems, an `fsync` will be called on the parent of each created
> directory.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/mkdir.hpp 
> 418db9af310ed763a5ae4735c2ebdd1ca38738ba 
> 
> 
> Diff: https://reviews.apache.org/r/69085/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69158: Added an integration test for resource provider removal.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69158 was successfully built and tested.

Reviews applied: `['68147', '69157', '69158']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2528/mesos-review-69158

- Mesos Reviewbot Windows


On Oct. 25, 2018, 10:47 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69158/
> ---
> 
> (Updated Oct. 25, 2018, 10:47 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an integration test for resource provider removal.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 637bedce20f63dfc1702ef549bcea8f790477be3 
> 
> 
> Diff: https://reviews.apache.org/r/69158/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69162: Added agent and resource provider IDs to operation status messages.

[James DeFelice]

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




include/mesos/v1/scheduler/scheduler.proto
Lines 147 (patched)


For these "certain cases" does Mesos still expect an ACK? If so, that's a 
problem.


- James DeFelice


On Oct. 25, 2018, 10:54 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69162/
> ---
> 
> (Updated Oct. 25, 2018, 10:54 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, and James DeFelice.
> 
> 
> Bugs: MESOS-9293
> https://issues.apache.org/jira/browse/MESOS-9293
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch add agent and resource provide IDs to
> `UpdateOperationStatus` and `UpdateOperationStatusMessage`. With that
> frameworks are able to reconcile enough information after failover to
> construct operation acknowledgements.
> 
> We will add code to populate these fields in a follow-up patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> f6a780a7b75878b9e74402a28a25bb868f7ac36f 
>   include/mesos/v1/scheduler/scheduler.proto 
> fcfec5e417463103e98dd6917722b4fde41cac7c 
>   src/internal/evolve.cpp aa60efed5a28ed4d847a4a27aa8e994cda82175d 
>   src/messages/messages.cpp dd8f60ecdbc06d10be1152bee1ddb65feaaf8fbb 
>   src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7 
> 
> 
> Diff: https://reviews.apache.org/r/69162/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 69163: Set agent and/or resource provider ID in operation status updates.

[Benjamin Bannier]

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

Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, and James DeFelice.


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


Repository: mesos


Description
---

This patch sets agent and/or resource provider ID operation status
update messages. This is not always possible, e.g., some operations
might fail validation so that no corresponding IDs can be extracted.

Since operations failing validation are currently directly rejected by
the master without going through a status update manager, they are not
retried either. If a master status update manager for operations is
introduced at a later point it should be possible to forward
acknowledgements for updates to the master's update manager (no agent
ID, not resource provider ID).


Diffs
-

  src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba 
  src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3 
  src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a 
  src/resource_provider/storage/provider.cpp 
db783b53558811081fb2671e005e8bbbd9edbede 
  src/slave/slave.cpp 7bb2b291a7b67563adf88bcedca1392890f17ffe 
  src/tests/slave_tests.cpp 637bedce20f63dfc1702ef549bcea8f790477be3 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 69159: Used an alias for reoccuring complicated type.

[Benjamin Bannier]

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

Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.


Repository: mesos


Description
---

Used an alias for reoccuring complicated type.


Diffs
-

  src/tests/mesos.hpp 40c63b0d6b272c4fb42ec2a126c1bcacb7abee57 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 69161: Renamed a function argument to not reuse member name.

[Benjamin Bannier]

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

Review request for mesos and Chun-Hung Hsiao.


Repository: mesos


Description
---

While a function argument shadowing a member variable is perfectly
legal, it is still confusing. In this patch we rename the function
argument to remove one case of such shadowing.


Diffs
-

  src/resource_provider/storage/provider.cpp 
db783b53558811081fb2671e005e8bbbd9edbede 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 69160: Included corresponding header file first.

[Benjamin Bannier]

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

Review request for mesos and Chun-Hung Hsiao.


Repository: mesos


Description
---

Included corresponding header file first.


Diffs
-

  src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 69162: Added agent and resource provider IDs to operation status messages.

[Benjamin Bannier]

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

Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, and James DeFelice.


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


Repository: mesos


Description
---

This patch add agent and resource provide IDs to
`UpdateOperationStatus` and `UpdateOperationStatusMessage`. With that
frameworks are able to reconcile enough information after failover to
construct operation acknowledgements.

We will add code to populate these fields in a follow-up patch.


Diffs
-

  include/mesos/scheduler/scheduler.proto 
f6a780a7b75878b9e74402a28a25bb868f7ac36f 
  include/mesos/v1/scheduler/scheduler.proto 
fcfec5e417463103e98dd6917722b4fde41cac7c 
  src/internal/evolve.cpp aa60efed5a28ed4d847a4a27aa8e994cda82175d 
  src/messages/messages.cpp dd8f60ecdbc06d10be1152bee1ddb65feaaf8fbb 
  src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 69157: Fixed handling for offer operation updates.

[Benjamin Bannier]

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

Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.


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


Repository: mesos


Description
---

The handling of offer operation updates introduced in `c946615ec6d`
made use of an update's `latest_status` without makeing sure that any
value was set. This could leave to situation where an uninitialized
enum value was switched on would would have caused a fatal error at
runtime.

This patch replaces uses of `latest_status` with `state` which does
contain the information we care about. We also adjust the error
logging so we log the value that lead to the error, not some other
value.


Diffs
-

  src/slave/slave.cpp 7bb2b291a7b67563adf88bcedca1392890f17ffe 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 68147: Added agent support to remove local resource providers.

[Benjamin Bannier]

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

(Updated Oct. 25, 2018, 12:47 p.m.)


Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, Greg Mann, and Jan 
Schlicht.


Changes
---

Rebased; address comments from Chun.


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


Repository: mesos


Description
---

This patch adds support for triggering permanent removal of local
resource providers. We also add authorization and tests as part of this
patch.


Diffs (updated)
-

  include/mesos/agent/agent.proto 74488e873cbf99ca487403b70691912cf3788288 
  include/mesos/v1/agent/agent.proto 5d1ab6fd7f6f2159367c66ed35ee90aea07f 
  src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3 
  src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a 
  src/slave/http.hpp 5b113fa2f7e3421d4219e0ece113010937b204c1 
  src/slave/http.cpp a4db532cc6477c386fcd9bf563895214e95a475a 
  src/slave/slave.hpp 0bd340176e2a8cefdfa7ef71e059441fb171aff6 
  src/slave/slave.cpp 7bb2b291a7b67563adf88bcedca1392890f17ffe 
  src/slave/validation.cpp df5e1373dbe497bc859455dcaf4e064e923bd72e 
  src/tests/api_tests.cpp c681b9b0b83f7472312083730a5433e3d6f0efc0 


Diff: https://reviews.apache.org/r/68147/diff/9/

Changes: https://reviews.apache.org/r/68147/diff/8-9/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 69158: Added an integration test for resource provider removal.

[Benjamin Bannier]

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

Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.


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


Repository: mesos


Description
---

Added an integration test for resource provider removal.


Diffs
-

  src/tests/slave_tests.cpp 637bedce20f63dfc1702ef549bcea8f790477be3 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 69123: Fixed an early fd close in the cgroups event notifier.

[Qian Zhang]

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



Not yours, I see in `MemorySubsystemProcess::oomListen` we have the these code:
```
  info->oomNotifier.onReady(
  defer(PID(this),
::oomWaited,
containerId,
cgroup,
lambda::_1));
```
Here the `onReady` seems not correct to me, I think it should be `onAny`.


src/linux/cgroups.cpp
Lines 1071-1080 (original), 1071-1089 (patched)


I see we already have an onAny callback `_listen`, can we close the fd at 
the end of that method (i.e., when we are sure that we will not listen 
anymore)? And then we could change the code here like below:
```
if (reading.isSome()) {
  reading->discard();
} else if (eventfd.isSome()) {
  Try unregister = unregisterNotifier(eventfd.get());
  if (unregister.isError()) {
LOG(ERROR) << "Failed to unregister eventfd: " << unregister.error();
  }
}

```


- Qian Zhang


On Oct. 23, 2018, 11:08 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69123/
> ---
> 
> (Updated Oct. 23, 2018, 11:08 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The cgroups event notifier was closing the eventfd while an
> `io::read()` operation may be in progress. This can lead to
> bugs where the fd gets re-used and read from a stale io::read.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 95efc1def3318dd93d30d91e9c8a91376cc610b8 
> 
> 
> Diff: https://reviews.apache.org/r/69123/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

[GitHub] watermeion opened a new pull request #315: fix Disarding a Future -> Discarding a Future

[GitBox]

watermeion opened a new pull request #315: fix Disarding a Future -> Discarding 
a Future
URL: https://github.com/apache/mesos/pull/315
 
 
   fix a typo in readme file


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: Review Request 69146: Made fetcher tests more robust by using the test sandbox.

[Benjamin Bannier]

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




src/tests/fetcher_tests.cpp
Lines 98-109 (patched)


We can avoid duplication here by making `FetcherTest` a `MesosTest` (that 
one would already today take care of `launcher_dir` and `fetcher_cache_dir`). 
This function would than at most need to invoke `MesosTest::CreateSlaveFlags` 
and make its non-general additions.

It probably even make sense to set `frameworks_home` in `CreateSlaveFlags` 
as well. That would remove the need for this function completely.


- Benjamin Bannier


On Oct. 25, 2018, 1:12 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> ---
> 
> (Updated Oct. 25, 2018, 1:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch adds a helper function `createSlaveFlags()`
> to make use of the test sandboxes. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a 
> 
> 
> Diff: https://reviews.apache.org/r/69146/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>