Re: Review Request 52382: Added stubs for OCI store.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52349, 52379, 52382]

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

- Mesos ReviewBot


On Nov. 29, 2016, 2:17 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52382/
> ---
> 
> (Updated Nov. 29, 2016, 2:17 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5011
> https://issues.apache.org/jira/browse/MESOS-5011
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added stubs for OCI store.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0884c6bb6f2ebddadc5cfc6b72d704086709748d 
>   src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 
>   src/slave/containerizer/mesos/provisioner/oci/store.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52382/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 54115: Added an http::serve abstraction.

[Benjamin Mahler]

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




3rdparty/libprocess/include/process/http.hpp (lines 884 - 885)


I'm not sure you should mention 'Connection: Keep-Alive' since that is from 
HTTP 1.0. HTTP 1.1 has persistent connections by default unless 'Connection: 
close' is specified.



3rdparty/libprocess/include/process/http.hpp (line 886)


Can you comment a bit on what discarding will do? i.e. is it a clean 
shutdown or not?

Because of this, I'm a bit inclined to think that we'll need an 
http::Server which can take arguments for this kind of thing. As an example:

```
class Server
{
  struct ShutdownOptions
  {
// During the grace period, no new connections will
// be accepted. Existing connections will be closed
// when currently received requests have been handled.
// The server will shut down reads on each connection
// to prevent new requests from arriving.
Duration gracePeriod;
  };

  // Shuts down the server.
  Future shutdown(Sever::ShutdownOptions options);
};
```

Since you're planning to remove the serve function below, how about leaving 
a TODO so that others understand the thinking? I'm thinking we would probably 
expose the 'Server' to callers and hide the 'serve' function from them (as it 
seems most callers will want the server).



3rdparty/libprocess/src/http.cpp (line 1398)


Socket::send is const? Weird. :)



3rdparty/libprocess/src/http.cpp (line 1432)


A bit weird that this takes a const socket, given it's going to do write 
operations on it.



3rdparty/libprocess/src/http.cpp (line 1433)


Why the copy? Note that copies of requests/responses is expensive, we want 
to move towards 0 copying of them.



3rdparty/libprocess/src/http.cpp (lines 1436 - 1437)


This seems a bit brittle since we're not checking for the things we expect, 
but rather what we do not expect. Should you just check for NONE and `BODY` 
here? I'm actually not sure why we have `NONE`, it seems equivalent to `BODY` 
with `body.empty()`.



3rdparty/libprocess/src/http.cpp (line 1449)


Ditto here.



3rdparty/libprocess/src/http.cpp (line 1450)


Ditto here w.r.t. copying.



3rdparty/libprocess/src/http.cpp (lines 1455 - 1457)


Should we CHECK against this since you're not handling the programming 
error?



3rdparty/libprocess/src/http.cpp (lines 1455 - 1457)


Should we CHECK against this since you're not handling the programming 
error? I'm not sure how we could surface this to the handler writer, it seems 
the only thing we could do is to CHECK here or prevent this kind of 
mal-construction altogether.



3rdparty/libprocess/src/http.cpp (line 1459)


This FD will leak into the child processes since you don't have 
`O_CLOEXEC`. Also, don't you want `O_NONBLOCK` here to avoid blocking in the 
kernel during reads?

We should use `os::open` from stout here, which handles O_NONBLOCK on 
systems without it defined:

```
Try fd = os::open(response.path, O_CLOEXEC | O_NONBLOCK | O_RDONLY);
```

And you won't have to use os::strerror.



3rdparty/libprocess/src/http.cpp (lines 1469 - 1483)


Hm.. seems like we need an `os::isdir` overload that takes the fd:

```
bool os::isdir(const string& path); // exists
bool os::isdir(int fd); // does not exist
```

oh well



3rdparty/libprocess/src/http.cpp (line 1476)


You're leaking the fd here?



3rdparty/libprocess/src/http.cpp (line 1482)


You're leaking the fd here?



3rdparty/libprocess/src/http.cpp (lines 1493 - 1504)


If the .then doesn't run, it looks like the fd will be leaked.



3rdparty/libprocess/src/http.cpp (line 1498)


Indentation is off here.



3rdparty/libprocess/src/http.cpp (line 1499)


It's weird that you value capture encoder only to reassign it, can you just 
avoid 

Re: Review Request 53974: Added support to handle ATTACH_CONTAINER_OUPUT in the io switchbaord.

[Kevin Klues]

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

(Updated Nov. 29, 2016, 6:49 a.m.)


Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.


Changes
---

Rebase on newest changes.


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


Repository: mesos


Description
---

Added support to handle ATTACH_CONTAINER_OUPUT in the io switchbaord.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
25cbf2447d197134f0753b062b6f4130821005b2 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 53939: Added implementation for containerizer 'attach()' call.

[Kevin Klues]

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

(Updated Nov. 29, 2016, 6:48 a.m.)


Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.


Changes
---

Rebase on newest changes.


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


Repository: mesos


Description
---

Added implementation for containerizer 'attach()' call.


Diffs (updated)
-

  src/slave/containerizer/composing.cpp 
e0818bce8a8c3561f2fdd1127cea9ce310ae7f4d 
  src/slave/containerizer/mesos/containerizer.cpp 
9b33495d9babc3ee489a8712fe1977746c41043f 
  src/slave/containerizer/mesos/io/switchboard.hpp 
aaa3a35245b291f6003f519dbf8c0e1b82bc15fd 
  src/slave/containerizer/mesos/io/switchboard.cpp 
25cbf2447d197134f0753b062b6f4130821005b2 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Review Request 54148: Updated the io switchboard to launch an external io switchboard server.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

We don't currently handle recovering access to the io switchboard
server process after agent restarts. We will add that in a subsequent
commit.


Diffs
-

  src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 
  src/slave/containerizer/mesos/containerizer.cpp 
9b33495d9babc3ee489a8712fe1977746c41043f 
  src/slave/containerizer/mesos/io/switchboard.hpp 
aaa3a35245b291f6003f519dbf8c0e1b82bc15fd 
  src/slave/containerizer/mesos/io/switchboard.cpp 
25cbf2447d197134f0753b062b6f4130821005b2 
  src/slave/containerizer/mesos/io/switchboard_main.cpp PRE-CREATION 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests

[  FAILED  ] 
bool/UserContainerLoggerTest.ROOT_LOGROTATE_RotateWithSwitchUserTrueOrFalse/0, 
where GetParam() = true


Thanks,

Kevin Klues

Review Request 54147: Added a server side component for the IOSwitchboard.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

The 'IOSwitchboardServer' component encapsulates the server side logic
for redirecting the 'stdin/stdout/stderr' of a container to/from
multiple sources/targets. For now, we only redirect IO from a
container to the FDs supplied to us by the logger.  We also send the
stdout/stderr data to a simple HTTP server that we launch on a unix
domain socket set up by the agent.  Right now this server is just a
stub and doesn't do anything useful.

In future commits, we will expand this HTTP server to handle
'ATTACH_CONTAINER_INPUT' and 'ATTACH_CONTAINER_OUTPUT' calls on behalf
of a container. It will use the stdout/stderr messages passed to it to
and send that data over the response stream to any clients connected
with an 'ATTACH_CONTAINER_OUTPUT' call. Likewise, it will take any
input streamed in over a 'ATTACH_CONTAINER_INPUT' request and write it
to a container's stdin.

In 'local' mode, it will be run inside the agent itself. In
'non-local' mode, it will be run as an external process to survive
agent restarts.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.hpp 
aaa3a35245b291f6003f519dbf8c0e1b82bc15fd 
  src/slave/containerizer/mesos/io/switchboard.cpp 
25cbf2447d197134f0753b062b6f4130821005b2 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 53837: Added a per container io switchboard server process.

[Kevin Klues]

> On Nov. 23, 2016, 7:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1400-1411
> > 
> >
> > hum, that means we cannot use attach with local mode? I think we should 
> > pass 'local' to io switchboard prepare. 'local' simply means redirect the 
> > container io to stdio (in fact, maybe we should have a local logger?). 
> > However, we should still be allowed to do attach.
> > 
> > In local mode, we probably want to run io switchboard as an actor in 
> > the same linux process, rather than forking a new process.
> > 
> > Thoughts?

For now, let's not bother with the "local" mode for the switchboard process, 
but I have addressed the comment abot passing locla in here for other purposes.


> On Nov. 23, 2016, 7:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 2389-2390
> > 
> >
> > See my comments below. I'd suggest we don't do wait/destroy for io 
> > switchboard yet.

As discussed offline. We are going to turn this component into an isolataor, 
which will remove the need for a special purpose `wait()` call. We will use the 
isolator `watch()` function for a similar purpose.


> On Nov. 23, 2016, 7:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/io_switchboard/io_switchboard.cpp, line 168
> > 
> >
> > Realized an FD leak in logger code. This is not yours because the 
> > original code has the same leak.
> > 
> > When we return error below, we need to make sure that fds in loggerInfo 
> > are closed.

What do you want me to do here exactly? Close the logger fds (if they are fds) 
if prepare() fails? I kind of feel like this should be inside the logger, not 
here.


> On Nov. 23, 2016, 7:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/io_switchboard/io_switchboard.cpp, lines 
> > 337-343
> > 
> >
> > So this is a TODO for agent restart case? What's the plan moving 
> > forward? Looks like previously, we don't handle logger process destory and 
> > wait as well. I'd suggest we don't do this optimization yet.
> > 
> > Moving forward, do we plan to move the processes (including io 
> > switchboard and logger) to the container's cgroup? If that's the case, 
> > maybe it makes sense to make this an isolator (wrapping ioswitch board and 
> > logger, e.g., 'posix/io' isolator) which is always enabled (like filesystem 
> > or network isolator). The subprocess io info can be part of the 
> > ContainerLaunchInfo. If we put those processes in the same cgroup, we don't 
> > have to worry about destroy/wait anymore. Thoughts?

As discussed offline, we are going to make this an isolator.


- Kevin


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


