Re: Review Request 55400: Fix compilation error for clang-3.5 type deduction error.

[Michael Park]

> On Jan. 10, 2017, 5:40 p.m., Joseph Wu wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 1598
> > 
> >
> > Technically, we expect the return type to be `Future`, 
> > to match the return type of the function.

Well, the intention is that we simply fill in the correct type that a correct 
compiler would deduce here.


- Michael


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


On Jan. 10, 2017, 5:28 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55400/
> ---
> 
> (Updated Jan. 10, 2017, 5:28 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `clang-3.5` performs incorrect type deduction for lambda return types.
> Namely,
> 
> ```cpp
> struct S {};
> auto l = [](const S& s) { return s; }
> ```
> 
> The deduced return type of lambda `l` is `const S` as opposed to `S`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> c444568dd478902afd79fe3c8960364fd68fabb5 
> 
> Diff: https://reviews.apache.org/r/55400/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 55400: Fix compilation error for clang-3.5 type deduction error.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [55400]

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 Jan. 11, 2017, 1:28 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55400/
> ---
> 
> (Updated Jan. 11, 2017, 1:28 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `clang-3.5` performs incorrect type deduction for lambda return types.
> Namely,
> 
> ```cpp
> struct S {};
> auto l = [](const S& s) { return s; }
> ```
> 
> The deduced return type of lambda `l` is `const S` as opposed to `S`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> c444568dd478902afd79fe3c8960364fd68fabb5 
> 
> Diff: https://reviews.apache.org/r/55400/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 55025: Transitioned `BUILD_DIR` to use Unix-style paths on Windows.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Dec. 24, 2016, 3:09 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55025/
> ---
> 
> (Updated Dec. 24, 2016, 3:09 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-6736
> https://issues.apache.org/jira/browse/MESOS-6736
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in the test harness, we pass the flags `-DSOURCE_DIR` and
> `-DBUILD_DIR` to the compiler. These are used to do things like
> determine where the containerizer is to launch it for tests.
> 
> On Windows, these paths should be Windows-style, with '\' characters
> separating path components. Unfortunately, CMake does not escape these
> slashes in path strings, so when we pass them as preprocessor flags,
> a string like `C:\src` will look to the standard Windows API like a
> string with an escaped '\s' character.
> 
> On the other hand, Windows APIs are happy to take Unix-style paths as
> arguments. So, to unblock making the agent tests work, we simply use
> Unix paths here.
> 
> 
> Diffs
> -
> 
>   src/tests/cmake/MesosTestsConfigure.cmake 
> ca042bd3f7ebc339f1c342b1854a66e33ee9f20c 
> 
> Diff: https://reviews.apache.org/r/55025/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Joseph Wu]

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




3rdparty/libprocess/src/process.cpp (lines 1040 - 1041)


Even if this is idempotent, we can remove the call from some of our own 
binaries now.

Also, what is the version check you mention here?



3rdparty/libprocess/src/process.cpp (line 1046)


s/teard down/teardown/

`process::finalize` should probably perform the socket teardown, or at 
least give the option to do so.  

Currently, very few of our processes call `process::finalize`.  They 
usually just rely on the OS cleaning up after them.


- Joseph Wu


On Dec. 24, 2016, 2:46 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55024/
> ---
> 
> (Updated Dec. 24, 2016, 2:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently libprocess will attempt to use sockets without initializing
> the socket stack on Windows. This commit will resolving this problem by
> causing `process::initialize` to initialize the socket stack.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 889a03444eaee7b5ad2be65bb414c30062d4a4f0 
> 
> Diff: https://reviews.apache.org/r/55024/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55022: Windows: Cause errors to be correctly reported in `io::read`.

[Joseph Wu]

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



The change LGTM, but it might need a rebase due to some changes BenH made to 
the same file.  And MPark will also be committing a change to this file soon 
(related to `io::peek`).

A few things to note:

* Why not make the same changes to `io::write`?
* Remove the TODO that BenH left: `TODO(benh): Confirm that `os::strerror` does 
the right thing for `error` on Windows.`

- Joseph Wu


On Dec. 24, 2016, 1:31 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55022/
> ---
> 
> (Updated Dec. 24, 2016, 1:31 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Cause errors to be correctly reported in `io::read`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/io.cpp 27da897894e12941a6bba5f5eda04c35100d2d73 
> 
> Diff: https://reviews.apache.org/r/55022/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55400: Fix compilation error for clang-3.5 type deduction error.

[Joseph Wu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/io/switchboard.cpp (line 1598)


Technically, we expect the return type to be `Future`, to 
match the return type of the function.


- Joseph Wu


On Jan. 10, 2017, 5:28 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55400/
> ---
> 
> (Updated Jan. 10, 2017, 5:28 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `clang-3.5` performs incorrect type deduction for lambda return types.
> Namely,
> 
> ```cpp
> struct S {};
> auto l = [](const S& s) { return s; }
> ```
> 
> The deduced return type of lambda `l` is `const S` as opposed to `S`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> c444568dd478902afd79fe3c8960364fd68fabb5 
> 
> Diff: https://reviews.apache.org/r/55400/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 54794: Cleaned up teardown tests slightly.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Dec. 15, 2016, 9:13 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54794/
> ---
> 
> (Updated Dec. 15, 2016, 9:13 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up teardown tests slightly.
> 
> 
> Diffs
> -
> 
>   src/tests/teardown_tests.cpp ce51d207de605699a7d409228cb2e355a4d6facc 
> 
> Diff: https://reviews.apache.org/r/54794/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 54793: Prevented task launches that reuse unreachable task IDs.

[Vinod Kone]

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




src/tests/partition_tests.cpp (line 3384)


Can you move this to master_validation_tests.cpp to be closer to other 
validation tests?


- Vinod Kone


On Jan. 10, 2017, 10:36 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54793/
> ---
> 
> (Updated Jan. 10, 2017, 10:36 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6805
> https://issues.apache.org/jira/browse/MESOS-6805
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master keeps an in-memory cache of task IDs that have recently been
> marked unreachable. The master now consults this cache to reject task
> launch attempts that reuse one of these recently unreachable task IDs
> (such tasks are not terminal and may resume running in the future). This
> check is not complete (we won't detect all cases in which unreachable
> task IDs are reused), but preventing this from happening in the common
> case seems worth doing. See MESOS-6785 for details.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f 
>   src/tests/partition_tests.cpp 72013d1bfee275c6f3cb90173f0c408d55e0bc5d 
> 
> Diff: https://reviews.apache.org/r/54793/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 54408: Replaced `Master::Framework::active` with a new `state` enum value.

[Vinod Kone]

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


Fix it, then Ship it!




Thanks for following through.


src/master/master.hpp (lines 2224 - 2239)


can you reorder these as so ?

RECOVERED,
DISCONNECTED,
INACTIVE,
ACTIVE

it's a bit more chronological but not really.



src/master/master.cpp (line 2914)


s/not connected/disconnected/


- Vinod Kone


On Dec. 7, 2016, 8:08 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54408/
> ---
> 
> (Updated Dec. 7, 2016, 8:08 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6719
> https://issues.apache.org/jira/browse/MESOS-6719
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> That is, the master previously tracked two separate things about a
> framework: its "state" (CONNECTED, DISCONNECTED, or RECOVERED), and
> whether the framework is considered active. It is simpler to represent
> the latter value as just another state: a framework can now be ACTIVE,
> INACTIVE, DISCONNECTED, or RECOVERED. A framework is "connected" if it
> is either ACTIVE or INACTIVE. This rules out a few combinations that
> never made sense, such as "state = DISCONNECTED and active = TRUE".
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp ac560d1fdd219d0de0c5d987a32a7112e149602f 
>   src/master/master.hpp b444b2352360fb4f7179acd97dffc0cd81cc7afa 
>   src/master/master.cpp 67f32229470da4cf7953881d1c5dcb99393002de 
>   src/master/quota_handler.cpp 5578663f26d9b737499dc4f5a286c34c37645442 
> 
> Diff: https://reviews.apache.org/r/54408/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 55400: Fix compilation error for clang-3.5 type deduction error.

[Michael Park]

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

Review request for mesos and Joseph Wu.


Repository: mesos


Description
---

`clang-3.5` performs incorrect type deduction for lambda return types.
Namely,

```cpp
struct S {};
auto l = [](const S& s) { return s; }
```

The deduced return type of lambda `l` is `const S` as opposed to `S`.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
c444568dd478902afd79fe3c8960364fd68fabb5 

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


Testing
---


Thanks,

Michael Park

Re: Review Request 54590: Removed unused peek function.

[Michael Park]

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

(Updated Jan. 10, 2017, 5:22 p.m.)


Review request for mesos, Daniel Pravat and Joris Van Remoortere.


Changes
---

Updated description.


Summary (updated)
-

Removed unused peek function.


Repository: mesos


Description (updated)
---

This essentially reverts the introduction of `peek` function from
0d87ec198b6f15d4e7025f82b7b9385175a10d4a. We can certainly re-introduce
it in the future, but as of now it has been sitting there for over
a year and we have no uses of it.


Diffs (updated)
-

  3rdparty/libprocess/include/process/io.hpp 
f5489dc66adb136d9f53f98ac64c3fbe7831a1c2 
  3rdparty/libprocess/src/io.cpp 8aa3576a11ed5e998492b4020cb37a45ec708093 
  3rdparty/libprocess/src/tests/io_tests.cpp 
466e343e6d775fe09debce119eae4fc4849b7006 

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


Testing
---


Thanks,

Michael Park

Re: Review Request 54590: Removed unused `peek` function.

[Joseph Wu]

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


Ship it!




LGTM.

I'd mention the original review in the commit description to make tracking the 
history easier:
https://reviews.apache.org/r/36404/

- Joseph Wu


On Jan. 9, 2017, 6:47 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54590/
> ---
> 
> (Updated Jan. 9, 2017, 6:47 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/io.hpp 
> f5489dc66adb136d9f53f98ac64c3fbe7831a1c2 
>   3rdparty/libprocess/src/io.cpp 8aa3576a11ed5e998492b4020cb37a45ec708093 
>   3rdparty/libprocess/src/tests/io_tests.cpp 
> 466e343e6d775fe09debce119eae4fc4849b7006 
> 
> Diff: https://reviews.apache.org/r/54590/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 54407: Refactored Master::removeFramework to use Master::deactivate.

[Vinod Kone]

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




src/master/master.cpp 


I think we (I?) originially didn't call `deactivate` here because that 
causes rescind messages to be sent to the framework, which don't make sense 
here. Maybe `decativate` can take a `bool rescind`?


- Vinod Kone


On Jan. 10, 2017, 10:35 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54407/
> ---
> 
> (Updated Jan. 10, 2017, 10:35 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6719
> https://issues.apache.org/jira/browse/MESOS-6719
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored Master::removeFramework to use Master::deactivate.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
> 
> Diff: https://reviews.apache.org/r/54407/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53803: Added new libprocess socket tests.

[Greg Mann]

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

(Updated Jan. 11, 2017, 12:52 a.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


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


Repository: mesos


Description
---

This patch adds NetSocketTest.EOFBeforeRecv and
NetSocketTest.EOFAfterRecv to verify that EOFs are
reliably received whether or not there is a pending recv()
request at the time the EOF is received.


Diffs
-

  3rdparty/libprocess/src/tests/socket_tests.cpp 
44c3c9adc39702dd598aa0105088517df601bbda 

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


Testing
---

Testing details are at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Jan. 11, 2017, 12:51 a.m.)


Review request for mesos, Benjamin Hindman, Benjamin Mahler, and Joseph Wu.


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


Repository: mesos


Description
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
57eaf4f607d0628db466cc1a139772eeeaa51136 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
0e292a8b0d470f4e199db08f09a0c863d73c 

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


Testing
---

Testing details are found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 54232: Shutdown tasks of completed frameworks on agent re-registration.

[Vinod Kone]

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




src/master/master.cpp (lines 5689 - 5728)


can you do one pass over framworks to decide whether to shutdown them or 
not? an added benefit is that, you won't end up sending shutdown twice (e.g., 
for non-partition aware completed frameworks).


- Vinod Kone


On Jan. 10, 2017, 10:32 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54232/
> ---
> 
> (Updated Jan. 10, 2017, 10:32 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6602
> https://issues.apache.org/jira/browse/MESOS-6602
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, if a framework completed (e.g., due to a teardown operation
> or framework shutdown), any framework tasks running on partitioned
> agents would not be shutdown when the agent re-registered. For tasks
> that are not partition-aware, the task would be shutdown on agent
> re-registration anyway. But for partition-aware tasks, this could lead
> to orphan tasks.
> 
> Fix this by changing the master to shutdown such tasks when the agent
> reregisters.
> 
> Note that if the master fails over between the time the framework
> completes and a partitioned agent re-registers, any framework tasks
> running on the agent will NOT be shutdown. This is a known bug; fixing
> it requires persisting the framework shutdown operation to the registry
> (MESOS-1719).
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 368ee1d5e97784fa54e0f141906405ee8f104317 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/tests/partition_tests.cpp 72013d1bfee275c6f3cb90173f0c408d55e0bc5d 
> 
> Diff: https://reviews.apache.org/r/54232/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53712: Added `system` environment variables in ` execvpe.cpp`.

[Joseph Wu]

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



Looks like this is a diff-of-a-diff.  I'd expect the actual diff to look like 
revisions 2 and 3 combined.

- Joseph Wu


On Jan. 10, 2017, 11:01 a.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53712/
> ---
> 
> (Updated Jan. 10, 2017, 11:01 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `system` environment variables in ` execvpe.cpp`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
> 
> Diff: https://reviews.apache.org/r/53712/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 54232: Shutdown tasks of completed frameworks on agent re-registration.

[Vinod Kone]

> On Jan. 3, 2017, 10:51 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 5512
> > 
> >
> > inline this?
> 
> Neil Conway wrote:
> To me, using a separate function was more readable than writing the logic 
> inline. Happy to change it if you disagree, though.

see my comment on the dependent review for my reasoning.


- Vinod


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


On Jan. 10, 2017, 10:32 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54232/
> ---
> 
> (Updated Jan. 10, 2017, 10:32 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6602
> https://issues.apache.org/jira/browse/MESOS-6602
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, if a framework completed (e.g., due to a teardown operation
> or framework shutdown), any framework tasks running on partitioned
> agents would not be shutdown when the agent re-registered. For tasks
> that are not partition-aware, the task would be shutdown on agent
> re-registration anyway. But for partition-aware tasks, this could lead
> to orphan tasks.
> 
> Fix this by changing the master to shutdown such tasks when the agent
> reregisters.
> 
> Note that if the master fails over between the time the framework
> completes and a partitioned agent re-registers, any framework tasks
> running on the agent will NOT be shutdown. This is a known bug; fixing
> it requires persisting the framework shutdown operation to the registry
> (MESOS-1719).
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 368ee1d5e97784fa54e0f141906405ee8f104317 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/tests/partition_tests.cpp 72013d1bfee275c6f3cb90173f0c408d55e0bc5d 
> 
> Diff: https://reviews.apache.org/r/54232/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Jan. 11, 2017, 12:11 a.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


Changes
---

Addressed BenH's comments, implemented approach with less churn.


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


Repository: mesos


Description
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
57eaf4f607d0628db466cc1a139772eeeaa51136 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
0e292a8b0d470f4e199db08f09a0c863d73c 

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


Testing
---

Testing details are found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 55395: Cleaned up `Master::updateTask` slightly.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 10, 2017, 10:35 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55395/
> ---
> 
> (Updated Jan. 10, 2017, 10:35 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6619
> https://issues.apache.org/jira/browse/MESOS-6619
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up `Master::updateTask` slightly.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
> 
> Diff: https://reviews.apache.org/r/55395/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 54183: Improved management of unreachable and completed tasks in master.

[Neil Conway]

> On Jan. 3, 2017, 10:25 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 5507
> > 
> >
> > can we inline this?
> 
> Neil Conway wrote:
> We could, but personally I find it more readable to make it a separate 
> function. The logic is a bit involved here (since we need to check two 
> different data structures due to backward compatibility requirements), and 
> there's already a lot going on in `Master::_reregisterSlave`.
> 
> Vinod Kone wrote:
> In general, we tend to factor out something into a function if it is 
> reusable and generic, not to reduce the number of lines someone has to read 
> in a function. In fact the argument was to inline in such cases because it 
> will avoid the need to context switch. 
> 
> Both `findPartitonAwareFrameworks` and `filterPartitionTasks` seem very 
> specific to the `_reregisterSlave` method, hence the suggestion.
> 
> For example, instead of looping through all the agent's tasks 3 times to 
> figure out which tasks to add to `Slave*`, it's probably easier to do it in 
> one inline loop here?

My goal isn't to reduce the # of lines to read the function, but to raise the 
level of abstraction a little bit. For example, `findPartitonAwareFrameworks` 
has to check _two_ data structures, depending on both agent version and whether 
the master has failed over. That is quite strange and takes a bit of thought to 
validate. With a separate function, the reader of `_reregisterSlave` doesn't 
need to think about those details if they don't want to -- instead they can 
just think "okay, we find all the partition-aware frameworks on the agent and 
then ..."

I'm fine with looping over the agent's tasks three times, because each of the 
three things we want to do is logically independent -- it takes a bit of 
thought to validate whether it is actually correct to combine them into a 
single loop (because each loop iteration would access a collection that is 
modified by the loop itself. It would actually be okay, but IMO current 
approach is safer).


- Neil


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


On Jan. 9, 2017, 10:39 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54183/
> ---
> 
> (Updated Jan. 9, 2017, 10:39 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6619
> https://issues.apache.org/jira/browse/MESOS-6619
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Before partition-awareness, when an agent failed health checks, the
> master removed the agent from the registry, marked all of its tasks
> TASK_LOST, and moved them to the `completedTasks` list in the master's
> memory. Although "lost" tasks might still be running, partitioned agents
> would only be allowed to re-register if the master failed over, in which
> case the `completedTasks` map would be emptied.
> 
> When partition-awareness was introduced, we initially followed the same
> scheme, with the only difference that partition-aware tasks are marked
> TASK_UNREACHABLE, not TASK_LOST.
> 
> This scheme has a few shortcomings. First, partition-aware tasks might
> resume running when the partitioned agent re-registers. Second, we
> re-added non-partition aware tasks when the agent re-registered but then
> marked them completed when the framework is shutdown, resulting in two
> entries in `completedTasks`.
> 
> This commit introduces a separate bounded map, `unreachableTasks`. These
> tasks are reported separately via the HTTP endpoints, because they have
> different semantics (unlike completed tasks, unreachable tasks can
> resume running). The size of this map is limited by a new master flag,
> `--max_unreachable_tasks_per_framework`. This commit also changes the
> master to omit re-adding non-partition-aware tasks on re-registering
> agents (unless the master has failed over): those tasks will shortly be
> shutdown anyway.
> 
> Finally, this commit fixes a minor bug in the previous code: the
> previous coding neglected to shutdown non-partition-aware frameworks
> running on pre-1.0 Mesos agents that re-register with the master after
> a network partition.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2113d06f58dddc0a28ae1241a24096266fe39801 
>   include/mesos/master/master.proto 03203c8ddd05876339b1c56789b7bb140499d49d 
>   include/mesos/v1/master/master.proto 
> f8edf39a68752c8601cece345f52bce8b9a8a68b 
>   src/master/constants.hpp 900b69460de9280f30b16b86925e436054e080ca 
>   src/master/flags.hpp 6a17b763dc76daa10073394f416b049e97a44238 
>   src/master/flags.cpp 

Re: Review Request 55394: Changed `Master::updateTask` to allow terminal tasks to be updated.

[Neil Conway]

> On Jan. 10, 2017, 11:51 p.m., Vinod Kone wrote:
> > What's the motivation for this change?

This is needed by https://reviews.apache.org/r/54232/ -- if we're going to 
update the task from `UNREACHABLE` to `KILLED`, we need `updateTask()` to be 
able to transition a task from one "terminal" state to another. (As a separate 
matter, we should remove TASK_UNREACHABLE from the list of states in 
`isTerminalState()`, but that should probably be done separately.)


- Neil


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


On Jan. 10, 2017, 10:34 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55394/
> ---
> 
> (Updated Jan. 10, 2017, 10:34 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6619
> https://issues.apache.org/jira/browse/MESOS-6619
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed `Master::updateTask` to allow terminal tasks to be updated.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
> 
> Diff: https://reviews.apache.org/r/55394/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 55394: Changed `Master::updateTask` to allow terminal tasks to be updated.

[Vinod Kone]

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



What's the motivation for this change?

- Vinod Kone


On Jan. 10, 2017, 10:34 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55394/
> ---
> 
> (Updated Jan. 10, 2017, 10:34 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6619
> https://issues.apache.org/jira/browse/MESOS-6619
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed `Master::updateTask` to allow terminal tasks to be updated.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
> 
> Diff: https://reviews.apache.org/r/55394/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 54183: Improved management of unreachable and completed tasks in master.

[Vinod Kone]

> On Jan. 3, 2017, 10:25 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 5496
> > 
> >
> > it's not necessary that the agent has failed over if we are here right?
> 
> Neil Conway wrote:
> Do you mean "it's not necessary that the master has failed over if we are 
> here"?
> 
> If so, that's right: the master may or may not have re-registered in the 
> time since the agent was marked unreachable. The comment talks about how 
> these two cases are handled differently.

I forgot what I was asking here originally :( Lets drop it for now.


> On Jan. 3, 2017, 10:25 p.m., Vinod Kone wrote:
> > src/master/master.hpp, lines 2634-2641
> > 
> >
> > this reads like `unreachableTasks` are completed tasks of PA 
> > frameworks. can you split this comment up and move the unreachable specific 
> > comments to #2644?
> 
> Neil Conway wrote:
> I added an additional comment describing the purpose of 
> `unreachableTasks`, but I felt like it was useful to still keep a note in the 
> `completedTasks` comment to describe which tasks are stored in 
> `unreachableTasks` vs `completedTasks` when an agent is marked unreachable. 
> Let me know if you think this is still confusing.

lgtm.


> On Jan. 3, 2017, 10:25 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 5507
> > 
> >
> > can we inline this?
> 
> Neil Conway wrote:
> We could, but personally I find it more readable to make it a separate 
> function. The logic is a bit involved here (since we need to check two 
> different data structures due to backward compatibility requirements), and 
> there's already a lot going on in `Master::_reregisterSlave`.

In general, we tend to factor out something into a function if it is reusable 
and generic, not to reduce the number of lines someone has to read in a 
function. In fact the argument was to inline in such cases because it will 
avoid the need to context switch. 

Both `findPartitonAwareFrameworks` and `filterPartitionTasks` seem very 
specific to the `_reregisterSlave` method, hence the suggestion.

For example, instead of looping through all the agent's tasks 3 times to figure 
out which tasks to add to `Slave*`, it's probably easier to do it in one inline 
loop here?


> On Jan. 3, 2017, 10:25 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 5523
> > 
> >
> > not sure if this is worth making into a function?
> 
> Neil Conway wrote:
> I thought it was a bit more readable (against, `Master::_reregisterSlave` 
> has a lot of stuff going on), but happy to change it if you disagree.

see above.


> On Jan. 3, 2017, 10:25 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 5539
> > 
> >
> > what if the agent was marked unreachable by the previous master? do we 
> > still want to add all its tasks?
> 
> Neil Conway wrote:
> If the master has failed over, all tasks on the agent will be allowed to 
> continue running (whether partition-aware or not) -- so that's why we re-add 
> all tasks here.

I see.


> On Jan. 3, 2017, 10:25 p.m., Vinod Kone wrote:
> > src/tests/partition_tests.cpp, lines 1819-1847
> > 
> >
> > hmm. it is unfortunate that we send TASK_LOST followed by TASK_FINISHED 
> > and then send TASK_LOST on reconciliation. can you remind me why the master 
> > forwards status updates for unknown tasks? looks like it can just drop them 
> > if the reason for doing so is no longer valid.
> 
> Neil Conway wrote:
> I don't know why we forward status updates for unknown tasks. I can see 
> it having some value, though -- in the scenario above, the first `TASK_LOST` 
> just means the agent become unreachable. `TASK_FINISHED` tells the framework 
> that the task actually did complete successfully. I can imagine situations 
> where, say, the framework ignores `TASK_LOST` or uses a large timeout before 
> assuming the task is "really lost", whereas `TASK_FINISHED` / `TASK_KILLED` / 
> etc. is a more definitive signal.

I see. Atleat the PA world is much saner. TASK_UNREACHABLE -> TASK_FINISHED -> 
TASK_UNKNOWN


- Vinod


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


On Jan. 9, 2017, 10:39 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54183/
> 

Re: Review Request 55296: Used `jsonify` in `operator<<` for `JSON::*` to reduce duplicate code.

[Michael Park]

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

(Updated Jan. 10, 2017, 3:22 p.m.)


Review request for mesos, Alexander Rojas and Joris Van Remoortere.


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


Repository: mesos


Description
---

The printing logic for `json.hpp` and `jsonify.hpp` are currently
duplicated. We can reduce this duplication by leveraging `jsonify` for
the implementation of `operator<<` for `JSON::*`. Since `JSON::Value`s
are not generally used for printing, there should be no performance
concerns here.


Diffs
-

  3rdparty/stout/include/stout/json.hpp 
62ce15274677112d142a3c829b4a9f06258c9e2c 

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


Testing
---


Thanks,

Michael Park

Re: Review Request 54232: Shutdown tasks of completed frameworks on agent re-registration.

[Neil Conway]

> On Jan. 3, 2017, 10:51 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 5512
> > 
> >
> > inline this?

To me, using a separate function was more readable than writing the logic 
inline. Happy to change it if you disagree, though.


- Neil


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


On Jan. 10, 2017, 10:32 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54232/
> ---
> 
> (Updated Jan. 10, 2017, 10:32 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6602
> https://issues.apache.org/jira/browse/MESOS-6602
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, if a framework completed (e.g., due to a teardown operation
> or framework shutdown), any framework tasks running on partitioned
> agents would not be shutdown when the agent re-registered. For tasks
> that are not partition-aware, the task would be shutdown on agent
> re-registration anyway. But for partition-aware tasks, this could lead
> to orphan tasks.
> 
> Fix this by changing the master to shutdown such tasks when the agent
> reregisters.
> 
> Note that if the master fails over between the time the framework
> completes and a partitioned agent re-registers, any framework tasks
> running on the agent will NOT be shutdown. This is a known bug; fixing
> it requires persisting the framework shutdown operation to the registry
> (MESOS-1719).
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 368ee1d5e97784fa54e0f141906405ee8f104317 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/tests/partition_tests.cpp 72013d1bfee275c6f3cb90173f0c408d55e0bc5d 
> 
> Diff: https://reviews.apache.org/r/54232/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 54793: Prevented task launches that reuse unreachable task IDs.

[Neil Conway]

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

(Updated Jan. 10, 2017, 10:36 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Unbreak test.


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


Repository: mesos


Description
---

The master keeps an in-memory cache of task IDs that have recently been
marked unreachable. The master now consults this cache to reject task
launch attempts that reuse one of these recently unreachable task IDs
(such tasks are not terminal and may resume running in the future). This
check is not complete (we won't detect all cases in which unreachable
task IDs are reused), but preventing this from happening in the common
case seems worth doing. See MESOS-6785 for details.


Diffs (updated)
-

  src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f 
  src/tests/partition_tests.cpp 72013d1bfee275c6f3cb90173f0c408d55e0bc5d 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 54407: Refactored Master::removeFramework to use Master::deactivate.

[Neil Conway]

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

(Updated Jan. 10, 2017, 10:35 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Refactored Master::removeFramework to use Master::deactivate.


Diffs (updated)
-

  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 54312: Added TASK_UNREACHABLE to master's state-summary endpoint.

[Neil Conway]

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

(Updated Jan. 10, 2017, 10:35 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added TASK_UNREACHABLE to master's state-summary endpoint.


Diffs (updated)
-

  src/master/http.cpp 75dcd6199dbfcca6260baed49b838a45312bb667 
  src/tests/partition_tests.cpp 72013d1bfee275c6f3cb90173f0c408d55e0bc5d 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 55395: Cleaned up `Master::updateTask` slightly.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Cleaned up `Master::updateTask` slightly.


Diffs
-

  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 55394: Changed `Master::updateTask` to allow terminal tasks to be updated.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Changed `Master::updateTask` to allow terminal tasks to be updated.


Diffs
-

  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 54232: Shutdown tasks of completed frameworks on agent re-registration.

[Neil Conway]

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

(Updated Jan. 10, 2017, 10:32 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Mark unreachable tasks as completed in `removeFramework`.


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


Repository: mesos


Description
---

Previously, if a framework completed (e.g., due to a teardown operation
or framework shutdown), any framework tasks running on partitioned
agents would not be shutdown when the agent re-registered. For tasks
that are not partition-aware, the task would be shutdown on agent
re-registration anyway. But for partition-aware tasks, this could lead
to orphan tasks.

Fix this by changing the master to shutdown such tasks when the agent
reregisters.

Note that if the master fails over between the time the framework
completes and a partitioned agent re-registers, any framework tasks
running on the agent will NOT be shutdown. This is a known bug; fixing
it requires persisting the framework shutdown operation to the registry
(MESOS-1719).


Diffs (updated)
-

  src/master/master.hpp 368ee1d5e97784fa54e0f141906405ee8f104317 
  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
  src/tests/partition_tests.cpp 72013d1bfee275c6f3cb90173f0c408d55e0bc5d 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 54081: Added `--pidfile` option to master and agent binaries.

[Greg Mann]

> On Dec. 6, 2016, 10:42 p.m., Benjamin Mahler wrote:
> > Thanks Ilya!
> > 
> > Have you looked at other pidfile related libraries? Looks like BSD provides 
> > some functions for this (they're also available on Linux):
> > https://www.freebsd.org/cgi/man.cgi?query=pidfile=3=FreeBSD+6.1-RELEASE
> > https://linux.die.net/man/3/pidfile
> > 
> > It would be great to provide an os-agnostic library within stout for this, 
> > e.g.:
> > 
> > ```
> > pidfile::open()
> > pidfile::close()
> > pidfile::remove()
> > ```
> > 
> > I haven't looked too deeply at what we should have these take and return, 
> > but I noticed the use of RAII seemed unnecessary in your patch (only the 
> > .cpp implementation uses the exposed RAII class? If so, perhaps we can just 
> > leave RAII out for now since the callers do not use it). Also it would be 
> > great to which process is already running (which is provided by the 
> > BSD/Linux functions). Also, another suggestion as a first step here is to 
> > avoid Windows support entirely for now if we don't implement the locking 
> > aspect of this (as that seems necessary for this to be safely usable?)
> > 
> > We could structure the patches like this:
> > 
> > (1) Addition of pidfile utilities to stout.
> > (2) Tests of the pidfile utilities in stout (should be possible?)
> > (3) Integration of the pidfile utilities into mesos (manually test since 
> > the logic will reside in the mains?)
> 
> Ilya Pronin wrote:
> Thanks for the feedback!
> 
> > Looks like BSD provides some functions for this (they're also available 
> on Linux)
> 
> Yes, I saw `libbsd` but since it's not part of POSIX I decided to make 
> things portable.
> 
> > Also it would be great to which process is already running
> 
> Good idea! I will add it.
> 
> > I noticed the use of RAII seemed unnecessary in your patch (only the 
> .cpp implementation uses the exposed RAII class? If so, perhaps we can just 
> leave RAII out for now since the callers do not use it).
> 
> My intention was to try to be nice and clean up the PID file when daemon 
> exits through `std::exit()` or terminates on `SIGTERM`. For handling the 
> first case I created a static RAII object so its destructor will be called 
> during `std::exit()` cleanup. We could place this object in `main()`. But 
> here comes the second case. Our daemons perform default `SIGTERM` action 
> which is `term`. There's a signal handler in `logging/logging.cpp` where we 
> can unlink the file. For that I introduced a helper function and placed the 
> RAII object inside `common/pid_file.cpp`.
> 
> So I think a RAII class is a good thing for implementing such thing. I'm 
> OK with moving it to stout and restructuring patches.
> 
> Also I just thought that I should check if the destructor is executing 
> inside the process that created the PID file to protect from unlinking by 
> forked processes.
> 
> What do you think?
> 
> Alexander Rukletsov wrote:
> A problem with the RAII class here is that we sometimes manually have to 
> release the lock (in case of a signal). When I look at this patch I read 
> something like "Hey, you don't have to close & remove the pidfile explicitly, 
> we'll do that for you... well, unless your process is signaled". I'm inclined 
> to require users call `removePidFile()` explicitly to avoid creaing a false 
> feeling of safety.
> 
> Another question: Why do you want to expose `PIDFile` class in the `.hpp`?
> 
> Ilya Pronin wrote:
> Well, I wrote it more like "you can remove it manually if you want but if 
> you just forget about it nothing bad will happen" :) The lock will be 
> released automatically when the fd is closed (e.g. on process termination).
> 
> I think current `SIGTERM` handling is an exceptional case here (even 
> though I assume it is the most frequent way the daemon terminates). Another 
> such case could be calling `std::_Exit()`. These are situations when usual 
> cleanup is not performed and some manual actions may be required.
> 
> Do we want to always do cleanup manually and explicitly? If this is the 
> accepted way in Mesos then of course RAII is not appliable here.
> 
> > Why do you want to expose PIDFile class in the .hpp?
> 
> For testing. Although there's only 1 simple test.
> 
> Benjamin Mahler wrote:
> Ah ok, I see all of the cases you were looking to cover now!
> 
> While we figure out if we can leverage RAII and a static global smart 
> pointer, we can start with a simple set of function utilities in stout (such 
> that one could build the RAII wrapper on top of them, if needed).
> 
> For the static smart pointer, we currently follow Google's style guide 
> rule to ban "static non-POD variables" in order to avoid destructor ordering 
> issues and access during/after destruction issues:
> 
> 

Re: Review Request 55388: Added `system` environment variables in `mesos-docker-executor`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [55388]

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 Jan. 10, 2017, 7:01 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55388/
> ---
> 
> (Updated Jan. 10, 2017, 7:01 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `system` environment variables in `mesos-docker-executor`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 
> 
> Diff: https://reviews.apache.org/r/55388/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 55177: Fixed scheme handling in URL::parse().

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Jan. 6, 2017, 11:37 a.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55177/
> ---
> 
> (Updated Jan. 6, 2017, 11:37 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Timothy Chen.
> 
> 
> Bugs: MESOS-6871
> https://issues.apache.org/jira/browse/MESOS-6871
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This method could mistake the host part for the scheme because of
> unsuitable use of find_first_of().
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/http.cpp 97d1424be20e217401519c2bee79857bcf087023 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> d168ad15da4706fb9c3aa11a125b7dfb5987570d 
> 
> Diff: https://reviews.apache.org/r/55177/diff/
> 
> 
> Testing
> ---
> 
> Added a test case demonstrating the problem. Ran `make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 55381: Added test for framework upgrading to multi-role capability.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [55381]

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

Error:
2017-01-10 19:23:04 URL:https://reviews.apache.org/r/55381/diff/raw/ 
[2169/2169] -> "55381.patch" [1]
error: patch failed: src/tests/master_validation_tests.cpp:2608
error: src/tests/master_validation_tests.cpp: patch does not apply

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

- Mesos ReviewBot


On Jan. 10, 2017, 2:50 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55381/
> ---
> 
> (Updated Jan. 10, 2017, 2:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6900
> https://issues.apache.org/jira/browse/MESOS-6900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55381/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 55388: Added `system` environment variables in `mesos-docker-executor`.

[Daniel Pravat]

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

Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joseph 
Wu, and Michael Park.


Repository: mesos


Description
---

Added `system` environment variables in `mesos-docker-executor`.


Diffs
-

  src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 

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


Testing
---


Thanks,

Daniel Pravat

Re: Review Request 53712: Added `system` environment variables in ` execvpe.cpp`.

[Daniel Pravat]

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

(Updated Jan. 10, 2017, 7:01 p.m.)


Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joseph 
Wu, and Michael Park.


Summary (updated)
-

Added `system` environment variables in ` execvpe.cpp`.


Repository: mesos


Description (updated)
---

Added `system` environment variables in ` execvpe.cpp`.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.cpp 
e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 

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


Testing
---


Thanks,

Daniel Pravat

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [55271]

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 Jan. 10, 2017, 2:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> ---
> 
> (Updated Jan. 10, 2017, 2:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
> https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> ---
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 55381: Added test for framework upgrading to multi-role capability.

[Benjamin Bannier]

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

Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/master_validation_tests.cpp 
e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 

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


Testing
---

make check (OS X)


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

[Benjamin Bannier]

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

(Updated Jan. 10, 2017, 3:49 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
---

Fixed upgrade to multi-role.


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


Repository: mesos


Description
---

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-

  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
  src/tests/master_validation_tests.cpp 
e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 

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


Testing
---

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55269: Avoided use of CHECK macros.

[Benjamin Bannier]

> On Jan. 7, 2017, 12:28 a.m., Kapil Arya wrote:
> > LGTM. Have you run any tests where the existing `CHECK_SOME` would fail and 
> > the new code will behave properly? It might be worth running it at least 
> > for one test. Maybe, in one of the containerizer value, just set it to 
> > `Error()` to validate the idea.
> 
> Benjamin Bannier wrote:
> I tested this in internal CI on an incompletly configured machine; after 
> this fix I was able to see failures in e.g., from `PortMappingIsolatorTest` 
> where previously the code would have just aborted. As a test I also redefined 
> to be just `return Error("")` which did not lead to aborts of the test run 
> via `HTTPCommandExecutorTest.TerminateWithACK` or 
> `ContainerLoggerTest.DefaultToSandbox` anymore.

Duh, 

> I tested this in internal CI on an incompletly configured machine; after this 
> fix I was able to see failures from e.g., PortMappingIsolatorTest where 
> previously the code would have just aborted. As a test I also redefined 
> `MesosContainerizer::create` to be just return Error("") which did not lead 
> to aborts of the test run via HTTPCommandExecutorTest.TerminateWithACK or 
> ContainerLoggerTest.DefaultToSandbox anymore.

FTFY, sorry.


- Benjamin


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


On Jan. 10, 2017, 2:19 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55269/
> ---
> 
> (Updated Jan. 10, 2017, 2:19 p.m.)
> 
> 
> Review request for mesos and Kapil Arya.
> 
> 
> Bugs: MESOS-6860
> https://issues.apache.org/jira/browse/MESOS-6860
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoided use of CHECK macros.
> 
> 
> Diffs
> -
> 
>   src/tests/command_executor_tests.cpp 
> f4e7044b82e8e81d6430551dc0b2a288db10bc3c 
>   src/tests/container_logger_tests.cpp 
> 7ac83338b5944967d0cbe768bf622c654fee99e1 
>   src/tests/containerizer/cgroups_tests.cpp 
> 0afaec6ae948cabf1472bf01103210d8f9809cb1 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 207742f165e2c91a917ec293689c9030931f29db 
>   src/tests/cram_md5_authentication_tests.cpp 
> 1f414282b8d2aba7403f8617a30fa15c4a694d02 
>   src/tests/credentials_tests.cpp ed50609cc5ed1937d2bcd782451245e0b5889ed5 
>   src/tests/disk_quota_tests.cpp 90c42b301d749e7a9c6d6d42a51e929ec66d761c 
>   src/tests/fetcher_cache_tests.cpp 03e817d46194d451eceb70f4cebb54dfdcb4c2e7 
>   src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b 
>   src/tests/partition_tests.cpp 72013d1bfee275c6f3cb90173f0c408d55e0bc5d 
>   src/tests/paths_tests.cpp a8aa5c26db7bd21f54eabaddf13a86582abaa2c3 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 1d40380dedbd6f0e1402b9a37d8a231cf73881f7 
>   src/tests/persistent_volume_tests.cpp 
> 8198b6b5ad323d17835ba067c7ff3d34ef948125 
>   src/tests/reservation_endpoints_tests.cpp 
> d94fe29e5972e6ed0011ca00cc56fe1c20cda495 
>   src/tests/slave_tests.cpp 152c53ff102a081ce5c3b74984720fda8b791811 
> 
> Diff: https://reviews.apache.org/r/55269/diff/
> 
> 
> Testing
> ---
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55269: Avoided use of CHECK macros.

[Benjamin Bannier]

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

(Updated Jan. 10, 2017, 2:19 p.m.)


Review request for mesos and Kapil Arya.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Avoided use of CHECK macros.


Diffs (updated)
-

  src/tests/command_executor_tests.cpp f4e7044b82e8e81d6430551dc0b2a288db10bc3c 
  src/tests/container_logger_tests.cpp 7ac83338b5944967d0cbe768bf622c654fee99e1 
  src/tests/containerizer/cgroups_tests.cpp 
0afaec6ae948cabf1472bf01103210d8f9809cb1 
  src/tests/containerizer/port_mapping_tests.cpp 
207742f165e2c91a917ec293689c9030931f29db 
  src/tests/cram_md5_authentication_tests.cpp 
1f414282b8d2aba7403f8617a30fa15c4a694d02 
  src/tests/credentials_tests.cpp ed50609cc5ed1937d2bcd782451245e0b5889ed5 
  src/tests/disk_quota_tests.cpp 90c42b301d749e7a9c6d6d42a51e929ec66d761c 
  src/tests/fetcher_cache_tests.cpp 03e817d46194d451eceb70f4cebb54dfdcb4c2e7 
  src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b 
  src/tests/partition_tests.cpp 72013d1bfee275c6f3cb90173f0c408d55e0bc5d 
  src/tests/paths_tests.cpp a8aa5c26db7bd21f54eabaddf13a86582abaa2c3 
  src/tests/persistent_volume_endpoints_tests.cpp 
1d40380dedbd6f0e1402b9a37d8a231cf73881f7 
  src/tests/persistent_volume_tests.cpp 
8198b6b5ad323d17835ba067c7ff3d34ef948125 
  src/tests/reservation_endpoints_tests.cpp 
d94fe29e5972e6ed0011ca00cc56fe1c20cda495 
  src/tests/slave_tests.cpp 152c53ff102a081ce5c3b74984720fda8b791811 

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


Testing
---

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55268: Avoided use of CHECK macros in libprocess.

[Benjamin Bannier]

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

(Updated Jan. 10, 2017, 2:19 p.m.)


Review request for mesos and Kapil Arya.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Avoided use of CHECK macros in libprocess.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/http_tests.cpp 
96ade15b36c3140bc1c6d8b230f0e26ff979e921 

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


Testing
---

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55269: Avoided use of CHECK macros.

[Benjamin Bannier]

> On Jan. 7, 2017, 12:28 a.m., Kapil Arya wrote:
> > LGTM. Have you run any tests where the existing `CHECK_SOME` would fail and 
> > the new code will behave properly? It might be worth running it at least 
> > for one test. Maybe, in one of the containerizer value, just set it to 
> > `Error()` to validate the idea.

I tested this in internal CI on an incompletly configured machine; after this 
fix I was able to see failures in e.g., from `PortMappingIsolatorTest` where 
previously the code would have just aborted. As a test I also redefined to be 
just `return Error("")` which did not lead to aborts of the test run via 
`HTTPCommandExecutorTest.TerminateWithACK` or 
`ContainerLoggerTest.DefaultToSandbox` anymore.


> On Jan. 7, 2017, 12:28 a.m., Kapil Arya wrote:
> > src/tests/master_quota_tests.cpp, lines 92-94
> > 
> >
> > Should we put it in a separate RR?

I felt this was just a refactoring similar to the instances of `return 
Error(...)` I added elsewhere in this patch, so I'd say no. Would you feel this 
would warrant its own patch?


- Benjamin


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


On Jan. 6, 2017, 4:16 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55269/
> ---
> 
> (Updated Jan. 6, 2017, 4:16 p.m.)
> 
> 
> Review request for mesos and Kapil Arya.
> 
> 
> Bugs: MESOS-6860
> https://issues.apache.org/jira/browse/MESOS-6860
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoided use of CHECK macros.
> 
> 
> Diffs
> -
> 
>   src/tests/command_executor_tests.cpp 
> f4e7044b82e8e81d6430551dc0b2a288db10bc3c 
>   src/tests/container_logger_tests.cpp 
> a8bc83449816ec8ba3f2b7e02594e4abe39c265d 
>   src/tests/containerizer/cgroups_tests.cpp 
> 0afaec6ae948cabf1472bf01103210d8f9809cb1 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 207742f165e2c91a917ec293689c9030931f29db 
>   src/tests/cram_md5_authentication_tests.cpp 
> 1f414282b8d2aba7403f8617a30fa15c4a694d02 
>   src/tests/credentials_tests.cpp ed50609cc5ed1937d2bcd782451245e0b5889ed5 
>   src/tests/disk_quota_tests.cpp 90c42b301d749e7a9c6d6d42a51e929ec66d761c 
>   src/tests/fetcher_cache_tests.cpp 03e817d46194d451eceb70f4cebb54dfdcb4c2e7 
>   src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b 
>   src/tests/partition_tests.cpp 3afdbba13a9753edce17b24ff6d1898d3cc6de62 
>   src/tests/paths_tests.cpp a8aa5c26db7bd21f54eabaddf13a86582abaa2c3 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 1d40380dedbd6f0e1402b9a37d8a231cf73881f7 
>   src/tests/persistent_volume_tests.cpp 
> 8198b6b5ad323d17835ba067c7ff3d34ef948125 
>   src/tests/reservation_endpoints_tests.cpp 
> d94fe29e5972e6ed0011ca00cc56fe1c20cda495 
>   src/tests/slave_tests.cpp d633a74d6b342239fbca0b44eec281eb315df5ff 
> 
> Diff: https://reviews.apache.org/r/55269/diff/
> 
> 
> Testing
> ---
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

[Benjamin Bannier]

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

(Updated Jan. 10, 2017, 1:29 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
---

Addressed gyliu's review comments.


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


Repository: mesos


Description
---

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-

  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
  src/tests/master_validation_tests.cpp 
e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 

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


Testing
---

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55296: Used `jsonify` in `operator<<` for `JSON::*` to reduce duplicate code.

[Alexander Rojas]

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


Ship it!




Ship It!

- Alexander Rojas


On Jan. 10, 2017, 1:36 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55296/
> ---
> 
> (Updated Jan. 10, 2017, 1:36 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The printing logic for `json.hpp` and `jsonify.hpp` are currently
> duplicated. We can reduce this duplication by leveraging `jsonify` for
> the implementation of `operator<<` for `JSON::*`. Since `JSON::Value`s
> are not generally used for printing, there should be no performance
> concerns here.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/json.hpp 
> 62ce15274677112d142a3c829b4a9f06258c9e2c 
> 
> Diff: https://reviews.apache.org/r/55296/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 53468: Show the pailer at the right side in WebUI.

[haosdent huang]

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

(Updated Jan. 10, 2017, 8:03 a.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Show the pailer at the right side in WebUI.


Diffs
-

  src/webui/master/static/browse.html b9849197227b06df348789a49348e2b5d4cfd2ae 
  src/webui/master/static/css/mesos.css 
5b1227e9d64757f9fc106e497f7fa3ed72112c10 
  src/webui/master/static/js/controllers.js 
dd0cc810748a9bd378d9c6b15e9fe89b7c0f11d9 
  src/webui/master/static/js/jquery.pailer.js 
537fe002166167748a6357a50f4b7dd04b1f4257 
  src/webui/master/static/pailer.html 19e0981143bd7e8372b49f4f036867e9dd05727a 

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


Testing
---


Thanks,

haosdent huang