On Nov. 23, 2016, 2:45 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53837/
> ---
> 
> (Updated Nov. 23, 2016, 2:45 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, this process simply intercepts the stdout/stderr of a
> container and writes it to the stdout/stderr FDs set up by the
> container logger. We also send the stdout/stderr data to a simple HTTP
> server that we launch on a unix domain socket set up by the agent.
> Right now this server is just a stub and doesn't do anything useful.
> 
> In future commits, we will expand this HTTP server to handle
> 'ATTACH_CONTAINER_INPUT' and 'ATTACH_CONTAINER_OUTPUT' calls on behalf
> of a container. It will use the stdout/stderr messages passed to it to
> and send that data over the response stream to any clients connected
> with an 'ATTACH_CONTAINER_OUTPUT' call. Likewise, it will take any
> input streamed in over a 'ATTACH_CONTAINER_INPUT' request and write it
> to a container's stdin.
> 
> We don't currently handle recovering access to the io switchboard
> server process after agent restarts. We will add that in a subsequent
> commit.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5e0b8406f7f624bd8b03ff76b887f20e22fc66e0 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e47a120bfbb607cc0cdbdaed934ae15f15666ae3 
>   src/slave/containerizer/mesos/io/switchboard.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/io/switchboard.cpp PRE-CREATION

Re: Review Request 53837: Added a per container io switchboard server process.

[Kevin Klues]

> On Nov. 23, 2016, 10:48 p.m., Jie Yu wrote:
> > We should probably split the server code out and add unit test for the 
> > server.

It is now split out, but I haven't written the unit tests yet.


> On Nov. 23, 2016, 10:48 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 274-275
> > 
> >
> > Do you need this one? Can you just inline `{}` in subprocess call below?

I can't just use '{}'. It appears to interpret that as None() instead of an 
empty map. I've changed it to `map()` though, and it seems to 
work.


> On Nov. 23, 2016, 10:48 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 293-296
> > 
> >
> > Instead of closing them in a parent hook which blocks the child, can 
> > you just close them in after 'subprocess()' call.

Where do you want me to close them?


> On Nov. 23, 2016, 10:48 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/io/switchboard_main.hpp, line 30
> > 
> >
> > I'd suggest we just use os::mktemp(). Attaching a container id to the 
> > socket path means that we might not be able to handle a deep nested 
> > container.

We talked offline about how best to do this. See the udpated code.


> On Nov. 23, 2016, 10:48 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/io/switchboard_main.cpp, lines 320-324
> > 
> >
> > I'd suggest we simply wait for the termination of the 
> > IOSwitchboardServer actor and if it terminates, we terminate the linux 
> > process.
> > 
> > Intead of rely on the return value of 'run' method, the standard 
> > pattern is to have a method 'future' which returns the future of the 
> > promise. And then, spawn the process. See src/log/consensus.cpp for 
> > examples.
> > 
> > Therefore, in IOSwitchboardServer, whenever there is an error, you can 
> > simply terminate(self()) and fail the promise.

The logic is all greatly simplified now for this. Take  look.


- Kevin


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


On Nov. 23, 2016, 2:45 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53837/
> ---
> 
> (Updated Nov. 23, 2016, 2:45 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, this process simply intercepts the stdout/stderr of a
> container and writes it to the stdout/stderr FDs set up by the
> container logger. We also send the stdout/stderr data to a simple HTTP
> server that we launch on a unix domain socket set up by the agent.
> Right now this server is just a stub and doesn't do anything useful.
> 
> In future commits, we will expand this HTTP server to handle
> 'ATTACH_CONTAINER_INPUT' and 'ATTACH_CONTAINER_OUTPUT' calls on behalf
> of a container. It will use the stdout/stderr messages passed to it to
> and send that data over the response stream to any clients connected
> with an 'ATTACH_CONTAINER_OUTPUT' call. Likewise, it will take any
> input streamed in over a 'ATTACH_CONTAINER_INPUT' request and write it
> to a container's stdin.
> 
> We don't currently handle recovering access to the io switchboard
> server process after agent restarts. We will add that in a subsequent
> commit.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5e0b8406f7f624bd8b03ff76b887f20e22fc66e0 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e47a120bfbb607cc0cdbdaed934ae15f15666ae3 
>   src/slave/containerizer/mesos/io/switchboard.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/io/switchboard.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/io/switchboard_main.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/io/switchboard_main.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53837/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 53995: Added API handler for ATTACH_CONTAINER_OUTPUT.

[Vinod Kone]

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

(Updated Nov. 29, 2016, 6:11 a.m.)


Review request for mesos, Anand Mazumdar and Benjamin Mahler.


Changes
---

anand's. NNFR.


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


Repository: mesos


Description
---

Added API handler for ATTACH_CONTAINER_OUTPUT.


Diffs (updated)
-

  src/internal/evolve.hpp e8b03c9f04b8a835f35ce7cb6cc44e3ecd799422 
  src/internal/evolve.cpp 23a515c0aa8c31e0789fab84830dae32574604bc 
  src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 
  src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb 
  src/slave/validation.cpp b3d8bcd0f0855d2a0a96821900d996ba86e11578 
  src/tests/api_tests.cpp 8889e7807ecead736eaac3910332e86d594e8cec 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 53995: Added API handler for ATTACH_CONTAINER_OUTPUT.

[Anand Mazumdar]

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


Fix it, then Ship it!





src/slave/http.cpp (line 2175)


argument by reference: `Future&`



src/tests/api_tests.cpp (line 31)


Remove this include?


- Anand Mazumdar


On Nov. 29, 2016, 5:38 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53995/
> ---
> 
> (Updated Nov. 29, 2016, 5:38 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6473
> https://issues.apache.org/jira/browse/MESOS-6473
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added API handler for ATTACH_CONTAINER_OUTPUT.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp e8b03c9f04b8a835f35ce7cb6cc44e3ecd799422 
>   src/internal/evolve.cpp 23a515c0aa8c31e0789fab84830dae32574604bc 
>   src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 
>   src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb 
>   src/slave/validation.cpp b3d8bcd0f0855d2a0a96821900d996ba86e11578 
>   src/tests/api_tests.cpp 8889e7807ecead736eaac3910332e86d594e8cec 
> 
> Diff: https://reviews.apache.org/r/53995/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 53995: Added API handler for ATTACH_CONTAINER_OUTPUT.

[Vinod Kone]

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

(Updated Nov. 29, 2016, 5:38 a.m.)


Review request for mesos, Anand Mazumdar and Benjamin Mahler.


Changes
---

anand's comments. thanks for the thorough review!

also, fixed the content type of the response.


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


Repository: mesos


Description
---

Added API handler for ATTACH_CONTAINER_OUTPUT.


Diffs (updated)
-

  src/internal/evolve.hpp e8b03c9f04b8a835f35ce7cb6cc44e3ecd799422 
  src/internal/evolve.cpp 23a515c0aa8c31e0789fab84830dae32574604bc 
  src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 
  src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb 
  src/slave/validation.cpp b3d8bcd0f0855d2a0a96821900d996ba86e11578 
  src/tests/api_tests.cpp 8889e7807ecead736eaac3910332e86d594e8cec 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 54039: Added `recordio::transform` helper.

[Vinod Kone]

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

(Updated Nov. 29, 2016, 5:36 a.m.)


Review request for mesos, Anand Mazumdar and Benjamin Mahler.


Changes
---

typo. NNFR.


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


Repository: mesos


Description
---

This helper lets us transform raw bytes read from a RecordIO
reader and write to a pipe.


Diffs (updated)
-

  src/common/recordio.hpp cbd9d3667ba43eabb22c1af20853b8ed8e093852 
  src/tests/common/recordio_tests.cpp 86a111724bd303bbb17b84d8b32b811476c66b57 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 54062: Added logic to validate FrameworkInfo roles.

[Guangya Liu]

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




src/master/master.cpp (lines 7013 - 7021)


You may want to create a follow up patch to handle a framework with 
multiple roles here.


- Guangya Liu


On 十一月 27, 2016, 4:12 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54062/
> ---
> 
> (Updated 十一月 27, 2016, 4:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Qiang Zhang.
> 
> 
> Bugs: MESOS-6629
> https://issues.apache.org/jira/browse/MESOS-6629
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We need to do necessary validation for the conflicts of role, roles
> and MULTI_ROLE capability. It complies with following matrix:
> 
> -- MULTI_ROLE is NOT set -
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  None   |
> |---|---|-|
> |No Role| Error |  None   |
> |---|---|-|
> 
> --- MULTI_ROLE is set 
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  Error  |
> |---|---|-|
> |No Role| None  |  None   |
> |---|---|-|
> 
> One test case is added to ensure validateRoles() catches invalid
> combination of these attributes. Another three test cases are added
> to ensure the master accepts/rejects subscription given valid/invalid
> multiple roles.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a 
>   src/master/validation.hpp b8389460f34b3531f2b6ff93f18f496c01e1a079 
>   src/master/validation.cpp 42d9b4a8784c2a161b74d7b46619cc22272e14e3 
>   src/tests/master_validation_tests.cpp 
> f893067859425967654401f3226149268b51cf57 
> 
> Diff: https://reviews.apache.org/r/54062/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53790, 53791]

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

- Mesos ReviewBot


On Nov. 29, 2016, 1:21 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Nov. 29, 2016, 1:21 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0884c6bb6f2ebddadc5cfc6b72d704086709748d 
>   src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 
>   src/linux/ldd.hpp PRE-CREATION 
>   src/linux/ldd.cpp PRE-CREATION 
>   src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
>   src/tests/containerizer/rootfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 54103: Made MesosContainerizer launch helper to take ContainerLaunchInfo.

[Gilbert Song]

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


Fix it, then Ship it!




The patch LGTM! To be safe, will make the second pass and give a second ship it 
after lunch.


src/slave/containerizer/mesos/launch.hpp (line 28)


why newline?


- Gilbert Song


On Nov. 26, 2016, 11:49 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54103/
> ---
> 
> (Updated Nov. 26, 2016, 11:49 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Kevin Klues.
> 
> 
> Bugs: MESOS-6648
> https://issues.apache.org/jira/browse/MESOS-6648
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, the launch helper takes various flags from
> MesosContainerizer to launch the container. This makes it very hard to
> add more parameters to the launch helper. This patch simplifies this
> by passing 'ContainerLaunchInfo' instead. 'ContainerLaunchInfo' is
> also the protobuf message returned by isolators during 'prepare()'.
> This makes it very easy to merge them and send it to the launch
> helper. More importantly, this makes it very easy to add more
> parameters to the launch helper in the future.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 2621522ae59cf9275f607679b4678ac54508993d 
>   src/launcher/posix/executor.cpp da0081c0e470aebb16d2e78031d276f5d7d2c726 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9b33495d9babc3ee489a8712fe1977746c41043f 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> 6f0d6b0c9c1d78e5fecbbef4c6ff03825356799d 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8f024d084189b59bb229c63d20108e7bfe42065f 
>   src/slave/containerizer/mesos/isolators/filesystem/shared.cpp 
> d16b42f2231ba2e7779e798efe05e2bbd20dfac9 
>   src/slave/containerizer/mesos/isolators/namespaces/pid.cpp 
> 5bf5ffbceed1229fad35ca94f42b93c097152fe5 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 3b2d4db325b23a13fa0c1a4035f309816de181fd 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> a994adf78898b0e55ced8a2214287a97edc16d38 
>   src/slave/containerizer/mesos/launch.hpp 
> 155e3c5a27b8c710971ee4b508600d3b5589a2e0 
>   src/slave/containerizer/mesos/launch.cpp 
> 320e42748adbabf09f77cb4f5951e2a7ea58fe64 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 79b0a60c48a81c2f868d361ca07373bf3c2a8477 
> 
> Diff: https://reviews.apache.org/r/54103/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49571: Added a benchmark test for allocations.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53096, 45962, 49571]

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

- Mesos ReviewBot


On Nov. 29, 2016, 12:25 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49571/
> ---
> 
> (Updated Nov. 29, 2016, 12:25 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5771
> https://issues.apache.org/jira/browse/MESOS-5771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allocations test has the following resource configurations:
> (1) REGULAR: Offers from every slave have regular resources.
> (2) SHARED: Offers from every slave include a shared resource.
> (3) REGULAR: Offers from every alternate slave contain only regular
> resources; and offers from every other alternate slave contains
> a shared resource.
> 
> This test is parameterized based on number of agents, number of
> frameworks and resource configuration.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3b0eb75882102f44b6f490789bc48a638437a2d9 
>   src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
>   src/tests/resources_utils.cpp be1feb97be552af582dbba3a54fa5fa0714b3eb2 
> 
> Diff: https://reviews.apache.org/r/49571/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> Allocations benchmark test results
> ==
> Support of shared resources has a small impact (roughly 10%) on runtime 
> performance in allocations as compared to HEAD (without shared resources). 
> Also, there is no visible impact in performance when shared resources are 
> added in the tests.
> 
> Following is a snapshot with 1000 agents and 200 frameworks.
> 
> With the patch (and no shared resources)
> 
> [ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
> Using 1000 agents and 200 frameworks with resource type 0
> Added 200 frameworks in 6907us
> Added 1000 agents in 2.057098secs
> round 0 allocate() took 1.689164secs to make 1000 offers
> round 50 allocate() took 1.672373secs to make 1000 offers
> round 100 allocate() took 1.680571secs to make 1000 offers
> round 150 allocate() took 1.674683secs to make 1000 offers
> round 199 allocate() took 1.671525secs to make 1000 offers
> 
> With the patch (and shared resources on all agents)
> ---
> [ RUN  ] 
> AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
> Using 1000 agents and 200 frameworks with resource type 1
> Added 200 frameworks in 6888us
> Added 1000 agents in 2.096218secs
> round 0 allocate() took 1.704491secs to make 1000 offers
> round 50 allocate() took 1.718623secs to make 1000 offers
> round 100 allocate() took 1.716224secs to make 1000 offers
> round 150 allocate() took 1.707343secs to make 1000 offers
> round 199 allocate() took 1.727467secs to make 1000 offers
> 
> With the patch (and shared resources on alternate agents)
> -
> [ RUN  ] 
> AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
> Using 1000 agents and 200 frameworks with resource type 2
> Added 200 frameworks in 7304us
> Added 1000 agents in 2.071009secs
> round 0 allocate() took 1.689045secs to make 1000 offers
> round 50 allocate() took 1.691524secs to make 1000 offers
> round 100 allocate() took 1.688873secs to make 1000 offers
> round 150 allocate() took 1.688713secs to make 1000 offers
> round 199 allocate() took 1.691223secs to make 1000 offers
> 
> Based on HEAD, with all regular resources (no shared resources in HEAD 
> supported)
> -
> [ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
> Using 1000 agents and 200 frameworks with resource type 0
> Added 200 frameworks in 6801us
> Added 1000 agents in 1.721447secs
> round 0 allocate() took 1.502953secs to make 1000 offers
> round 50 allocate() took 1.520157secs to make 1000 offers
> round 100 allocate() took 1.517221secs to make 1000 offers
> round 150 allocate() took 1.526446secs to make 1000 offers
> round 199 allocate() took 1.538005secs to make 1000 offers
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 54110: Added `process::loop` abstraction.

[Benjamin Mahler]

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


Fix it, then Ship it!




There are also some comments in your pending review (e.g. re-structuring 
`run()` to always call `iterate()` at the beginning). Otherwise, looks good.

Will be interesting to see how we can integrate this into the rest of our code. 
I've wanted an asynchronous loop construct for awhile, we've written a lot of 
"delay loops" that have complicated cancellation handling.


3rdparty/libprocess/include/process/loop.hpp (lines 44 - 47)


Can you update all of these examples to show that body is also an 
asynchronous function?



3rdparty/libprocess/include/process/loop.hpp (lines 98 - 111)


Can we make this a managed process so that we don't have this wait issue?

See this example:

https://github.com/apache/mesos/blob/58f63747f185995d7f9cbfca9d240e2d60053184/3rdparty/libprocess/src/http.cpp#L1253-L1274



3rdparty/libprocess/include/process/loop.hpp (lines 117 - 131)


Maybe we make this a class with methods?

```
class Loop
{
public:
  void start();
  void run();
  // more?
};
```

To make the lifecycle more clear?



3rdparty/libprocess/include/process/loop.hpp (line 129)


Maybe '`item`'?



3rdparty/libprocess/include/process/loop.hpp (lines 212 - 228)


Once this moves into Loop::start / Loop::initialize / whatever we want to 
call it, then perhaps we can document the overall discard strategy at the top 
of `Loop`?



3rdparty/libprocess/src/tests/loop_tests.cpp (lines 28 - 75)


Can you also add a test for discard semantics?


- Benjamin Mahler


On Nov. 28, 2016, 6:33 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54110/
> ---
> 
> (Updated Nov. 28, 2016, 6:33 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `process::loop` abstraction.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
>   3rdparty/libprocess/include/process/loop.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/loop_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54110/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 54115: Added an http::serve abstraction.

[Benjamin Hindman]

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




3rdparty/libprocess/include/process/http.hpp (lines 898 - 902)


I'm going to remove this for now.



3rdparty/libprocess/src/tests/http_tests.cpp (line 1815)


Add a TODO here. Suggestions:

request.url = http::URL("http", address.path, 80, "/");

request.url = http::URL("http", "", 80, "/");

Which is still weird because it has port 80! So instead maybe a new URL 
overload that takes an address and "does the right thing" for Unix Domain 
Sockets?


- Benjamin Hindman


On Nov. 28, 2016, 6:20 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54115/
> ---
> 
> (Updated Nov. 28, 2016, 6:20 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an http::serve abstraction.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> a684e09c8353112a0656b7e899a469c1e022e93b 
>   3rdparty/libprocess/src/http.cpp 3f16f293a5c5cd0b31a85efe94cb6f8019543d45 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> d41929a9c8b2469c10b9e31985c447076c1684dc 
> 
> Diff: https://reviews.apache.org/r/54115/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 54110: Added `process::loop` abstraction.

[Benjamin Hindman]

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




3rdparty/libprocess/include/process/loop.hpp (line 55)


return iterate()
  .then([](T t) {
return body(t)
  .then([](bool condition) {
if (condition) {
  return loop();
} else {
  return Nothing();
}
  });
  });

Future t = iterate();
while (t.isReady()) {
  Future condition = body(t.get());
  if (condition.isReady()) {
t = iterate();
continue;
  } else {
condition
  .onAny(defer(...));
break;
  }
}

t.onAny(defer(...));

return iterate()
  .then(defer(pid, [](T t) {
return body(t)
  .then(defer(pid, [](bool condition) {
if (condition) {
  return loop();
} else {
  return Nothing();
}
  }));
  }));

loop(
[]() { return io::read(fd, data, size); },
[](size_t length) {
  if (length == 0) {
return false;
  }
  buffer.append(data, length);
})
.then([]() {
  return buffer.str();
});

Promise promise;

[]() {
  .onAny([]() { if (!condition) { promise.set(value); }})
});

return promise.future();



3rdparty/libprocess/include/process/loop.hpp (line 99)


We'll do a "managed" spawn here instead so that we don't need to `wait` or 
`delete` we just have to `terminate`.



3rdparty/libprocess/include/process/loop.hpp (line 124)


void start()
{
  promise.future()
.onDiscard(...);
  run();
}



3rdparty/libprocess/include/process/loop.hpp (line 129)


s/future/next/



3rdparty/libprocess/include/process/loop.hpp (line 137)


Future condition = true;

while (condition.get()) {
  Future next = discard_if_necessary(iterate());
  if (next.isReady()) {
condition = discard_if_necessary(body(next.get()));
if (condition.isReady()) {
  continue;
} else {
  block_on_condition();
  return;
}
  } else {
block_on_next();
return;
  }
}

promise.set(Nothing()); // All done!

void block_on_next()
{
next
  .onAny([]() {
  if (next.isReady()) {
condition = discard_if_necessary(body(next.get()));
if (condition.isReady()) {
  run(loop);
} else {
  block_on_condition();
}
  } else if (isDiscarded) {
  } else if (isFailed) {
  }
  });

}

void block_on_condition()
{
  condition
.onAny([]() {
  if (condition.isReady()) {
if (condition.get()) {
  run(loop);
} else {
  promise.set(Nothing());
}
  } else if (isDiscarded) {
  } else if (isFailed) {
  }
});
}



3rdparty/libprocess/include/process/loop.hpp (line 204)


dispatch(pid, [=]() {
  loop->start();
});



3rdparty/libprocess/include/process/loop.hpp (lines 206 - 208)


Add a comment which explains why we need to explicitly do the discard here, 
as well as in all the cases above.


- Benjamin Hindman


On Nov. 28, 2016, 6:33 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54110/
> ---
> 
> (Updated Nov. 28, 2016, 6:33 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `process::loop` abstraction.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
>   3rdparty/libprocess/include/process/loop.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/loop_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54110/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53628: Document the namespaces/uts isolator.

[haosdent huang]

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




docs/mesos-containerizer.md (lines 293 - 300)


Let's put this before `network/cni` isolator to ensure it sort 
alphabetically.


- haosdent huang


On Nov. 28, 2016, 5:07 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53628/
> ---
> 
> (Updated Nov. 28, 2016, 5:07 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6556
> https://issues.apache.org/jira/browse/MESOS-6556
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document the namespaces/uts isolator.
> 
> 
> Diffs
> -
> 
>   docs/mesos-containerizer.md 2bff35f6361f760a9001a249d2c01bbbc9e72932 
> 
> Diff: https://reviews.apache.org/r/53628/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53627: Implement a namespaces/uts isolator.

[haosdent huang]

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




src/slave/containerizer/mesos/isolators/namespaces/uts.cpp (lines 67 - 68)


Do we need to update this part to follow what we do in `namespace/pid` 
isolator?

```
Future> NamespacesPidIsolatorProcess::prepare(
const ContainerID& containerId,
const ContainerConfig& containerConfig)
{
  ContainerLaunchInfo launchInfo;

  if (containerId.has_parent()) {
// If we are a nested container, then we want to enter our
// parent's pid namespace before cloning a new one.
launchInfo.set_enter_namespaces(CLONE_NEWPID);

// However, if we are a nested container in the `DEBUG` class,
// then we don't want to clone a new PID namespace at all, so we
// short cirucuit here.
if (containerConfig.has_container_class() &&
containerConfig.container_class() == ContainerClass::DEBUG) {
  return launchInfo;
}
  }

  launchInfo.set_clone_namespaces(CLONE_NEWPID);
```



src/tests/containerizer/isolator_tests.cpp (lines 59 - 64)


Move this after

```
#include "slave/containerizer/mesos/launcher.hpp"
#include "slave/containerizer/mesos/paths.hpp"
```

Since it works in other operate systems as well.



src/tests/containerizer/isolator_tests.cpp (lines 384 - 396)


I think we could simpify these line to

```
  Owned detector = master.get()->createDetector();
  Try> slave = StartSlave(detector.get(), agentFlags);
  ASSERT_SOME(slave);
```


- haosdent huang


On Nov. 28, 2016, 5:07 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53627/
> ---
> 
> (Updated Nov. 28, 2016, 5:07 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6556
> https://issues.apache.org/jira/browse/MESOS-6556
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement a very simple namespaces/uts isolator that can be used to set
> the hostname and domainname of a container without the necessity of a
> CNI plugin.
> 
> Since we already had a `hostname` field in the ContainerInfo, we can
> use that to set the host name once we are in the UTS namespace. Add a
> corresponding `domainname` to the ContainerInfo to allow setting the
> domain name.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto cbfcd8a666e0b4a486f6dcd9e8356c9d5a1cea05 
>   include/mesos/slave/containerizer.proto 
> 2621522ae59cf9275f607679b4678ac54508993d 
>   src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 
>   src/cli/execute.cpp ddf7ecac21f2680c3027fafeb4194a2dd4a66d47 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9b33495d9babc3ee489a8712fe1977746c41043f 
>   src/slave/containerizer/mesos/isolators/namespaces/uts.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/namespaces/uts.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.hpp 
> 155e3c5a27b8c710971ee4b508600d3b5589a2e0 
>   src/slave/containerizer/mesos/launch.cpp 
> 320e42748adbabf09f77cb4f5951e2a7ea58fe64 
>   src/tests/containerizer/isolator_tests.cpp 
> 9766aaf144722b18d88f694ff37ffd53974cb60d 
> 
> Diff: https://reviews.apache.org/r/53627/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 52382: Added stubs for OCI store.

[Qian Zhang]

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

(Updated Nov. 29, 2016, 10:17 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added stubs for OCI store.


Diffs (updated)
-

  src/CMakeLists.txt 0884c6bb6f2ebddadc5cfc6b72d704086709748d 
  src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 
  src/slave/containerizer/mesos/provisioner/oci/store.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 52379: Added agent flag '--oci_store_dir'.

[Qian Zhang]

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

(Updated Nov. 29, 2016, 10:17 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Change the default value of '--oci_store_dir' from '/tmp/mesos/store/oci' to 
'/var/lib/mesos/store/oci'.


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


Repository: mesos


Description
---

Added agent flag '--oci_store_dir'.


Diffs (updated)
-

  docs/configuration.md efe3e9bd9d203a7ba44adf4ead24f14b8b577637 
  docs/endpoints/slave/state.json.md 0f82c1926404e79b281b2ea5f4d0ca21323aeded 
  docs/endpoints/slave/state.md b34459e8624f0b29e927ff79be7fc845ac88080b 
  src/slave/flags.hpp c6c3197bbf30ec617751f4a1a34914c0f0e29eb5 
  src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44 
  src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 53995: Added API handler for ATTACH_CONTAINER_OUTPUT.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54048, 54093, 54039, 53994, 54049, 53995]

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

- Mesos ReviewBot


On Nov. 28, 2016, 10:50 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53995/
> ---
> 
> (Updated Nov. 28, 2016, 10:50 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6473
> https://issues.apache.org/jira/browse/MESOS-6473
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added API handler for ATTACH_CONTAINER_OUTPUT.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp e8b03c9f04b8a835f35ce7cb6cc44e3ecd799422 
>   src/internal/evolve.cpp 23a515c0aa8c31e0789fab84830dae32574604bc 
>   src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 
>   src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb 
>   src/slave/validation.cpp b3d8bcd0f0855d2a0a96821900d996ba86e11578 
>   src/tests/api_tests.cpp 8889e7807ecead736eaac3910332e86d594e8cec 
>   src/tests/containerizer/mock_containerizer.hpp 
> 7a30b8307b93b7bf549efb52d72367f652d0d95a 
> 
> Diff: https://reviews.apache.org/r/53995/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 52349: Add protobuf messages for OCI image spec.

[Qian Zhang]

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

(Updated Nov. 29, 2016, 10:15 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add protobuf messages for OCI image spec.


Diffs (updated)
-

  include/mesos/oci/spec.hpp PRE-CREATION 
  include/mesos/oci/spec.proto PRE-CREATION 
  src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 54113: Added support for specifying how a socket should be shutdown.

[Benjamin Mahler]

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


Ship it!





3rdparty/libprocess/include/process/socket.hpp (line 388)


Can you leave a TODO to avoid having READ as the default in favor of 
READ_WRITE? Alternatively, we could remove the default entirely.



3rdparty/libprocess/include/process/socket.hpp (line 390)


Do you want a reference capture?



3rdparty/libprocess/include/process/socket.hpp (lines 392 - 394)


Be sure to commit greg's windows fix for mapping SHUT_WR and SHUT_RDWR 
before you commit this.


- Benjamin Mahler


On Nov. 28, 2016, 6:19 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54113/
> ---
> 
> (Updated Nov. 28, 2016, 6:19 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for specifying how a socket should be shutdown.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> f798af7879546d71e8ef4a295c9cf489a70cb61f 
> 
> Diff: https://reviews.apache.org/r/54113/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 54112: Added support for http::connect to take an network::Address.

[Benjamin Mahler]

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


Ship it!





3rdparty/libprocess/include/process/http.hpp (lines 863 - 867)


I only used URL in connect because I needed both scheme and the "host:port" 
(which is either an IP / hostname and a port). Since this is just an address, 
we should probably just have a single signature for this:

```
Future connect(const network::Address& address, bool ssl = 
false);
```

Maybe a comment with a suggestion about the direction we'd like to take?


- Benjamin Mahler


On Nov. 28, 2016, 6:19 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54112/
> ---
> 
> (Updated Nov. 28, 2016, 6:19 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for http::connect to take an network::Address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> a684e09c8353112a0656b7e899a469c1e022e93b 
>   3rdparty/libprocess/src/http.cpp 3f16f293a5c5cd0b31a85efe94cb6f8019543d45 
> 
> Diff: https://reviews.apache.org/r/54112/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 54111: Removed unused Socket from Encoder.

[Benjamin Mahler]

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


Fix it, then Ship it!





3rdparty/libprocess/src/tests/decoder_tests.cpp 


This seems unrelated? Maybe pull out the socket.hpp include from 
decoder_tests.cpp and encoder_tests.cpp in a small patch separately?


- Benjamin Mahler


On Nov. 28, 2016, 6:19 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54111/
> ---
> 
> (Updated Nov. 28, 2016, 6:19 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unused Socket from Encoder.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/encoder.hpp 
> 515821acb20d2a09d10781af17dcac472a7c117a 
>   3rdparty/libprocess/src/process.cpp 
> e9a4bbb0b2410e0260d120b97e73972c94eb0f26 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> 5f84d84dc8ff5e1f2684da14d30c1866ad8562d5 
> 
> Diff: https://reviews.apache.org/r/54111/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[James Peach]

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

(Updated Nov. 29, 2016, 1:21 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.


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


Repository: mesos


Description
---

The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since Stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.


Diffs (updated)
-

  src/CMakeLists.txt 0884c6bb6f2ebddadc5cfc6b72d704086709748d 
  src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 
  src/linux/ldd.hpp PRE-CREATION 
  src/linux/ldd.cpp PRE-CREATION 
  src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
  src/tests/containerizer/rootfs.cpp PRE-CREATION 

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


Testing
---

sudo make check (Fedora 24)


Thanks,

James Peach

Re: Review Request 53790: Move containerizer Rootfs support to a cpp file.

[James Peach]

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

(Updated Nov. 29, 2016, 1:21 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Move containerizer Rootfs support to a cpp file.


Diffs (updated)
-

  src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 
  src/tests/CMakeLists.txt 23230123f96f35f2aca3676b0eaa835098ea4f79 
  src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
  src/tests/containerizer/rootfs.cpp PRE-CREATION 

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


Testing
---

sudo make check (Fedora 24)


Thanks,

James Peach

Re: Review Request 53825: MESOS-6597 Enabled java protos generation for all V1 proto files.

[Vijay Srinivasaraghavan]

> On Nov. 20, 2016, 9:04 p.m., Anand Mazumdar wrote:
> > Nice first patch and welcome to the community!
> > 
> > - Can you update the Testing Done section with details on testing?
> > - We also support Python bindings. Do you mind adding these protos to our 
> > python build too in a follow up review? (See my comments later on how you 
> > can use review dependencies if you are new to ReviewBoard)
> 
> Vijay Srinivasaraghavan wrote:
> Thanks Anand. I have made the python binding changes locally on my 
> machine. Do you want me to create a new JIRA bug for this issue and create a 
> new review or add the patch to this JIRA/review itself? I am little confused 
> as to how the dependencies are handled with RB.
> 
> Anand Mazumdar wrote:
> No need to create a separate issue. It would be fine to create a new 
> review with it only containing the python related changes. This would be 
> another commit based on top of this one. You can then use `post-reviews.py` 
> and it should handle setting the dependencies for you. Feel free to ping me 
> on IRC/Slack if you run into any issues (anandm)
> 
> Vijay Srinivasaraghavan wrote:
> Thanks for the clarification. I have attached a new patch with "53825" as 
> dependendency
> 
> Vijay Srinivasaraghavan wrote:
> I have created two patch for those additional issues that was addressed 
> as part of the original request. Please let me know if you are waiting for 
> anything from my end before this can be merged? Thanks  
> https://reviews.apache.org/r/54014/
> https://reviews.apache.org/r/54015/
> 
> Anand Mazumdar wrote:
> Sorry for the delay. I would get to them sometime later this week. There 
> is nothing needed from your end for now.

Thanks Anand


- Vijay


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


On Nov. 23, 2016, 5:46 a.m., Vijay Srinivasaraghavan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53825/
> ---
> 
> (Updated Nov. 23, 2016, 5:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zameer Manji.
> 
> 
> Bugs: MESOS-6597
> https://issues.apache.org/jira/browse/MESOS-6597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6597 Enabled java protos generation for all V1 proto files.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5e0b8406f7f624bd8b03ff76b887f20e22fc66e0 
> 
> Diff: https://reviews.apache.org/r/53825/diff/
> 
> 
> Testing
> ---
> 
> Built mesos and confirmed jar file containing new java PB wrappers for V1 
> API. Ran standard unit test (make tests)
> 
> 
> Thanks,
> 
> Vijay Srinivasaraghavan
> 
>

Re: Review Request 54053: Updated 'io::redirect()' to take an optional vector of callback hooks.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54053]

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

- Mesos ReviewBot


On Nov. 28, 2016, 9:18 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54053/
> ---
> 
> (Updated Nov. 28, 2016, 9:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Bugs: MESOS-6639
> https://issues.apache.org/jira/browse/MESOS-6639
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These callback hooks will be invoked before passing any data read from
> the 'from' file descriptor on to the 'to' file descriptor.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/io.hpp 
> eec5efd7e6b71a783f2bb40826054d0488cee71f 
>   3rdparty/libprocess/src/io.cpp d930ceebc90468e082b984e41385549f906dc6ae 
>   3rdparty/libprocess/src/tests/io_tests.cpp 
> 66b610e35fa9f6a1738c77d181d76dca3921e6fb 
> 
> Diff: https://reviews.apache.org/r/54053/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j40 check
> GTEST_FILTER="IOTest.Redirect" 3rdparty/libprocess/libprocess-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 54109: Changes in Mesos to make http::Request::client optional.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Nov. 28, 2016, 6:18 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54109/
> ---
> 
> (Updated Nov. 28, 2016, 6:18 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes in Mesos to make http::Request::client optional.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 90cbed1ba411e18906fe9c26bc14576a26d1b7b9 
>   src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 
> 
> Diff: https://reviews.apache.org/r/54109/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53462: Updated usage of network::Address and network::Socket in Mesos.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Nov. 28, 2016, 6:18 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53462/
> ---
> 
> (Updated Nov. 28, 2016, 6:18 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated usage of network::Address in Mesos.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 79526ae7719eaef1456a0986d191b0b36b5f1ae6 
>   src/tests/utils.cpp d36aa9ca32b45da304059210de83c55bf9181847 
> 
> Diff: https://reviews.apache.org/r/53462/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 54108: Changes in libprocess to make http::Request::client optional.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Nov. 28, 2016, 6:17 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54108/
> ---
> 
> (Updated Nov. 28, 2016, 6:17 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes in libprocess to make http::Request::client optional.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> a684e09c8353112a0656b7e899a469c1e022e93b 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> d41929a9c8b2469c10b9e31985c447076c1684dc 
> 
> Diff: https://reviews.apache.org/r/54108/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[James Peach]

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

(Updated Nov. 29, 2016, 12:37 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.


Changes
---

Rebase and update from review comments.


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


Repository: mesos


Description
---

The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since Stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.


Diffs (updated)
-

  src/CMakeLists.txt 9ff47d7a95c3baa5aa10039039e5ad065180ba45 
  src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 
  src/linux/ldd.hpp PRE-CREATION 
  src/linux/ldd.cpp PRE-CREATION 
  src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
  src/tests/containerizer/rootfs.cpp PRE-CREATION 

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


Testing
---

sudo make check (Fedora 24)


Thanks,

James Peach

Re: Review Request 53790: Move containerizer Rootfs support to a cpp file.

[James Peach]

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

(Updated Nov. 29, 2016, 12:36 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Move containerizer Rootfs support to a cpp file.


Diffs (updated)
-

  src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 
  src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
  src/tests/containerizer/rootfs.cpp PRE-CREATION 

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


Testing
---

sudo make check (Fedora 24)


Thanks,

James Peach

Re: Review Request 53888: Improved SlaveRecoveryTest.ReconcileShutdownFramework.

[Neil Conway]

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

(Updated Nov. 29, 2016, 12:26 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Check the output of the "/state" endpoint to confirm that the framework
has been shutdown properly and the task has been marked as killed.


Diffs (updated)
-

  src/tests/slave_recovery_tests.cpp 3b7d8cf9600496d2e7f5105e4f561e914c54a36a 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

[Anindya Sinha]

> On Nov. 19, 2016, 12:30 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 757
> > 
> >
> > So if `operation` contians multiple copies, the result will end up 
> > having multiple copies right?

This piece of code is executed for non LAUNCH operations only. So we should 
have at most a single copy of shared resources at this point for a specific 
operation.


> On Nov. 19, 2016, 12:30 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 752-755
> > 
> >
> > This seems the wrong place to do it, multiple copies of the same shared 
> > resources are added to the allocation of the role (in roleSorter and 
> > quotaRoleSorter) from multiple places:
> > 
> > - updateAllocation
> > - addFramework
> > - addSlave
> > - allocate
> > 
> > If the rule is to not have more than one copy the shared resources to 
> > roleSorter and quotaRoleSorter, they should be invariants, i.e., we should 
> > prevent them from being added instead of deduping them here.
> > 
> > Let's chat about the design.

That is indeed the case. We can have multiple copies of shared resources in 
allocations for role sorter and quota sorter; but only a single copy of shared 
resources in the total maintained in role sorter and quota sorter. The issue 
here is that we use the shared count in allocations in framework sorter to 
remove appropriate resources in the role and quota sorter's total resources. 
Since total resources in role sorter and quota sorter is atmost 1, but 
allocations in framework sorter can be more which results in 
`CHECK(total_.resources[slaveId].contains(resources))` failing.

So, this fix seems to take care of the inconsistencies in the shared count. 
However, as discussed it would be better to use the agent's total resources 
(before and after applying the appropriate `operation`) to account for the 
total resources in the role and quota sorter, i.e. something similar to the 
approach in `HierarchicalAllocatorProcess::updateAvailable()`, as follows:

```
  // Update the total resources.
  Try updatedTotal = slaves[slaveId].total.apply(operations);
  CHECK_SOME(updatedTotal);

  const Resources oldTotal = slaves[slaveId].total;
  slaves[slaveId].total = updatedTotal.get();

  // Now, update the total resources in the role sorters by removing
  // the previous resources at this slave and adding the new resources.
  roleSorter->remove(slaveId, oldTotal);
  roleSorter->add(slaveId, updatedTotal.get());

  // See comment at `quotaRoleSorter` declaration regarding non-revocable.
  quotaRoleSorter->remove(slaveId, oldTotal.nonRevocable());
  quotaRoleSorter->add(slaveId, updatedTotal.get().nonRevocable());
```


- Anindya


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


On Nov. 18, 2016, 5:14 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53096/
> ---
> 
> (Updated Nov. 18, 2016, 5:14 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6444
> https://issues.apache.org/jira/browse/MESOS-6444
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We maintain a single copy of shared resource in the role and quota
> sorter's total resources. So, when we update these resources, we ensure
> that we only count a single copy even though the framework sorter
> may be returned multiple copies of a shared resource.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
>   src/tests/persistent_volume_tests.cpp 
> 8651b2c5455089041f16d091c90a7e0d948191b8 
> 
> Diff: https://reviews.apache.org/r/53096/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

(Updated Nov. 29, 2016, 12:25 a.m.)


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 49571: Added a benchmark test for allocations.

[Anindya Sinha]

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

(Updated Nov. 29, 2016, 12:25 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Allocations test has the following resource configurations:
(1) REGULAR: Offers from every slave have regular resources.
(2) SHARED: Offers from every slave include a shared resource.
(3) REGULAR: Offers from every alternate slave contain only regular
resources; and offers from every other alternate slave contains
a shared resource.

This test is parameterized based on number of agents, number of
frameworks and resource configuration.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
3b0eb75882102f44b6f490789bc48a638437a2d9 
  src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
  src/tests/resources_utils.cpp be1feb97be552af582dbba3a54fa5fa0714b3eb2 

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


Testing
---

All tests passed.

Allocations benchmark test results
==
Support of shared resources has a small impact (roughly 10%) on runtime 
performance in allocations as compared to HEAD (without shared resources). 
Also, there is no visible impact in performance when shared resources are added 
in the tests.

Following is a snapshot with 1000 agents and 200 frameworks.

With the patch (and no shared resources)

[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6907us
Added 1000 agents in 2.057098secs
round 0 allocate() took 1.689164secs to make 1000 offers
round 50 allocate() took 1.672373secs to make 1000 offers
round 100 allocate() took 1.680571secs to make 1000 offers
round 150 allocate() took 1.674683secs to make 1000 offers
round 199 allocate() took 1.671525secs to make 1000 offers

With the patch (and shared resources on all agents)
---
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
Using 1000 agents and 200 frameworks with resource type 1
Added 200 frameworks in 6888us
Added 1000 agents in 2.096218secs
round 0 allocate() took 1.704491secs to make 1000 offers
round 50 allocate() took 1.718623secs to make 1000 offers
round 100 allocate() took 1.716224secs to make 1000 offers
round 150 allocate() took 1.707343secs to make 1000 offers
round 199 allocate() took 1.727467secs to make 1000 offers

With the patch (and shared resources on alternate agents)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
Using 1000 agents and 200 frameworks with resource type 2
Added 200 frameworks in 7304us
Added 1000 agents in 2.071009secs
round 0 allocate() took 1.689045secs to make 1000 offers
round 50 allocate() took 1.691524secs to make 1000 offers
round 100 allocate() took 1.688873secs to make 1000 offers
round 150 allocate() took 1.688713secs to make 1000 offers
round 199 allocate() took 1.691223secs to make 1000 offers

Based on HEAD, with all regular resources (no shared resources in HEAD 
supported)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6801us
Added 1000 agents in 1.721447secs
round 0 allocate() took 1.502953secs to make 1000 offers
round 50 allocate() took 1.520157secs to make 1000 offers
round 100 allocate() took 1.517221secs to make 1000 offers
round 150 allocate() took 1.526446secs to make 1000 offers
round 199 allocate() took 1.538005secs to make 1000 offers


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

[Anindya Sinha]

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

(Updated Nov. 29, 2016, 12:25 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description (updated)
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we
remove the previous resources at this agent and add the new resources
at this agent.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
3b759494071c4cae4b8b7dbcb0028df4146fc30e 
  src/tests/persistent_volume_tests.cpp 
8651b2c5455089041f16d091c90a7e0d948191b8 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 54039: Added `recordio::transform` helper.

[Anand Mazumdar]

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


Fix it, then Ship it!





src/tests/common/recordio_tests.cpp (line 165)


s/the future/, the future


- Anand Mazumdar


On Nov. 28, 2016, 6:48 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54039/
> ---
> 
> (Updated Nov. 28, 2016, 6:48 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6473
> https://issues.apache.org/jira/browse/MESOS-6473
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This helper lets us transform raw bytes read from a RecordIO
> reader and write to a pipe.
> 
> 
> Diffs
> -
> 
>   src/common/recordio.hpp cbd9d3667ba43eabb22c1af20853b8ed8e093852 
>   src/tests/common/recordio_tests.cpp 
> 86a111724bd303bbb17b84d8b32b811476c66b57 
> 
> Diff: https://reviews.apache.org/r/54039/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 53995: Added API handler for ATTACH_CONTAINER_OUTPUT.

[Anand Mazumdar]

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



Looks good! Just some minor comments around style/questions around 
implementation.


src/internal/evolve.hpp (line 80)


Can you move this method after L78 to sort them by return type as we did 
for the other overloads in this file?

You would also need to move the method in the .cpp file



src/slave/http.cpp (line 89)


Move this above L88?



src/slave/http.cpp (line 2119)


4 space indent here



src/slave/http.cpp (line 2125)


Nit: s/'HOST'/The 'HOST'



src/slave/http.cpp (lines 2131 - 2132)


hmm, it's somewhat weird that we set it to `/api`. Why not `/switchboard` 
or anything else.

I would just set it to `/` i.e., the server root since currently i.e. the 
only path we support on the server. We can then make the switchboard 
implementation validate it. Is there an issue if we set it to `/`?



src/slave/http.cpp (line 2138)


Can you be explicit here? (You don't need access to `this` here and it can 
lead to subtle bugs in the future)



src/slave/http.cpp (line 2143)


Quotes before `ProcessIO`



src/slave/http.cpp (lines 2145 - 2148)


You might want to reorder these statements as:

```cpp
  Pipe pipe;
  Pipe::Writer writer = pipe.writer();
  
  OK ok;
  ok.reader = pipe.reader();
  
```



src/slave/http.cpp (line 2160)


Do you want to capture everything here?



src/slave/http.cpp (line 2168)


hmm, you would need to capture `Connection` here in this lambda. I don't 
quite understand how it works if you don't do that i.e., the destructor of 
`Connection` would be called since there is no reference to it remaining that 
would in turn result in the receiving side of the socket closing i.e., it won't 
accept any more streaming responses!



src/tests/api_tests.cpp (line 80)


Why do you need this using declaration and the corresponding header?



src/tests/api_tests.cpp 


hmm, why did you remove this using declaration?



src/tests/containerizer/mock_containerizer.hpp (lines 71 - 73)


hmm, not sure why did you need the change in this review? You don't seem to 
be using the `MockContainerizer` in your test?


- Anand Mazumdar


On Nov. 28, 2016, 10:50 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53995/
> ---
> 
> (Updated Nov. 28, 2016, 10:50 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6473
> https://issues.apache.org/jira/browse/MESOS-6473
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added API handler for ATTACH_CONTAINER_OUTPUT.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp e8b03c9f04b8a835f35ce7cb6cc44e3ecd799422 
>   src/internal/evolve.cpp 23a515c0aa8c31e0789fab84830dae32574604bc 
>   src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 
>   src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb 
>   src/slave/validation.cpp b3d8bcd0f0855d2a0a96821900d996ba86e11578 
>   src/tests/api_tests.cpp 8889e7807ecead736eaac3910332e86d594e8cec 
>   src/tests/containerizer/mock_containerizer.hpp 
> 7a30b8307b93b7bf549efb52d72367f652d0d95a 
> 
> Diff: https://reviews.apache.org/r/53995/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 50827: Windows: Disable modules tests and related helpers.

[Joseph Wu]

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

(Updated Nov. 28, 2016, 3:37 p.m.)


Review request for mesos, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, and 
Joris Van Remoortere.


Changes
---

Rebase on new container logger tests.


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


Repository: mesos


Description
---

Building and linking modules has some extra challenges on Windows
(see MESOS-5994).

This patch #ifdef's some module-specific tests on Windows.
NOTE: Most module tests are already parameterized to run the same code,
once as a built-in class and once as a module.


Diffs (updated)
-

  src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474 
  src/tests/container_logger_tests.cpp 6f69c08d305c971df2684db00910d485e8a38261 
  src/tests/cram_md5_authentication_tests.cpp 
56c249f48a09e86e1652362b54dca1fea5fc448b 
  src/tests/http_authentication_tests.cpp 
e73f21f7bd5ebb776ce5e950e1035182b32018ad 
  src/tests/main.cpp e1507bae8267a10c85e631d10f243f299ff81dc9 
  src/tests/master_authorization_tests.cpp 
4712361021708fff1ebdb5fa34386196c10c838e 
  src/tests/slave_authorization_tests.cpp 
b579ee9d8fefae54923cc3b202442eea43ad6639 

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


Testing
---

make check (OSX)

TODO: Test this on Linux and Windows.


Thanks,

Joseph Wu

Re: Review Request 53760: CMake: Changed example module output location and depedencies.

[Joseph Wu]

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

(Updated Nov. 28, 2016, 3:29 p.m.)


Review request for mesos, Alex Clemmer and Joris Van Remoortere.


Changes
---

Simplify the library output settings.  Instead of one property per project, we 
set it globally instead.


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


Repository: mesos


Description
---

This moves some test libraries from the default CMake build
output locations to `/src/.libs`, which is where
the Automake build outputs libraries.  We have a few tests
that expect modules to be located in this directory.

Additionally, this makes the test target (make check) depend
on the test modules and example frameworks, so that they are built
in the correct order.


Diffs (updated)
-

  src/CMakeLists.txt 9ff47d7a95c3baa5aa10039039e5ad065180ba45 
  src/tests/CMakeLists.txt 90b92251b7e133124f6d9a896190ceea402cbff1 

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


Testing
---

cmake ..
make clean
make


Thanks,

Joseph Wu

Re: Review Request 53753: CMake: Added variable for default linking strategy.

[Joseph Wu]

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

(Updated Nov. 28, 2016, 3:27 p.m.)


Review request for mesos, Alex Clemmer and Joris Van Remoortere.


Changes
---

Bump minimum cmake version from 2.8 to 2.8.10


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


Repository: mesos


Description
---

The `MESOS_DEFAULT_LIBRARY_LINKAGE` changes all Mesos libraries to
the use specified linking strategy (static or shared).  This does
not affect third-party libraries, which have their own requirements.

On Posix, the default is shared linkage.  On Windows the default
is static linkage.

This review replaces: https://reviews.apache.org/r/49862/


Diffs (updated)
-

  3rdparty/CMakeLists.txt 092184bc4e27661dafc76788485f70584c870a28 
  CMakeLists.txt 1362d65afc23816c34173866a3fdbce45936921d 
  cmake/MesosConfigure.cmake de04389b016b22bdff69e94d37fad4eedbda5874 
  src/CMakeLists.txt 9ff47d7a95c3baa5aa10039039e5ad065180ba45 
  src/slave/qos_controllers/CMakeLists.txt 
87c92af21c012655c201c01cd4ba5ff912555119 
  src/slave/resource_estimators/CMakeLists.txt 
17b149f734ea9dc8ac4c5dd45bdb8312faf4cc77 

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


Testing
---

cmake ..
make


Thanks,

Joseph Wu

Re: Review Request 53961: CMake: Move test-specific variables into StoutTestsConfigure.

[Joseph Wu]

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

(Updated Nov. 28, 2016, 3:26 p.m.)


Review request for mesos, Alex Clemmer and Joris Van Remoortere.


Changes
---

Rephrase a comment so that it fits under 80 characters.


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


Repository: mesos


Description (updated)
---

These definitions were originally in:
`3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake`

By consolidating these definitions in `StoutTestsConfigure`, we can
reuse the same definitions for libprocess and Mesos tests as well.

This also rephrases/reformats some of the moved comments to get 
them below 80 characters per line.


Diffs (updated)
-

  3rdparty/stout/cmake/StoutTestsConfigure.cmake 
2c786c53008d071a44137aa9507794e9860ed6b6 

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


Testing
---

See next review.


Thanks,

Joseph Wu

Re: Review Request 53764: CMake: Added a target for the default executor.

[Joseph Wu]

> On Nov. 28, 2016, 11:13 a.m., Alex Clemmer wrote:
> > src/launcher/CMakeLists.txt, line 48
> > 
> >
> > Are we deleting this line because this is included also in the `src/` 
> > directory? Do you think there is any benefit in having submodules include 
> > everything they need to build, themselves, rather than depending on the 
> > `src/` directory to set it up?

There isn't any real benefit, unless we greatly rearrange the codebase.

With our current codebase, most files implicitly expect to have access to 
*internal* headers from the top level of the source.  This means we don't have 
a very strong separation of components in the codebase (the agent imports 
master things; the master imports agent things; etc).  But at the same time, it 
also makes development simpler, as we don't have to mess with the build system 
to add extra includes.

---

As for why I've deleted this line, the CMake "environment" is inherited by 
children files, but not the other way around.  By importing in multiple places, 
we are just adding code bloat, with no real change in functionality.  It would 
be a different story if each `include_directories` line had something besides 
`${AGENT_INCLUDE_DIRS}` (which is defined globally).


- Joseph


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


On Nov. 14, 2016, 7:15 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53764/
> ---
> 
> (Updated Nov. 14, 2016, 7:15 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This executor has support for TaskGroups and will eventually take
> precedence over the default command executor.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake de04389b016b22bdff69e94d37fad4eedbda5874 
>   src/launcher/CMakeLists.txt 3137ea1479970c7ea46e6595536d481002fc585c 
> 
> Diff: https://reviews.apache.org/r/53764/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53695: Allows caching extractable files when outputFile is set.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53695]

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

- Mesos ReviewBot


On Nov. 28, 2016, 7:14 p.m., Stephen Hankinson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53695/
> ---
> 
> (Updated Nov. 28, 2016, 7:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6587
> https://issues.apache.org/jira/browse/MESOS-6587
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows caching extractable files when outputFile is set.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f 
>   src/tests/fetcher_cache_tests.cpp 03e817d46194d451eceb70f4cebb54dfdcb4c2e7 
> 
> Diff: https://reviews.apache.org/r/53695/diff/
> 
> 
> Testing
> ---
> 
> A new test, FetcherCacheTest.LocalCachedExtractCustomOutputFile, was created 
> to confirm that when a URI is created with cached: true, extract: true, a 
> custom outputFile with a proper compression suffix set, and an original URI 
> that doesn't end with a proper compression suffix fails to extract properly 
> from the cache.
> 
> The test makes a copy of the test archive in the cache but appends "misc" to 
> the end, forming an invalid compression suffix, causing 
> mesos-fetcher-test-archive.tgz to become mesos-fetcher-test-archive.tgzmisc. 
> The URI path is set to this modified location. The outputFile of the URI is 
> set ARCHIVE_NAME which ends with a valid compression suffix, so therefore 
> should extract properly.
> 
> At the end of the test the temporary cached archive file 
> (mesos-fetcher-test-archive.tgzmisc) is deleted.
> 
> Before the patch was written this test was failing as expected. After 
> implementing the patch, this and all other tests were passing.
> 
> 
> Thanks,
> 
> Stephen Hankinson
> 
>

Re: Review Request 54102: Moved the default PATH setting to containerizer.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Nov. 27, 2016, 7:48 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54102/
> ---
> 
> (Updated Nov. 27, 2016, 7:48 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Kevin Klues.
> 
> 
> Bugs: MESOS-6648
> https://issues.apache.org/jira/browse/MESOS-6648
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, it is set as part of the executor environment. This is not
> ideal because isolators might choose to set environment variables for
> the contianer. We should set the default PATH in the last step (right
> before launch) if it does not exist.
> 
> Mesos containerizer launch helper already does default PATH setting
> right before launching the container. This patch did the same in
> Docker containerizer, and remove the default PATH setting when
> generating the executor environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp a8e522fc058f50560e8ec162c31be079e620bf9d 
>   src/slave/slave.cpp 521f08d59cd78f9089d58cd3294f0ee4a099cd7f 
> 
> Diff: https://reviews.apache.org/r/54102/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53995: Added API handler for ATTACH_CONTAINER_OUTPUT.

[Vinod Kone]

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

(Updated Nov. 28, 2016, 10:50 p.m.)


Review request for mesos, Anand Mazumdar and Benjamin Mahler.


Changes
---

updated the diff per changs in the dependent reviews.


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


Repository: mesos


Description
---

Added API handler for ATTACH_CONTAINER_OUTPUT.


Diffs (updated)
-

  src/internal/evolve.hpp e8b03c9f04b8a835f35ce7cb6cc44e3ecd799422 
  src/internal/evolve.cpp 23a515c0aa8c31e0789fab84830dae32574604bc 
  src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 
  src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb 
  src/slave/validation.cpp b3d8bcd0f0855d2a0a96821900d996ba86e11578 
  src/tests/api_tests.cpp 8889e7807ecead736eaac3910332e86d594e8cec 
  src/tests/containerizer/mock_containerizer.hpp 
7a30b8307b93b7bf549efb52d72367f652d0d95a 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 54101: Removed MesosContainerizerLaunchTest.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Nov. 27, 2016, 7:48 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54101/
> ---
> 
> (Updated Nov. 27, 2016, 7:48 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Kevin Klues.
> 
> 
> Bugs: MESOS-6648
> https://issues.apache.org/jira/browse/MESOS-6648
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test is not necessary as we already test that in linux filesystem
> isolator tests. This patch removes it.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am dd1626d177b38a6613f18f32bb0668abbb5100e0 
>   src/tests/containerizer/launch_tests.cpp 
> 680c147198c84879c6ab728c65ea686379b216b7 
> 
> Diff: https://reviews.apache.org/r/54101/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54099: Used environment to pass flags to launch helper.

[Kevin Klues]

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




src/slave/containerizer/mesos/containerizer.cpp (lines 1548 - 1559)


Hmm. As it is written, an environment variable set on the agent will 
override any environment variables we set up via our launchFlags. Is this the 
intended behaviour?

It seems like this could lead to unexpected consequences if (for example), 
we happen to have `MESOS_CONTAINERIZER_USER=root` set when we launch our agent.


- Kevin Klues


On Nov. 27, 2016, 7:47 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54099/
> ---
> 
> (Updated Nov. 27, 2016, 7:47 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Kevin Klues.
> 
> 
> Bugs: MESOS-6648
> https://issues.apache.org/jira/browse/MESOS-6648
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of doing that just for command environment variables, this
> patch does it consistently by using environment variables to pass
> launch helper flags. This will be more secure.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9b33495d9babc3ee489a8712fe1977746c41043f 
> 
> Diff: https://reviews.apache.org/r/54099/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54100: Removed two disabled shared filesystem isolator tests.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Nov. 27, 2016, 7:48 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54100/
> ---
> 
> (Updated Nov. 27, 2016, 7:48 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Kevin Klues.
> 
> 
> Bugs: MESOS-6648
> https://issues.apache.org/jira/browse/MESOS-6648
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Shared filesystem isolator has been deprecated for a while, and those
> two tests are disabled all the time. This patch removes them.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/isolator_tests.cpp 
> 9766aaf144722b18d88f694ff37ffd53974cb60d 
> 
> Diff: https://reviews.apache.org/r/54100/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54098: Moved a VLOG to a proper place in MesosContainerizer.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Nov. 27, 2016, 7:47 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54098/
> ---
> 
> (Updated Nov. 27, 2016, 7:47 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Kevin Klues.
> 
> 
> Bugs: MESOS-6648
> https://issues.apache.org/jira/browse/MESOS-6648
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We should print the VLOG after we calculate the launch flags.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9b33495d9babc3ee489a8712fe1977746c41043f 
> 
> Diff: https://reviews.apache.org/r/54098/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54097: Added support to export the flags as environment variables.

[Kevin Klues]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/flags/flags.hpp (line 302)


The name `exportEnvironment` is misleading. It makes me think this function 
is actually going to export flags as environment variables in the current 
environment.

What it's actually doing is taking the flags and building a string based 
key/value map out of them.

I would rename this function to something like `buildEnvironment` or even 
`buildKeyValueMap` instead of `export*`.


- Kevin Klues


On Nov. 27, 2016, 7:47 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54097/
> ---
> 
> (Updated Nov. 27, 2016, 7:47 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Kevin Klues.
> 
> 
> Bugs: MESOS-6648
> https://issues.apache.org/jira/browse/MESOS-6648
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch added a method to FlagsBase to export the flags as a map of
> environment variables. This simplifies the logic when the caller wants
> to pass flags as environment variables to the subprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/flags/flags.hpp 
> 06aac13836fbe7f1d970476f073a48e9f4bdb168 
>   3rdparty/stout/tests/flags_tests.cpp 
> 9ee538bd0afa3fb3b6debc6eda010552d9062ad7 
> 
> Diff: https://reviews.apache.org/r/54097/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53761: CMake: Add a target between MESOS_TARGET and MESOS_PROTOBUFs.

[Joseph Wu]

> On Nov. 28, 2016, 11:04 a.m., Alex Clemmer wrote:
> > cmake/MesosConfigure.cmake, line 142
> > 
> >
> > Interesting, for my own education: is this the same in the AC build 
> > solution? I don't see a target like this in the AC system, but I also don't 
> > really know what to search for.

The automake build has some special variables that automatically add certain 
things to the build.  The various programs like `mesos-agent` and 
`mesos-fetcher` are added to a `sbin_PROGRAMS` or `pkglibexec_PROGRAMS` target. 
 These targets are implicit dependencies of the "default" make target (which is 
an implicit dependency on the make check target).

See: 
https://www.gnu.org/software/automake/manual/html_node/The-Two-Parts-of-Install.html

In CMake, there are no such implicit targets, so we end up creating "custom" 
targets to account for dependency chains.  The `mesos-and-binaries` target is 
my replacement for automake's implicit "default" target.


- Joseph


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


On Nov. 14, 2016, 7:11 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53761/
> ---
> 
> (Updated Nov. 14, 2016, 7:11 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We need this extra level of targets in order to model the
> dependency chain for binaries and libraries.  An example
> is the mesos-containerizer binary, which depends on libmesos
> (i.e. MESOS_TARGET) but does not get included in the dependency
> chain if you only depend on MESOS_TARGET.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake de04389b016b22bdff69e94d37fad4eedbda5874 
> 
> Diff: https://reviews.apache.org/r/53761/diff/
> 
> 
> Testing
> ---
> 
> See next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 54096: Fixed an indentation problem in Subcommand.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Nov. 27, 2016, 7:46 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54096/
> ---
> 
> (Updated Nov. 27, 2016, 7:46 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Kevin Klues.
> 
> 
> Bugs: MESOS-6648
> https://issues.apache.org/jira/browse/MESOS-6648
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed an indentation problem in Subcommand.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/subcommand.hpp 
> 930e3c979eaa638342018e59efea6b0ecf262c02 
> 
> Diff: https://reviews.apache.org/r/54096/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54095: Removed an unused function in launch helper.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Nov. 27, 2016, 7:46 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54095/
> ---
> 
> (Updated Nov. 27, 2016, 7:46 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Kevin Klues.
> 
> 
> Bugs: MESOS-6648
> https://issues.apache.org/jira/browse/MESOS-6648
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed an unused function in launch helper.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.hpp 
> 155e3c5a27b8c710971ee4b508600d3b5589a2e0 
> 
> Diff: https://reviews.apache.org/r/54095/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54049: Aliased `process::http` namespace to `http`.

[Vinod Kone]

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

(Updated Nov. 28, 2016, 10:22 p.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

updated summary.


Summary (updated)
-

Aliased `process::http` namespace to `http`.


Repository: mesos


Description (updated)
---

This clarifies usage of objects from different namespaces,
e.g., http::Request vs mesos::Request.


Diffs (updated)
-

  src/tests/api_tests.cpp 8889e7807ecead736eaac3910332e86d594e8cec 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 53994: Added streaming support to `/api/v1` handler on the agent.

[Vinod Kone]

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

(Updated Nov. 28, 2016, 10:21 p.m.)


Review request for mesos, Anand Mazumdar and Benjamin Mahler.


Changes
---

anand's comments. NNFR.


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


Repository: mesos


Description
---

Note that this change only updates the handler to correctly
handle non-streaming calls given it receives `PIPE` requests.
Handling streaming calls will come later.


Diffs (updated)
-

  src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 
  src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb 
  src/slave/slave.cpp 521f08d59cd78f9089d58cd3294f0ee4a099cd7f 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 53994: Added streaming support to `/api/v1` handler on the agent.

[Vinod Kone]

> On Nov. 28, 2016, 9:28 p.m., Anand Mazumdar wrote:
> > src/slave/http.cpp, line 394
> > 
> >
> > Do you need to implicitly capture everything or can we be explicit here?

originally i was capturing explicitly, but when i looked around it seems like 
we use "=" when the capture includes "this".


- Vinod


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


On Nov. 28, 2016, 10:21 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53994/
> ---
> 
> (Updated Nov. 28, 2016, 10:21 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6473
> https://issues.apache.org/jira/browse/MESOS-6473
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Note that this change only updates the handler to correctly
> handle non-streaming calls given it receives `PIPE` requests.
> Handling streaming calls will come later.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 
>   src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb 
>   src/slave/slave.cpp 521f08d59cd78f9089d58cd3294f0ee4a099cd7f 
> 
> Diff: https://reviews.apache.org/r/53994/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 53760: CMake: Changed example module output location and depedencies.

[Joseph Wu]

> On Nov. 28, 2016, 11:03 a.m., Alex Clemmer wrote:
> > src/slave/container_loggers/CMakeLists.txt, line 33
> > 
> >
> > Minor nit: probably want to move this directory to a variable?

Good point.  Instead of setting target properties in every CMakeLists, I can 
just add this to the top level CMakeLists:
```
# Generate all libraries in the same folder to conform with the Automake build.
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/src/.libs)
if (WIN32)
  SET(CMAKE_LIBRARY_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/src/.libs)
  SET(CMAKE_LIBRARY_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/src/.libs)
endif (WIN32)
```


- Joseph


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


On Nov. 15, 2016, 3:09 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53760/
> ---
> 
> (Updated Nov. 15, 2016, 3:09 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This moves some test libraries from the default CMake build
> output locations to `/src/.libs`, which is where
> the Automake build outputs libraries.  We have a few tests
> that expect modules to be located in this directory.
> 
> Additionally, this makes the test target (make check) depend
> on the test modules and example frameworks, so that they are built
> in the correct order.
> 
> 
> Diffs
> -
> 
>   src/examples/CMakeLists.txt PRE-CREATION 
>   src/slave/container_loggers/CMakeLists.txt PRE-CREATION 
>   src/slave/qos_controllers/CMakeLists.txt 
> 87c92af21c012655c201c01cd4ba5ff912555119 
>   src/slave/resource_estimators/CMakeLists.txt 
> 17b149f734ea9dc8ac4c5dd45bdb8312faf4cc77 
>   src/tests/CMakeLists.txt cf583ea3f10482b510459fb11a7ecaf76e498391 
> 
> Diff: https://reviews.apache.org/r/53760/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make clean
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53460: Refactored network::Address into inet::Address.

[Jie Yu]

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/address.hpp (line 69)


Ditto.



3rdparty/libprocess/include/process/address.hpp (line 80)


Use switch consistently as other functions?



3rdparty/libprocess/include/process/address.hpp (lines 80 - 81)


This won't compile on windows. Looks like AF_UNIX is not even defined on 
windows:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms740506(v=vs.85).aspx

We should properly wrap UNIX with `#ifndef __WINDOWS__`



3rdparty/libprocess/include/process/address.hpp (line 108)


Wrap with `#ifndef __WINDOWS__`



3rdparty/libprocess/include/process/address.hpp (line 129)


+1. We should check on path length here.

Probably introduce a 'create' function?



3rdparty/libprocess/include/process/address.hpp (lines 133 - 140)


+1 on using union as suggested by James.



3rdparty/libprocess/include/process/address.hpp (line 143)


Maybe you can store a `network::Address` here?



3rdparty/libprocess/include/process/address.hpp (line 167)


This is non-POD global constant which might cause trouble during global 
teardown.

We should either use constexpr here (making sure Address's constructor is 
constexpr constructor), or use static method as we did before.



3rdparty/libprocess/include/process/address.hpp (line 169)


Ditto.



3rdparty/libprocess/include/process/address.hpp (lines 223 - 231)


+1 on using a union as suggested by James.



3rdparty/libprocess/include/process/socket.hpp (line 432)


inline is not necessary for template specification? Let's be consistent 
here (either use inline for all of them, or none of them).



3rdparty/libprocess/src/socket.cpp (lines 56 - 59)


`ifndef WINDOWS`



3rdparty/libprocess/src/tests/socket_tests.cpp (line 39)


Looks like this only works on Linux? should we use a socket path in temp 
dir. Maybe use TemporaryDirectoryTest fixture and use 
`path::join(sandbox.get(), "socket")` as the path?



3rdparty/libprocess/src/tests/ssl_tests.cpp (line 51)


wrap with ifdef WINDOWS


- Jie Yu


On Nov. 28, 2016, 6:15 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53460/
> ---
> 
> (Updated Nov. 28, 2016, 6:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added unix::Address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
>   3rdparty/libprocess/include/process/address.hpp 
> 04e3155d65f476208fa852e83b79d173b66288fd 
>   3rdparty/libprocess/include/process/network.hpp 
> 52110667185370a4c92e2fa524819ab1f34bdec9 
>   3rdparty/libprocess/include/process/socket.hpp 
> f798af7879546d71e8ef4a295c9cf489a70cb61f 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 21a0fc45b55a368a21b3e616c751ab43eebd4902 
>   3rdparty/libprocess/src/address.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 5c0929d3d9f5595bd2f343b98b899fd6b06a67b2 
>   3rdparty/libprocess/src/poll_socket.cpp 
> eb7b48713edd30b545d7be95b5d51b0f71bd422a 
>   3rdparty/libprocess/src/process.cpp 
> e9a4bbb0b2410e0260d120b97e73972c94eb0f26 
>   3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
>   3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 55c8c309571b1892b0acc4d766eda9bb98085a6f 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 
> 
> Diff: https://reviews.apache.org/r/53460/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53759: CMake: Change libprocess to a shared library.

[Joseph Wu]

> On Nov. 17, 2016, 9:09 a.m., Alex Clemmer wrote:
> > 3rdparty/libprocess/src/CMakeLists.txt, line 93
> > 
> >
> > Seems like this should be using `MESOS_DEFAULT_LIBRARY_LINKAGE`?
> 
> Joseph Wu wrote:
> Nope, Mesos variables shouldn't be used in stout/libprocess.
> 
> Alex Clemmer wrote:
> Right. I suppose my real question is: should this be a variable of some 
> sort? It seems odd we'd use one elsewhere, but not for Stout.

For the 3rdparty libraries, libprocess is the only one where we explicitly 
control the linking strategy.  (Stout is header-only and bundled dependencies 
have their own build systems.)  I suppose the HTTP parser library also counts, 
but this one is also built as an external project, meaning that CMake variables 
will not be inherited.

In the automake build, there is an option to build libprocess as a static 
library, but that build path is untested (and completely broken :).  We can 
consider adding an equivalent variable in future, but for now, it is safer not 
to.


- Joseph


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


On Nov. 14, 2016, 7:09 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53759/
> ---
> 
> (Updated Nov. 14, 2016, 7:09 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to load modules that are themselves based on libprocess,
> we must link libprocess as a shared library.  Since modules are
> only supported on non-Windows platforms, this changes the default
> linking mode to SHARED on non-Windows.
> 
> This review replaces: https://reviews.apache.org/r/49924/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/CMakeLists.txt 
> d1547ef6a8762385f653d3824307727e4d0a7e71 
> 
> Diff: https://reviews.apache.org/r/53759/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53753: CMake: Added variable for default linking strategy.

[Alex Clemmer]

> On Nov. 28, 2016, 6:46 p.m., Alex Clemmer wrote:
> > cmake/MesosConfigure.cmake, line 71
> > 
> >
> > Hmm. I think this is not available in CMake 2.8.0, which is the minimum 
> > version of CMake we support? In the documentation for 2.8.7[1] it's not 
> > clear to me that this variable is implemented, but in 2.8.11, it is[2]. 
> > Originally we decided to support 2.8.0 because the Ubuntu CMake packages 
> > going back to version Ubuntu 12 support CMake 2.8.0 or later. These days it 
> > looks like 12 supports CMake 2.8.7, while 14 supports 2.8.11. So, we could 
> > bump the version here, but then we should update `cmake_minimum_required`. 
> > We should do this with the conscious understanding that we're not 
> > supporting 12's default CMake installation, which makes the barrier to 
> > entry higher.
> > 
> > [1] https://cmake.org/cmake/help/v2.8.7/cmake.html
> > [2] https://cmake.org/cmake/help/v2.8.11/cmake.html
> 
> Joseph Wu wrote:
> Here's what I'm considering:
> 
> 1) By setting `CMAKE_POSITION_INDEPENDENT_CODE` globally, we can reduce 
> logic in 3 existing places where we build an intermediate library.  I have 
> some plans (in my head) to increase the number of intermediate libraries so 
> that we don't have a gigantic libmesos.
> 2) 2.8.10 is the earliest version which includes the 
> `CMAKE_POSITION_INDEPENDENT_CODE` option.  Increasing the burden for using 
> these old distros is not exactly a problem, as cmake works on those old 
> distros.  There are already barriers for all the old distros, CentOS 6 
> especially (which requires you to download rpms from some really arbitrary 
> locations).
> 
> In total, I'm in favor of bumping up the required CMake version.

I'm completely fine with bumping the version. Just wanted to make sure we make 
the decision consciously. :)


- Alex


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


On Nov. 15, 2016, 3:08 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53753/
> ---
> 
> (Updated Nov. 15, 2016, 3:08 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `MESOS_DEFAULT_LIBRARY_LINKAGE` changes all Mesos libraries to
> the use specified linking strategy (static or shared).  This does
> not affect third-party libraries, which have their own requirements.
> 
> On Posix, the default is shared linkage.  On Windows the default
> is static linkage.
> 
> This review replaces: https://reviews.apache.org/r/49862/
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 770a3828a6e0ffaa4f185392fdc1a2152446449d 
>   cmake/MesosConfigure.cmake de04389b016b22bdff69e94d37fad4eedbda5874 
>   src/CMakeLists.txt aef9ae6d2872dc15725c01ce85b657965485605f 
>   src/slave/qos_controllers/CMakeLists.txt 
> 87c92af21c012655c201c01cd4ba5ff912555119 
>   src/slave/resource_estimators/CMakeLists.txt 
> 17b149f734ea9dc8ac4c5dd45bdb8312faf4cc77 
> 
> Diff: https://reviews.apache.org/r/53753/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53758: CMake: Added test sources to the build.

[Joseph Wu]

> On Nov. 28, 2016, 11:01 a.m., Alex Clemmer wrote:
> > src/tests/CMakeLists.txt, line 79
> > 
> >
> > Just so I'm clear about this: in other places, we're disabling source 
> > builds on WIN32 by `if`'ing out the source inclusion in the file, but here 
> > (because of legacy reasons) we're depending on the "old way" of doing 
> > stuff, which is to `if` out the `add_subdirectory(tests/)`?
> > 
> > If so, it would be nice to have a transition plan going in -- either 
> > one of us should clean it up (either in this chain or after it), or we 
> > should file an issue, I think. Less desirable would be to have two 
> > different conventions. _i.e._ I personally don't consider the changes 
> > required to be a stop-ship, but I do consider having a plan to be a stop 
> > ship. :)

As it stands, so few of the Mesos tests actually build on Windows, so it's not 
too useful to remove the `if (WIN32)`.  
The plan should be:

1) We get the CMake build, with tests, working on OSX and Linux (this chain).
2) We get the tests working on Windows, with a separate chain (presumably 
starting https://reviews.apache.org/r/53550/ ).  We will most likely add some 
more `if (WIN32)` blocks into the test/CMakeLists.txt file during this process.
3) We'll have a single review that removes the `if (WIN32)` around 
`add_subdirectory(tests/)` once (2) is complete.  In the meantime, it's trivial 
for us contributors to carry around this commit.


- Joseph


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


On Nov. 22, 2016, noon, Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53758/
> ---
> 
> (Updated Nov. 22, 2016, noon)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds most existing tests to the CMake build.  This excludes
> a few special cases, just as Java, Python, and network isolator
> tests.
> 
> This review replaces: https://reviews.apache.org/r/49921/
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 1362d65afc23816c34173866a3fdbce45936921d 
>   src/CMakeLists.txt aef9ae6d2872dc15725c01ce85b657965485605f 
>   src/tests/CMakeLists.txt 90b92251b7e133124f6d9a896190ceea402cbff1 
>   src/tests/cmake/MesosTestsConfigure.cmake 
> 3f543c010ae87ff04e6b45745bc49ef65b6590ff 
> 
> Diff: https://reviews.apache.org/r/53758/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53753: CMake: Added variable for default linking strategy.

[Joseph Wu]

> On Nov. 28, 2016, 10:46 a.m., Alex Clemmer wrote:
> > cmake/MesosConfigure.cmake, line 71
> > 
> >
> > Hmm. I think this is not available in CMake 2.8.0, which is the minimum 
> > version of CMake we support? In the documentation for 2.8.7[1] it's not 
> > clear to me that this variable is implemented, but in 2.8.11, it is[2]. 
> > Originally we decided to support 2.8.0 because the Ubuntu CMake packages 
> > going back to version Ubuntu 12 support CMake 2.8.0 or later. These days it 
> > looks like 12 supports CMake 2.8.7, while 14 supports 2.8.11. So, we could 
> > bump the version here, but then we should update `cmake_minimum_required`. 
> > We should do this with the conscious understanding that we're not 
> > supporting 12's default CMake installation, which makes the barrier to 
> > entry higher.
> > 
> > [1] https://cmake.org/cmake/help/v2.8.7/cmake.html
> > [2] https://cmake.org/cmake/help/v2.8.11/cmake.html

Here's what I'm considering:

1) By setting `CMAKE_POSITION_INDEPENDENT_CODE` globally, we can reduce logic 
in 3 existing places where we build an intermediate library.  I have some plans 
(in my head) to increase the number of intermediate libraries so that we don't 
have a gigantic libmesos.
2) 2.8.10 is the earliest version which includes the 
`CMAKE_POSITION_INDEPENDENT_CODE` option.  Increasing the burden for using 
these old distros is not exactly a problem, as cmake works on those old 
distros.  There are already barriers for all the old distros, CentOS 6 
especially (which requires you to download rpms from some really arbitrary 
locations).

In total, I'm in favor of bumping up the required CMake version.


- Joseph


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


On Nov. 14, 2016, 7:08 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53753/
> ---
> 
> (Updated Nov. 14, 2016, 7:08 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `MESOS_DEFAULT_LIBRARY_LINKAGE` changes all Mesos libraries to
> the use specified linking strategy (static or shared).  This does
> not affect third-party libraries, which have their own requirements.
> 
> On Posix, the default is shared linkage.  On Windows the default
> is static linkage.
> 
> This review replaces: https://reviews.apache.org/r/49862/
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 770a3828a6e0ffaa4f185392fdc1a2152446449d 
>   cmake/MesosConfigure.cmake de04389b016b22bdff69e94d37fad4eedbda5874 
>   src/CMakeLists.txt aef9ae6d2872dc15725c01ce85b657965485605f 
>   src/slave/qos_controllers/CMakeLists.txt 
> 87c92af21c012655c201c01cd4ba5ff912555119 
>   src/slave/resource_estimators/CMakeLists.txt 
> 17b149f734ea9dc8ac4c5dd45bdb8312faf4cc77 
> 
> Diff: https://reviews.apache.org/r/53753/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53961: CMake: Move test-specific variables into StoutTestsConfigure.

[Joseph Wu]

> On Nov. 28, 2016, 10:46 a.m., Alex Clemmer wrote:
> > 3rdparty/stout/cmake/StoutTestsConfigure.cmake, line 20
> > 
> >
> > Super nit, and technically not your problem since you're moving this, 
> > but: I think this line is over 80 characters?

We're not as picky in the build files, but I shall rephrase to squeeze it in :)


> On Nov. 28, 2016, 10:46 a.m., Alex Clemmer wrote:
> > 3rdparty/stout/cmake/StoutTestsConfigure.cmake, line 44
> > 
> >
> > Not your fault, but I believe this is not true with our current version 
> > of gtest. :)

These flags are still very much required (on OSX and BSD apparently).

The flags were last updated about a year ago: 
https://github.com/apache/mesos/commit/a8970ce78f1abbee3dd4de282d54ced38d0e5fa1


- Joseph


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


On Nov. 21, 2016, 2:08 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53961/
> ---
> 
> (Updated Nov. 21, 2016, 2:08 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These definitions were originally in:
> `3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake`
> 
> By consolidating these definitions in `StoutTestsConfigure`, we can
> reuse the same definitions for libprocess and Mesos tests as well.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/cmake/StoutTestsConfigure.cmake 
> 2c786c53008d071a44137aa9507794e9860ed6b6 
> 
> Diff: https://reviews.apache.org/r/53961/diff/
> 
> 
> Testing
> ---
> 
> See next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53994: Added streaming support to `/api/v1` handler on the agent.

[Anand Mazumdar]

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


Fix it, then Ship it!




Looks good, just some minor comments.


src/slave/http.cpp (lines 335 - 356)


hmm, we should kill this block and use `deserialize()` in 
`src/common/http.hpp` directly. It seems that we never cleaned this block up 
after implementing `deserialize()`. This would ensure that we keep the 
deserialization logic in one place.

Note that this applies to the other handlers on the master/agent too and we 
might want to do a sweep later.



src/slave/http.cpp (lines 354 - 355)


Can it ever go inside this `else` block provided that we already checked 
for content type being JSON/PROTOBUF before?

I would just put an `UNREACHABLE()` here. Also, we might not need this 
`else` block if you replace this block with `deserialize()` as per my earlier 
comment.



src/slave/http.cpp (line 390)


Do you need to implicitly capture everything or can we be explicit here?


- Anand Mazumdar


On Nov. 28, 2016, 8:56 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53994/
> ---
> 
> (Updated Nov. 28, 2016, 8:56 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6473
> https://issues.apache.org/jira/browse/MESOS-6473
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Note that this change only updates the handler to correctly
> handle non-streaming calls given it receives `PIPE` requests.
> Handling streaming calls will come later.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 
>   src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb 
>   src/slave/slave.cpp 521f08d59cd78f9089d58cd3294f0ee4a099cd7f 
> 
> Diff: https://reviews.apache.org/r/53994/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 53962: CMake: Remove test-specific variables from ProcessTestsConfigure.

[Joseph Wu]

> On Nov. 28, 2016, 10:46 a.m., Alex Clemmer wrote:
> > 3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake, line 18
> > 
> >
> > Just so I'm clear: this comment is being removed because we're moving 
> > away from the model where we `include(ProcessTestsConfigure)` and instead 
> > `include(StoutTestsConfigure)`? That's fine, I just want to be clear about 
> > it.

Exactly.  The comment is moved as well.


- Joseph


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


On Nov. 21, 2016, 2:08 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53962/
> ---
> 
> (Updated Nov. 21, 2016, 2:08 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The removed definitions are now defined in:
> `3rdparty/stout/cmake/StoutTestsConfigure.cmake`
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 
> c73c7a4960243917ad21f2d8a529575157eefa32 
> 
> Diff: https://reviews.apache.org/r/53962/diff/
> 
> 
> Testing
> ---
> 
> See next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 54053: Updated 'io::redirect()' to take an optional vector of callback hooks.

[Kevin Klues]

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

(Updated Nov. 28, 2016, 9:18 p.m.)


Review request for mesos, Benjamin Mahler and Jie Yu.


Changes
---

Updated to address Jie's comments.


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


Repository: mesos


Description
---

These callback hooks will be invoked before passing any data read from
the 'from' file descriptor on to the 'to' file descriptor.


Diffs (updated)
-

  3rdparty/libprocess/include/process/io.hpp 
eec5efd7e6b71a783f2bb40826054d0488cee71f 
  3rdparty/libprocess/src/io.cpp d930ceebc90468e082b984e41385549f906dc6ae 
  3rdparty/libprocess/src/tests/io_tests.cpp 
66b610e35fa9f6a1738c77d181d76dca3921e6fb 

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


Testing
---

GTEST_FILTER="" make -j40 check
GTEST_FILTER="IOTest.Redirect" 3rdparty/libprocess/libprocess-tests


Thanks,

Kevin Klues

Re: Review Request 50974: Documented all the API calls for Operator HTTP API.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50974]

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

- Mesos ReviewBot


On Nov. 28, 2016, 6:50 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50974/
> ---
> 
> (Updated Nov. 28, 2016, 6:50 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5992
> https://issues.apache.org/jira/browse/MESOS-5992
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented all the API calls for Operator HTTP API.
> 
> 
> Diffs
> -
> 
>   docs/operator-http-api.md 4f4c39e7b4b6de32af1933c34eba21f126fae8ac 
> 
> Diff: https://reviews.apache.org/r/50974/diff/
> 
> 
> Testing
> ---
> 
> Checked the generated page through "rake dev".
> Validated and formatted all the JSON snippets with:
> http://jsonlint.com/
> http://jsonviewer.stack.hu/
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 53825: MESOS-6597 Enabled java protos generation for all V1 proto files.

[Anand Mazumdar]

> On Nov. 20, 2016, 9:04 p.m., Anand Mazumdar wrote:
> > Nice first patch and welcome to the community!
> > 
> > - Can you update the Testing Done section with details on testing?
> > - We also support Python bindings. Do you mind adding these protos to our 
> > python build too in a follow up review? (See my comments later on how you 
> > can use review dependencies if you are new to ReviewBoard)
> 
> Vijay Srinivasaraghavan wrote:
> Thanks Anand. I have made the python binding changes locally on my 
> machine. Do you want me to create a new JIRA bug for this issue and create a 
> new review or add the patch to this JIRA/review itself? I am little confused 
> as to how the dependencies are handled with RB.
> 
> Anand Mazumdar wrote:
> No need to create a separate issue. It would be fine to create a new 
> review with it only containing the python related changes. This would be 
> another commit based on top of this one. You can then use `post-reviews.py` 
> and it should handle setting the dependencies for you. Feel free to ping me 
> on IRC/Slack if you run into any issues (anandm)
> 
> Vijay Srinivasaraghavan wrote:
> Thanks for the clarification. I have attached a new patch with "53825" as 
> dependendency
> 
> Vijay Srinivasaraghavan wrote:
> I have created two patch for those additional issues that was addressed 
> as part of the original request. Please let me know if you are waiting for 
> anything from my end before this can be merged? Thanks  
> https://reviews.apache.org/r/54014/
> https://reviews.apache.org/r/54015/

Sorry for the delay. I would get to them sometime later this week. There is 
nothing needed from your end for now.


- Anand


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


On Nov. 23, 2016, 5:46 a.m., Vijay Srinivasaraghavan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53825/
> ---
> 
> (Updated Nov. 23, 2016, 5:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zameer Manji.
> 
> 
> Bugs: MESOS-6597
> https://issues.apache.org/jira/browse/MESOS-6597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6597 Enabled java protos generation for all V1 proto files.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5e0b8406f7f624bd8b03ff76b887f20e22fc66e0 
> 
> Diff: https://reviews.apache.org/r/53825/diff/
> 
> 
> Testing
> ---
> 
> Built mesos and confirmed jar file containing new java PB wrappers for V1 
> API. Ran standard unit test (make tests)
> 
> 
> Thanks,
> 
> Vijay Srinivasaraghavan
> 
>

Re: Review Request 53994: Added streaming support to `/api/v1` handler on the agent.

[Vinod Kone]

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

(Updated Nov. 28, 2016, 8:56 p.m.)


Review request for mesos, Anand Mazumdar and Benjamin Mahler.


Changes
---

updated refactoring per comments and offline discussion.


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


Repository: mesos


Description
---

Note that this change only updates the handler to correctly
handle non-streaming calls given it receives `PIPE` requests.
Handling streaming calls will come later.


Diffs (updated)
-

  src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 
  src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb 
  src/slave/slave.cpp 521f08d59cd78f9089d58cd3294f0ee4a099cd7f 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 53825: MESOS-6597 Enabled java protos generation for all V1 proto files.

[Vijay Srinivasaraghavan]

> On Nov. 20, 2016, 9:04 p.m., Anand Mazumdar wrote:
> > Nice first patch and welcome to the community!
> > 
> > - Can you update the Testing Done section with details on testing?
> > - We also support Python bindings. Do you mind adding these protos to our 
> > python build too in a follow up review? (See my comments later on how you 
> > can use review dependencies if you are new to ReviewBoard)
> 
> Vijay Srinivasaraghavan wrote:
> Thanks Anand. I have made the python binding changes locally on my 
> machine. Do you want me to create a new JIRA bug for this issue and create a 
> new review or add the patch to this JIRA/review itself? I am little confused 
> as to how the dependencies are handled with RB.
> 
> Anand Mazumdar wrote:
> No need to create a separate issue. It would be fine to create a new 
> review with it only containing the python related changes. This would be 
> another commit based on top of this one. You can then use `post-reviews.py` 
> and it should handle setting the dependencies for you. Feel free to ping me 
> on IRC/Slack if you run into any issues (anandm)
> 
> Vijay Srinivasaraghavan wrote:
> Thanks for the clarification. I have attached a new patch with "53825" as 
> dependendency

I have created two patch for those additional issues that was addressed as part 
of the original request. Please let me know if you are waiting for anything 
from my end before this can be merged? Thanks  
https://reviews.apache.org/r/54014/
https://reviews.apache.org/r/54015/


- Vijay


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


On Nov. 23, 2016, 5:46 a.m., Vijay Srinivasaraghavan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53825/
> ---
> 
> (Updated Nov. 23, 2016, 5:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zameer Manji.
> 
> 
> Bugs: MESOS-6597
> https://issues.apache.org/jira/browse/MESOS-6597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6597 Enabled java protos generation for all V1 proto files.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5e0b8406f7f624bd8b03ff76b887f20e22fc66e0 
> 
> Diff: https://reviews.apache.org/r/53825/diff/
> 
> 
> Testing
> ---
> 
> Built mesos and confirmed jar file containing new java PB wrappers for V1 
> API. Ran standard unit test (make tests)
> 
> 
> Thanks,
> 
> Vijay Srinivasaraghavan
> 
>

Re: Review Request 53963: CMake: Use StoutTestsConfigure for test-specific variables.

[Alex Clemmer]

> On Nov. 28, 2016, 6:46 p.m., Alex Clemmer wrote:
> > src/tests/CMakeLists.txt, line 17
> > 
> >
> > Super tiny nit: we usually include these in the 
> > `ProcessTestsConfigure.cmake` to miniimize the number of things we have to 
> > include in order to configure a given tests package. I don't consider this 
> > a ship-stopper though.

Upon going further down the chain, I see that we're retiring the inclusion of 
`ProcessTestsConfigure` in all the other `CMakeLists.txt`. So we can likely 
mark this issue as "dropped".


- Alex


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


On Nov. 21, 2016, 10:08 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53963/
> ---
> 
> (Updated Nov. 21, 2016, 10:08 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Uses test variables defined in `StoutTestsConfigure` instead of
> redefining the variables in the Mesos tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 770a3828a6e0ffaa4f185392fdc1a2152446449d 
>   src/master/cmake/MasterConfigure.cmake 
> 6bbd7e87273976f40527d719cc9450ff9a1d2ac7 
>   src/slave/cmake/SlaveConfigure.cmake 
> b339239761a5de321d65b92376dae69c339bee5c 
>   src/tests/CMakeLists.txt cf583ea3f10482b510459fb11a7ecaf76e498391 
>   src/tests/cmake/MesosTestsConfigure.cmake 
> 3f543c010ae87ff04e6b45745bc49ef65b6590ff 
> 
> Diff: https://reviews.apache.org/r/53963/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53982: Added documentation for posix/rlimit isolator.

[Neil Conway]

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



Overall seems okay, but there no discussion of _why_ I would want to use 
rlimits to constrain the resource usage of a task, compared with other methods 
of resource isolation (e.g., `posix/cpu`, `posix/mem`, etc.)

Other things it would be good to spell out:

* The resource limit for a task is different than the amount of resources the 
task is allocated by Mesos. Presumably it is up to the user to ensure that the 
`rlimits_info` in their `ContainerInfo` is set appropriately, right? As an 
operator, I can't require that all tasks are run under a certain rlimit, right?
* If the `mesos-agent` is started under a resource limit, what happens? i.e., 
presumably we can launch tasks with higher hard rlimits iff `mesos-agent` 
process has sufficient priviledges to do so.
* If the containerizer fails to set the resource limits for a task, what 
happens? Presumably the task launch fails; how is this signaled to the 
framework?


docs/posix_rlimits.md (line 3)


s/The isolators/This isolator/ ?



docs/posix_rlimits.md (line 4)


"adds support for setting POSIX resource limits (rlimits) for containers 
..."



docs/posix_rlimits.md (line 7)


"POSIX rlimits can be used to control the resources that a process can 
consume."



docs/posix_rlimits.md (line 8)


"Resource limits are typically set at boot time and inherited when a child 
process is forked from a parent process; resource limits can also be modified 
via `setrlimit(2)`."



docs/posix_rlimits.md (line 11)


Comma after "shells"



docs/posix_rlimits.md (line 14)


"tuple" seems like unnecessary jargon here. "A resource limit consists of a 
_soft_ and a _hard_ limit. The soft limit specifies the effective ..."



docs/posix_rlimits.md (line 15)


The difference between hard and soft limits is unclear here. Would be good 
to elaborate on what purpose the different types of limits serve (e.g., soft 
limits are useful when the limiter and limited process can be assumed to be 
cooperative; hard limits are more useful as a security mechanism).



docs/posix_rlimits.md (line 22)


"enables interpretation" seems vague here. The point is that by using this 
isolator, resource limits specified in the task's `ContainerInfo` will be 
applied to the task's container, right? If so, I'd say that directly and avoid 
phrases like "interpreting rlimits".

Also mention that rlimits are specified via the `rlimit_info` field of 
`ContainerInfo`. Wouldn't hurt to give an example of doing this.



docs/posix_rlimits.md (line 47)


Comma after "types".



docs/posix_rlimits.md (line 48)


s/In addition many limits defined on Linux/Linux defines a number of 
additional resource limits that are not included in the POSIX standard. These 
are also supported by this isolator; see the definition of `RLimitInfo::RLimit` 
for the list of currently exposed types."

Is there a reason to not include all the rlimit types in this document? The 
list is probably not going to change very often.


- Neil Conway


On Nov. 22, 2016, 1:27 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53982/
> ---
> 
> (Updated Nov. 22, 2016, 1:27 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Bugs: MESOS-6427
> https://issues.apache.org/jira/browse/MESOS-6427
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for posix/rlimit isolator.
> 
> 
> Diffs
> -
> 
>   docs/mesos-containerizer.md 2bff35f6361f760a9001a249d2c01bbbc9e72932 
>   docs/posix_rlimits.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53982/diff/
> 
> 
> Testing
> ---
> 
> Tested with github markdown renderer, and with build setup from `site/`.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 54114: Updated http::Connection::disconnect to do a complete socket shutdown.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Nov. 28, 2016, 6:20 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54114/
> ---
> 
> (Updated Nov. 28, 2016, 6:20 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated http::Connection::disconnect to do a complete socket shutdown.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/http.cpp 3f16f293a5c5cd0b31a85efe94cb6f8019543d45 
> 
> Diff: https://reviews.apache.org/r/54114/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 54016: Fixed a few places to use `foreachkey` / `foreachvalue`.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 22, 2016, 8:56 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54016/
> ---
> 
> (Updated Nov. 22, 2016, 8:56 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6626
> https://issues.apache.org/jira/browse/MESOS-6626
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is preferrable to doing `foreach` over `hashmap::keys()` or
> `hashmap::values()`, respectively.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp f4e1ea4816fee9a2f6d59bb534933b66013f11cf 
>   src/slave/containerizer/fetcher.cpp 
> d200c117579bc1c2d9d24f14bf4da8f650d3f562 
> 
> Diff: https://reviews.apache.org/r/54016/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 53993: Updated quota doc to support quota update.

[Zhitao Li]

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

Review request for mesos, Alexander Rukletsov and Xiaojian Huang.


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


Repository: mesos


Description
---

Updated quota doc to support quota update.


Diffs
-

  docs/quota.md d7e8d58eeeac9d9ffc1a6f7614c7ddce6d7a9b89 

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


Testing
---

https://github.com/zhitaoli/mesos/blob/quota_update_sept/docs/quota.md


Thanks,

Zhitao Li

Re: Review Request 53768: CMake Cleanup: Generate protobufs with fewer dependencies.

[Alex Clemmer]

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


Ship it!




Ship It!

- Alex Clemmer


On Nov. 28, 2016, 6:48 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53768/
> ---
> 
> (Updated Nov. 28, 2016, 6:48 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The protobuf sub-library only actually requires the protobuf
> dependencies in order to build.  Previously, it was dependent
> on all of libprocess.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake de04389b016b22bdff69e94d37fad4eedbda5874 
>   src/CMakeLists.txt 9ff47d7a95c3baa5aa10039039e5ad065180ba45 
> 
> Diff: https://reviews.apache.org/r/53768/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53769: CMake: Added -fPIC to leveldb build.

[Alex Clemmer]

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


Ship it!




Ship It!

- Alex Clemmer


On Nov. 15, 2016, 3:21 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53769/
> ---
> 
> (Updated Nov. 15, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Leveldb is built statically (this is not configurable).
> This supplies the `-fPIC` compiler option as well as the default
> options used by leveldb (`-O2 -g`).
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 770a3828a6e0ffaa4f185392fdc1a2152446449d 
> 
> Diff: https://reviews.apache.org/r/53769/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53767: CMake Cleanup: Rename SlaveConfigure.cmake.

[Alex Clemmer]

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


Ship it!




Ship It!

- Alex Clemmer


On Nov. 15, 2016, 3:19 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53767/
> ---
> 
> (Updated Nov. 15, 2016, 3:19 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed to `AgentConfigure.cmake`.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake de04389b016b22bdff69e94d37fad4eedbda5874 
>   src/slave/cmake/SlaveConfigure.cmake 
> b339239761a5de321d65b92376dae69c339bee5c 
> 
> Diff: https://reviews.apache.org/r/53767/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53766: CMake: Added option for enabling optimization.

[Alex Clemmer]

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


Ship it!




Ship It!

- Alex Clemmer


On Nov. 15, 2016, 3:18 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53766/
> ---
> 
> (Updated Nov. 15, 2016, 3:18 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This mirrors the automake build, which sets -O2 by default.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 11a8507eb773391073a7b945e2aac503262f86b7 
> 
> Diff: https://reviews.apache.org/r/53766/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53765: CMake: Revised compilation configuration file.

[Alex Clemmer]

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


Ship it!




Ship It!

- Alex Clemmer


On Nov. 15, 2016, 10:05 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53765/
> ---
> 
> (Updated Nov. 15, 2016, 10:05 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This moves global configuration options into one place and
> logically groups the various operations: by Posix, Linux, Windows,
> and then "general" groups.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 1362d65afc23816c34173866a3fdbce45936921d 
>   cmake/CompilationConfigure.cmake 11a8507eb773391073a7b945e2aac503262f86b7 
> 
> Diff: https://reviews.apache.org/r/53765/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53695: Allows caching extractable files when outputFile is set.

[Stephen Hankinson]

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

(Updated Nov. 28, 2016, 7:14 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Changes requested by reviewer.


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


Repository: mesos


Description
---

Allows caching extractable files when outputFile is set.


Diffs (updated)
-

  src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f 
  src/tests/fetcher_cache_tests.cpp 03e817d46194d451eceb70f4cebb54dfdcb4c2e7 

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


Testing
---

A new test, FetcherCacheTest.LocalCachedExtractCustomOutputFile, was created to 
confirm that when a URI is created with cached: true, extract: true, a custom 
outputFile with a proper compression suffix set, and an original URI that 
doesn't end with a proper compression suffix fails to extract properly from the 
cache.

The test makes a copy of the test archive in the cache but appends "misc" to 
the end, forming an invalid compression suffix, causing 
mesos-fetcher-test-archive.tgz to become mesos-fetcher-test-archive.tgzmisc. 
The URI path is set to this modified location. The outputFile of the URI is set 
ARCHIVE_NAME which ends with a valid compression suffix, so therefore should 
extract properly.

At the end of the test the temporary cached archive file 
(mesos-fetcher-test-archive.tgzmisc) is deleted.

Before the patch was written this test was failing as expected. After 
implementing the patch, this and all other tests were passing.


Thanks,

Stephen Hankinson

Re: Review Request 53764: CMake: Added a target for the default executor.

[Alex Clemmer]

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


Ship it!





src/launcher/CMakeLists.txt 


Are we deleting this line because this is included also in the `src/` 
directory? Do you think there is any benefit in having submodules include 
everything they need to build, themselves, rather than depending on the `src/` 
directory to set it up?


- Alex Clemmer


On Nov. 15, 2016, 3:15 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53764/
> ---
> 
> (Updated Nov. 15, 2016, 3:15 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This executor has support for TaskGroups and will eventually take
> precedence over the default command executor.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake de04389b016b22bdff69e94d37fad4eedbda5874 
>   src/launcher/CMakeLists.txt 3137ea1479970c7ea46e6595536d481002fc585c 
> 
> Diff: https://reviews.apache.org/r/53764/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53763: CMake: Added binaries as dependencies for MESOS_TARGET.

[Alex Clemmer]

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


Ship it!




Ship It!

- Alex Clemmer


On Nov. 15, 2016, 10:57 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53763/
> ---
> 
> (Updated Nov. 15, 2016, 10:57 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This hooks up some of Mesos's companion binaries to the build chain.
> Tests can now rely on the `MESOS_TARGET` to get a complete build.
> 
> 
> Diffs
> -
> 
>   src/cli/CMakeLists.txt c0120cd7f4ef8dc275edbd5cb1a4a6e23952ec63 
>   src/docker/CMakeLists.txt dca15dbf0679ae196b7a993f2e782ce2907c6750 
>   src/health-check/CMakeLists.txt ca577895411a13dcca27dbd67a63de831084ed37 
>   src/launcher/CMakeLists.txt 3137ea1479970c7ea46e6595536d481002fc585c 
>   src/local/CMakeLists.txt 643a6cf653d91b8a606ac83b126dfd2245fa49bd 
>   src/log/CMakeLists.txt 8a92de4b0f9f69164930bd28c527f3a92ea0d17e 
>   src/master/CMakeLists.txt d9b4c170148daf041cb68fcebbc993abcde98acf 
>   src/slave/CMakeLists.txt 33120ace79bce449329a7cc4b7ef264d2867fc13 
>   src/slave/container_loggers/CMakeLists.txt PRE-CREATION 
>   src/slave/containerizer/mesos/CMakeLists.txt 
> 8cb3507d4d250dee4ee2f1b324ac3fe945cf23c4 
>   src/slave/qos_controllers/CMakeLists.txt 
> 87c92af21c012655c201c01cd4ba5ff912555119 
>   src/slave/resource_estimators/CMakeLists.txt 
> 17b149f734ea9dc8ac4c5dd45bdb8312faf4cc77 
>   src/usage/CMakeLists.txt e5df5e74977ae085db85e35c67dedc974832496a 
> 
> Diff: https://reviews.apache.org/r/53763/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 54135: Added test for offer rescind in quota update.

[Zhitao Li]

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

Review request for mesos, Alexander Rukletsov and Xiaojian Huang.


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


Repository: mesos


Description
---

Added test for offer rescind in quota update.


Diffs
-

  src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 52103: Implemented quota update through `PUT` method.

[Zhitao Li]

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

(Updated Nov. 28, 2016, 7:06 p.m.)


Review request for mesos, Alexander Rukletsov and Xiaojian Huang.


Changes
---

Fixed offer rescinding logic.


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


Repository: mesos


Description (updated)
---

This patch implemented atomic quota update through
`PUT` methood on the `/quota` endpoint.

Changes included:
- a new virtual function on the allocator interface-
- implementation in hierarchical allocator;
- implementation in quota_handler;
- some integration tests for updating a quota.

More tests around resource rescind and operator API
implementation will be sent in later patches.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
3fe04f8a4fb331defbe672c739df4e137bdb3627 
  src/master/allocator/mesos/allocator.hpp 
61159a91700270b1aa6fb335b8a41592e03ef8a9 
  src/master/allocator/mesos/hierarchical.hpp 
2c31471ee0f5d6836393bf87ff9ecfd8df835013 
  src/master/allocator/mesos/hierarchical.cpp 
3b759494071c4cae4b8b7dbcb0028df4146fc30e 
  src/master/http.cpp 90cbed1ba411e18906fe9c26bc14576a26d1b7b9 
  src/master/master.hpp b6c610b79aab146ece6b9be59be86f283d1c9ee8 
  src/master/quota_handler.cpp 5578663f26d9b737499dc4f5a286c34c37645442 
  src/tests/allocator.hpp bce11904f0b8de66a6620b636321568fdc0d113f 
  src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 53762: CMake: Renamed MESOS_TARGET to MESOS_LIBS_TARGET.

[Alex Clemmer]

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


Ship it!




Ship It!

- Alex Clemmer


On Nov. 21, 2016, 10:15 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53762/
> ---
> 
> (Updated Nov. 21, 2016, 10:15 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This purposefully excludes the dependency on `MESOS_TARGET` that
> exists in `MesosTestsConfigure.cmake`, as the tests require all
> of Mesos to be build first.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt aef9ae6d2872dc15725c01ce85b657965485605f 
>   src/cli/CMakeLists.txt c0120cd7f4ef8dc275edbd5cb1a4a6e23952ec63 
>   src/docker/CMakeLists.txt dca15dbf0679ae196b7a993f2e782ce2907c6750 
>   src/examples/CMakeLists.txt PRE-CREATION 
>   src/health-check/CMakeLists.txt ca577895411a13dcca27dbd67a63de831084ed37 
>   src/local/CMakeLists.txt 643a6cf653d91b8a606ac83b126dfd2245fa49bd 
>   src/log/CMakeLists.txt 8a92de4b0f9f69164930bd28c527f3a92ea0d17e 
>   src/master/CMakeLists.txt d9b4c170148daf041cb68fcebbc993abcde98acf 
>   src/slave/CMakeLists.txt 33120ace79bce449329a7cc4b7ef264d2867fc13 
>   src/slave/container_loggers/CMakeLists.txt PRE-CREATION 
>   src/slave/containerizer/mesos/CMakeLists.txt 
> 8cb3507d4d250dee4ee2f1b324ac3fe945cf23c4 
>   src/slave/qos_controllers/CMakeLists.txt 
> 87c92af21c012655c201c01cd4ba5ff912555119 
>   src/slave/resource_estimators/CMakeLists.txt 
> 17b149f734ea9dc8ac4c5dd45bdb8312faf4cc77 
>   src/tests/cmake/MesosTestsConfigure.cmake 
> 3f543c010ae87ff04e6b45745bc49ef65b6590ff 
>   src/usage/CMakeLists.txt e5df5e74977ae085db85e35c67dedc974832496a 
> 
> Diff: https://reviews.apache.org/r/53762/diff/
> 
> 
> Testing
> ---
> 
> rm -rf build
> mkdir build
> cd build
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53761: CMake: Add a target between MESOS_TARGET and MESOS_PROTOBUFs.

[Alex Clemmer]

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


Fix it, then Ship it!





cmake/MesosConfigure.cmake (line 142)


Interesting, for my own education: is this the same in the AC build 
solution? I don't see a target like this in the AC system, but I also don't 
really know what to search for.


- Alex Clemmer


On Nov. 15, 2016, 3:11 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53761/
> ---
> 
> (Updated Nov. 15, 2016, 3:11 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We need this extra level of targets in order to model the
> dependency chain for binaries and libraries.  An example
> is the mesos-containerizer binary, which depends on libmesos
> (i.e. MESOS_TARGET) but does not get included in the dependency
> chain if you only depend on MESOS_TARGET.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake de04389b016b22bdff69e94d37fad4eedbda5874 
> 
> Diff: https://reviews.apache.org/r/53761/diff/
> 
> 
> Testing
> ---
> 
> See next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53760: CMake: Changed example module output location and depedencies.

[Alex Clemmer]

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


Fix it, then Ship it!





src/slave/container_loggers/CMakeLists.txt (line 33)


Minor nit: probably want to move this directory to a variable?


- Alex Clemmer


On Nov. 15, 2016, 11:09 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53760/
> ---
> 
> (Updated Nov. 15, 2016, 11:09 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This moves some test libraries from the default CMake build
> output locations to `/src/.libs`, which is where
> the Automake build outputs libraries.  We have a few tests
> that expect modules to be located in this directory.
> 
> Additionally, this makes the test target (make check) depend
> on the test modules and example frameworks, so that they are built
> in the correct order.
> 
> 
> Diffs
> -
> 
>   src/examples/CMakeLists.txt PRE-CREATION 
>   src/slave/container_loggers/CMakeLists.txt PRE-CREATION 
>   src/slave/qos_controllers/CMakeLists.txt 
> 87c92af21c012655c201c01cd4ba5ff912555119 
>   src/slave/resource_estimators/CMakeLists.txt 
> 17b149f734ea9dc8ac4c5dd45bdb8312faf4cc77 
>   src/tests/CMakeLists.txt cf583ea3f10482b510459fb11a7ecaf76e498391 
> 
> Diff: https://reviews.apache.org/r/53760/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make clean
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53758: CMake: Added test sources to the build.

[Alex Clemmer]

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


Fix it, then Ship it!





src/tests/CMakeLists.txt (line 79)


Just so I'm clear about this: in other places, we're disabling source 
builds on WIN32 by `if`'ing out the source inclusion in the file, but here 
(because of legacy reasons) we're depending on the "old way" of doing stuff, 
which is to `if` out the `add_subdirectory(tests/)`?

If so, it would be nice to have a transition plan going in -- either one of 
us should clean it up (either in this chain or after it), or we should file an 
issue, I think. Less desirable would be to have two different conventions. 
_i.e._ I personally don't consider the changes required to be a stop-ship, but 
I do consider having a plan to be a stop ship. :)


- Alex Clemmer


On Nov. 22, 2016, 8 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53758/
> ---
> 
> (Updated Nov. 22, 2016, 8 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds most existing tests to the CMake build.  This excludes
> a few special cases, just as Java, Python, and network isolator
> tests.
> 
> This review replaces: https://reviews.apache.org/r/49921/
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 1362d65afc23816c34173866a3fdbce45936921d 
>   src/CMakeLists.txt aef9ae6d2872dc15725c01ce85b657965485605f 
>   src/tests/CMakeLists.txt 90b92251b7e133124f6d9a896190ceea402cbff1 
>   src/tests/cmake/MesosTestsConfigure.cmake 
> 3f543c010ae87ff04e6b45745bc49ef65b6590ff 
> 
> Diff: https://reviews.apache.org/r/53758/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 54129: Removed superseded `slavePreLaunchDockerEnvironmentDecorator` hook .

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54038, 54068, 54128, 54129]

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

- Mesos ReviewBot


On Nov. 28, 2016, 3:53 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54129/
> ---
> 
> (Updated Nov. 28, 2016, 3:53 p.m.)
> 
> 
> Review request for mesos, Adam B and Kapil Arya.
> 
> 
> Bugs: MESOS-6396
> https://issues.apache.org/jira/browse/MESOS-6396
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/hook.hpp f0606e3a68fa179cf7ea036f10563ef47c2aefa7 
>   src/examples/test_hook_module.cpp 5e91a71f2450cf3c37eb9039ef28c026095c917e 
>   src/hook/manager.hpp 5ecfcab48da808c84d36f9bcfcb5a8e0ad2167e5 
>   src/hook/manager.cpp 24885226a788a7abd851e12b527f74fa972ec935 
>   src/slave/containerizer/docker.cpp a8e522fc058f50560e8ec162c31be079e620bf9d 
>   src/tests/hook_tests.cpp d334d6c5ff7f966d55b395bfbf4f25ee3fa2 
> 
> Diff: https://reviews.apache.org/r/54129/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 53757: CMake: Moved logrotate container logger binary build definition.

[Alex Clemmer]

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


Ship it!




Ship It!

- Alex Clemmer


On Nov. 15, 2016, 10:56 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53757/
> ---
> 
> (Updated Nov. 15, 2016, 10:56 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was originally included at a higher level.  Per CMake style,
> build files should generally be included in the CMakeLists.txt file
> located in the same directory.
> 
> 
> Diffs
> -
> 
>   src/slave/CMakeLists.txt 33120ace79bce449329a7cc4b7ef264d2867fc13 
>   src/slave/container_loggers/CMakeLists.txt PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53757/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53756: CMake: Added logrotate container logger module to the build.

[Alex Clemmer]

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


Ship it!




Ship It!

- Alex Clemmer


On Nov. 15, 2016, 10:56 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53756/
> ---
> 
> (Updated Nov. 15, 2016, 10:56 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This review replaces: https://reviews.apache.org/r/49874/
> 
> 
> Diffs
> -
> 
>   src/slave/CMakeLists.txt 33120ace79bce449329a7cc4b7ef264d2867fc13 
>   src/slave/cmake/SlaveConfigure.cmake 
> b339239761a5de321d65b92376dae69c339bee5c 
>   src/slave/container_loggers/CMakeLists.txt PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53756/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53755: CMake: Added example frameworks and executors.

[Alex Clemmer]

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


Ship it!




Ship It!

- Alex Clemmer


On Nov. 23, 2016, 9:25 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53755/
> ---
> 
> (Updated Nov. 23, 2016, 9:25 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This builds some binaries that are mainly used in the example
> framework tests.
> 
> This review replaces: https://reviews.apache.org/r/49870/ 
> and https://reviews.apache.org/r/50327/
> 
> 
> Diffs
> -
> 
>   src/examples/CMakeLists.txt PRE-CREATION 
>   src/examples/cmake/ExamplesConfigure.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53755/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>