Re: Review Request 55895: Extract a BasicBlocks class for disk block arithmetic.

[Jiang Yan Xu]

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




src/slave/containerizer/mesos/isolators/xfs/utils.hpp
Lines 46 (patched)


s/b/bytes/



src/slave/containerizer/mesos/isolators/xfs/utils.hpp
Lines 49 (patched)


s/c/count/

or I wonder if 

s/c/blocks/ makes more sense? 

Not `count` isn't ok but you named the method `uint64_t blocks() const` and 
not `uint64_t count() const` after all?

So for consistency stick to one?



src/slave/containerizer/mesos/isolators/xfs/utils.hpp
Lines 52 (patched)


A blank line above looks better with your current method grouping.



src/slave/containerizer/mesos/isolators/xfs/utils.hpp
Lines 58 (patched)


Styling issues: 

- Space after `;`.
- s/block_size/blockSize/ (We don't use snake casing in Mesos unless it's a 
constant (then it's capitalized).

(we do use snake casing in libprocess and stout, sigh, don't ask me why)

However why a method? It's semantically a constant value so it's more 
idiomatic in Mesos to use a variable. Just move `static constexpr Bytes 
BASIC_BLOCK_SIZE = Bytes(512u);` into the class?



src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Lines 252-254 (original), 248-250 (patched)


This is not intuitive to me. 

The assignments above `quota.d_blk_hardlimit = 
BasicBlocks(limit).blocks();` is very clear that `d_blk_hardlimit` is the 
number of blocks.

Here `info.limit = BasicBlocks(quota.d_blk_hardlimit);` looks like 
`info.limit`'s unit is blocks but it's not due to the implicit conversion.

How about we remove the implicit conversion to `Bytes` and just use the 
method `Bytes bytes() const`?



src/tests/containerizer/xfs_quota_tests.cpp
Lines 944 (patched)


Unless there's useful variables printed out, we typically capture these as 
comments in tests. (See the rest of this file)

Comments give you a bit more flexibility in the placement and structure 
while attaching to stdout allows you to see it in the log.

Consistency is more important though so let's not start a new style here 
yet. Let's chat if you feel strongly about advocating for this style.



src/tests/containerizer/xfs_quota_tests.cpp
Lines 948 (patched)


Be consistent in the use of unsigned literal.


- Jiang Yan Xu


On April 25, 2017, 3:37 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55895/
> ---
> 
> (Updated April 25, 2017, 3:37 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5116
> https://issues.apache.org/jira/browse/MESOS-5116
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a BasicBlocks class to handle disk blocks to clarify block-based
> arithmetic in the XFS disk isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
> eddd4c37fb42339ca21ecb392dea47acf6b277bb 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp 
> 8018ad348d26bd962357543a5fb9f6cb43ff13b1 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> 7beb60b059910a0d4451b1ace895a35dc974a043 
> 
> 
> Diff: https://reviews.apache.org/r/55895/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58763: Added volume/secret isolator.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58758, 58759, 58760, 58761, 58762, 58763]

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 April 28, 2017, 6:54 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58763/
> ---
> 
> (Updated April 28, 2017, 6:54 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7418
> https://issues.apache.org/jira/browse/MESOS-7418
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is still Work in Progress.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 29da17bee13226e18757e2ad3a7a091427fd35f4 
>   src/slave/containerizer/containerizer.hpp 
> 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41 
>   src/slave/containerizer/containerizer.cpp 
> 9024371b6c4228f0903cfeef3bbec736e1a425f8 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 56db7eb1193c9812b62b9149c9c2b2dd9b66701c 
>   src/slave/containerizer/mesos/isolators/volume/secret.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/volume/secret.cpp PRE-CREATION 
>   src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d 
>   src/tests/cluster.cpp a4f57e0e9c4f221c42bdbb36d8925f688b3d742f 
>   src/tests/mock_slave.hpp 767ed3d0444258f30b7338981ccc0080166ac5a7 
>   src/tests/mock_slave.cpp c435ec76ac44775bf8a87421640649495c16187e 
> 
> 
> Diff: https://reviews.apache.org/r/58763/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 58869: Bumped the default timeout value for docker volume detach operation.

[Jie Yu]

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

(Updated April 29, 2017, 12:39 a.m.)


Review request for mesos and Gilbert Song.


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


Repository: mesos


Description
---

A volume detach operation can be very time consuming in some cloud
environment. In extreme cases, the detach can take up to 20 minutes.
Since 'Unmount' in Docker Volume Driver Interface (DVDI) is not
idempotent, the container orchestrator (i.e., Mesos) cannot safely
retry the operation. As a result, we don't have much flexibility here.
This patch increases the timeout value hoping the operation will
finish within this new timeout value in most of the cases.


Diffs
-

  src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 
18a8066b064835cbd7a2cef6d1a43462bdd5e87c 


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


Testing
---

make check


Thanks,

Jie Yu

Review Request 58869: Bumped the default timeout value for docker volume detach operation.

[Jie Yu]

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

Review request for mesos and Gilbert Song.


Repository: mesos


Description
---

A volume detach operation can be very time consuming in some cloud
environment. In extreme cases, the detach can take up to 20 minutes.
Since 'Unmount' in Docker Volume Driver Interface (DVDI) is not
idempotent, the container orchestrator (i.e., Mesos) cannot safely
retry the operation. As a result, we don't have much flexibility here.
This patch increases the timeout value hoping the operation will
finish within this new timeout value in most of the cases.


Diffs
-

  src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 
18a8066b064835cbd7a2cef6d1a43462bdd5e87c 


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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 58357: Support more test frameworks in test-upgrade script.

[Zhitao Li]

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

(Updated April 29, 2017, 12:17 a.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Greg Mann.


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


Repository: mesos


Description
---

This patch added support to java and python based test framework in
`test-upgrade.py` script.


Diffs (updated)
-

  support/test-upgrade.py 84df21dd6e63653e1a18e700ef904aa3a04b2b45 


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

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


Testing
---

Ran this on all three languages options for cpp, java and python.


Thanks,

Zhitao Li

Re: Review Request 58828: Replaced std::set with hashset for framework roles.

[Benjamin Mahler]

> On April 28, 2017, 6:30 a.m., Michael Park wrote:
> > It's true that we don't use `std::set` for its ordering in the general case,
> > however, I think the ordering does help produce easier-to-read error 
> > messages.
> > 
> > 
> > What do you think of preserving that property?

Yes you're right, I forgot that was the original reason I suggested std::set, 
given we log it. My intention with this change was to order it when we need to 
(before logging it), but seems it is not worth the trouble, I'll drop this.


- Benjamin


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


On April 28, 2017, 12:43 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58828/
> ---
> 
> (Updated April 28, 2017, 12:43 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since we do not need ordering, this updates the framework roles to
> be stored using hashset instead of std::set.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 
>   src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
>   src/master/allocator/mesos/hierarchical.hpp 
> 219f508dacb3b372c12da3f15e07a3bf5f27e6e6 
>   src/master/allocator/mesos/hierarchical.cpp 
> 984a0a4e2671ee7bb2a3515849342f49f2c4e3aa 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
>   src/master/master.cpp a0cb505a1300637f42fa8062c57deb1590779dd4 
>   src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 
>   src/tests/protobuf_utils_tests.cpp 5239182812835b93a28e85146b2df2b20ae77328 
> 
> 
> Diff: https://reviews.apache.org/r/58828/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 56732: Remove unnecessary perf version checks.

[Jiang Yan Xu]

> On April 28, 2017, 4:06 p.m., Jiang Yan Xu wrote:
> > src/linux/perf.cpp
> > Line 322 (original), 313 (patched)
> > 
> >
> > Now without the `process::collect`.
> > 
> > 1. Remove from includes:
> > ```
> > #include 
> > ```
> > 
> > 2. We can just inline the lambda (the way we usually do this)
> > 
> > ```
> >   return output
> > .then([start, duration](const string output)
> > -> Future> {
> >   Try> result =
> > perf::parse(output);
> > 
> >   if (result.isError()) {
> > return Failure("Failed to parse perf sample: " + 
> > result.error());
> >   }
> > 
> >   foreachvalue (mesos::PerfStatistics& statistics, result.get()) {
> > statistics.set_timestamp(start.secs());
> > statistics.set_duration(duration.secs());
> >   }
> > 
> >   return result.get();
> > });
> > ```

Apparently we have to keep `#include ` for `process:await`.


- Jiang Yan


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


On Feb. 15, 2017, 3:22 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56732/
> ---
> 
> (Updated Feb. 15, 2017, 3:22 p.m.)
> 
> 
> Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7049
> https://issues.apache.org/jira/browse/MESOS-7049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We can remove the version check from the perf subprocess
> wrapper because we longer uses the version to figure out
> how to parse the stat output and the callers explicitly
> check perf::supported().
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp 9c4330b6086abb18f03660fe89a6fb01d8ed 
>   src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb 
>   src/tests/containerizer/perf_tests.cpp 
> d536ecc5cb24787bc3487efb146573cd4f3ded43 
> 
> 
> Diff: https://reviews.apache.org/r/56732/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 56732: Remove unnecessary perf version checks.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Committing with the minor changes if no objection.


src/linux/perf.cpp
Line 322 (original), 313 (patched)


Now without the `process::collect`.

1. Remove from includes:
```
#include 
```

2. We can just inline the lambda (the way we usually do this)

```
  return output
.then([start, duration](const string output)
-> Future> {
  Try> result =
perf::parse(output);

  if (result.isError()) {
return Failure("Failed to parse perf sample: " + result.error());
  }

  foreachvalue (mesos::PerfStatistics& statistics, result.get()) {
statistics.set_timestamp(start.secs());
statistics.set_duration(duration.secs());
  }

  return result.get();
});
```


- Jiang Yan Xu


On Feb. 15, 2017, 3:22 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56732/
> ---
> 
> (Updated Feb. 15, 2017, 3:22 p.m.)
> 
> 
> Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7049
> https://issues.apache.org/jira/browse/MESOS-7049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We can remove the version check from the perf subprocess
> wrapper because we longer uses the version to figure out
> how to parse the stat output and the callers explicitly
> check perf::supported().
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp 9c4330b6086abb18f03660fe89a6fb01d8ed 
>   src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb 
>   src/tests/containerizer/perf_tests.cpp 
> d536ecc5cb24787bc3487efb146573cd4f3ded43 
> 
> 
> Diff: https://reviews.apache.org/r/56732/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58821: Added a test that verifies a task and its check share the work dir.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58847, 58262, 58718, 58817, 58818, 58819, 58820, 58263, 58821]

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 April 28, 2017, 9:14 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58821/
> ---
> 
> (Updated April 28, 2017, 9:14 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7433
> https://issues.apache.org/jira/browse/MESOS-7433
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp ec0d5ee78a94991c68f38174d09b41e8f2e4be35 
> 
> 
> Diff: https://reviews.apache.org/r/58821/diff/2/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS and Fedora 24
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58673: Fix FlagsFileTest to check for absolute path properly on Windows.

[Jeff Coffler]

> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/flags/parse.hpp
> > Line 96 (original), 96 (patched)
> > 
> >
> > There are more instances of erroneous absolute path checking through 
> > the code base, some of which we should probably fix.
> > 
> > That said, this change should probably be in a separate commit/patch to 
> > address the particular bug, followed by more patches for the other 
> > instances (which don't need to come immediately). But I don't think I'd 
> > include this directly with the porting of `path::absolute`.
> 
> Joseph Wu wrote:
> This particular chunk of code is something long deprecated, so even if 
> the "correct" thing is to use `path::absolute`, we shouldn't change it.
> 
> Instead (in a separate patch, and only if you want to), we should simply 
> `#ifndef __WINDOWS__` the entire conditional and the body of the if-statement.
> 
> Jeff Coffler wrote:
> I don't quite understand this, as the end result is identical (on 
> non-Windows platforms). The old line here, as you can see, is:
> 
> ```
>  if (strings::startsWith(value, "/")) {
> ```
> 
> This change calls `path::absolute`. That routine, for Linux, is doing the 
> identical thing:
> 
> ```
> #ifndef __WINDOWS__
>   return strings::startsWith(path, os::PATH_SEPARATOR);
> ```
> 
> It feels like changing the code here to `#ifndef __WINDOWS__` is adding 
> changes/complexity when the end result is exactly what you're asking for (no 
> behaviorial change).
> 
> Is your concern here that, for Linux, we may change what we believe an 
> "absolute path" to be? While possible, that strikes me as exceedingly 
> unlikely. For a VERY long time, the way to tell (on Linux) that a path is 
> absolute is to just check the first byte of the string.
> 
> Please clarify what your concern is here. I'd like to understand what 
> you're thinkin' here, thanks.

I just pushed a new review, this one was not addressed pending feedback from 
Joe.


> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 79-86 (patched)
> > 
> >
> > I think this got a bit out of date. We settled on `\...` being an 
> > absolute path, not just `\?...` (per below implementation).
> 
> Jeff Coffler wrote:
> I didn't have a test for this specifically. The above is accurate, 
> though. The 1-3 cases are for files on disk. Network shares are a special 
> case and, while technically an absolute path, isn't related to files on disk.
> 
> That said, the function does implement this properly. The comment isn't 
> out of date. The note was just to point out that we handled this as well.
> 
> Andrew Schwartzmeyer wrote:
> Sorry, I mean that your third bullet should read `3. "\..."` instead of 
> `3. "\?..."`.

The latest review clarifies that these paths are for files on Windows.


> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 92-108 (patched)
> > 
> >
> > I'm hoping Joe approves of this style; it's what I came up with while 
> > working with Jeff. It seems to be the most readable.
> > 
> > For what it's worth, we also tried `std::regex`, and profiled it. Even 
> > in release mode with explicit optimization, it was 70x slower.
> 
> Jeff Coffler wrote:
> Yeah. Before this, the code was essentially the same (a bunch of boolean 
> conditionals), but rather "ugly" (three or four lines to represent the 
> conditions with `||` and `&&`). At first I resisted Andy's suggestion, but 
> ultimately decided it was a lot more readable, so I adopted it.
> 
> Andy strongly recommended using a RegEx. While use of a RegEx allows for 
> a nice one-liner on Windows, it was problematic:
> 
> 1. Windows and Linux behaved differently. Indeed, Linux would fail given 
> a Regex that was perfectly valid and worked fine on Windows. Ultimately, this 
> didn't matter, though, since the Windows solution was Windows-specific.
> 
> 2. I've worked with RegEx engines before, and I knew that it wouldn't 
> perform. RegEx engines are great for a lot of cases, but this one (a few 
> hard-coded boolean comparisons) isn't one of those cases. And indeed, a RegEx 
> for this was > 70x slower, which is pretty dramatic. This code is ultimately 
> a few boolean conditions, although it's 15 lines of C++ code. It is nice and 
> readable, though, if you understand lambda expressions.
> 
> I liked this in the end, so this was what I submitted. But behind it was 
> performance analysis and all! :-)
> 
> Joseph Wu wrote:
> Hm... I'm not seeing why the lambda is necessary 

Re: Review Request 58673: Fix FlagsFileTest to check for absolute path properly on Windows.

[Jeff Coffler]

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

(Updated April 28, 2017, 9:43 p.m.)


Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li 
Li.


Changes
---

Incorporated all review feedback from prior reviews. Same tests run (see 
testing down), no differences in output (although PathTest.Absolute does run 
more tests internally).


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


Repository: mesos


Description
---

Added 3rdparty/stout/tests/path_tests.cpp tests to Windows platform,
but disabled tests that did not pass. Enabled absolute path tests,
adding tests that were appropriate for the Windows platform.

Note that, for Windows, absolute paths may not be valid. For example,
a path like "\\?\abc:file.txt" is not valid. In this case, the
path::absolute method is undefined; the expectation is that it is
called with valid paths (absolute or relative, but valid).


Diffs (updated)
-

  3rdparty/stout/include/stout/flags/parse.hpp 
65edd86372596c2107e9f29cf27301e025e6620e 
  3rdparty/stout/include/stout/path.hpp 
2d2088aadfa1ea82c59424242671c4fb655dede1 
  3rdparty/stout/tests/CMakeLists.txt 4bbe713f259e7858d423dcb33956d41e62a915eb 
  3rdparty/stout/tests/flags_tests.cpp e2681f8f68f6478d4c8a20c1e75ddb050d16b56d 
  3rdparty/stout/tests/path_tests.cpp 0490d93908566c46a10d91b05790e5a7f2f289bc 


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

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


Testing
---

Passes `make check` on the Linux platform.

On Windows, the FlagsFileTest.JSONFile now passes, along with new 
PathTest.Absolute tests that I added due to new path::absolute handling on the 
Windows platform.

PS C:\mesos> .\build\3rdparty\stout\tests\Debug\stout-tests.exe 
--gtest_filter="*FlagsFileTest*"
Note: Google Test filter = *FlagsFileTest*-
[==] Running 2 tests from 1 test case.
[--] Global test environment set-up.
[--] 2 tests from FlagsFileTest
[ RUN  ] FlagsFileTest.JSONFile
WARNING: Logging before InitGoogleLogging() is written to STDERR
W0425 10:05:20.959357 1060440 parse.hpp:97] Specifying an absolute filename to 
read a command line option out of without using 'file:// is deprecated and will 
be removed in a future release. Simply adding 'file://' to the beginning of the 
path should eliminate this warning.
[   OK ] FlagsFileTest.JSONFile (9 ms)
[ RUN  ] FlagsFileTest.FilePrefix
[   OK ] FlagsFileTest.FilePrefix (6 ms)
[--] 2 tests from FlagsFileTest (18 ms total)

[--] Global test environment tear-down
[==] 2 tests from 1 test case ran. (23 ms total)
[  PASSED  ] 2 tests.
PS C:\mesos> .\build\3rdparty\stout\tests\Debug\stout-tests.exe 
--gtest_filter="*PathTest*"
Note: Google Test filter = *PathTest*-
[==] Running 2 tests from 1 test case.
[--] Global test environment set-up.
[--] 2 tests from PathTest
[ RUN  ] PathTest.Absolute
[   OK ] PathTest.Absolute (1 ms)
[ RUN  ] PathTest.Comparison
[   OK ] PathTest.Comparison (0 ms)
[--] 2 tests from PathTest (2 ms total)

[--] Global test environment tear-down
[==] 2 tests from 1 test case ran. (6 ms total)
[  PASSED  ] 2 tests.

  YOU HAVE 4 DISABLED TESTS

PS C:\mesos>


Thanks,

Jeff Coffler

Re: Review Request 57534: Added and implemented RegisterAgent ACL.

[Jiang Yan Xu]

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

(Updated April 28, 2017, 2:40 p.m.)


Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, and Greg Mann.


Changes
---

Rebased. NNFR.


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


Repository: mesos


Description
---

Added and implemented RegisterAgent ACL.


Diffs (updated)
-

  include/mesos/authorizer/acls.proto 796ebb7882af89bb58346444e6c718035c01d647 
  include/mesos/authorizer/authorizer.proto 
3ae5df5c276fc48b21e131ca9e0944315fe632e3 
  src/authorizer/local/authorizer.cpp 0e323f3eeb81a27c1737882162d3df29c9f00b2e 
  src/tests/authorization_tests.cpp b59623f30d152a634d388a0c15108b6d28a24305 


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

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 58263: Inherited working dir from parent when launching a DEBUG container.

[Vinod Kone]

> On April 28, 2017, 8:49 p.m., Vinod Kone wrote:
> > Nice test.

Modulo Jie's comment.


- Vinod


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


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58263/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7433
> https://issues.apache.org/jira/browse/MESOS-7433
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> c30b1fc659ee9b3cd343899638ced6408d8b51a2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58263/diff/4/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58821/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58263: Inherited working dir from parent when launching a DEBUG container.

[Vinod Kone]

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


Fix it, then Ship it!




Nice test.


src/slave/containerizer/mesos/containerizer.cpp
Lines 1484 (patched)


Curious if we want to allow isolators to override this behavior for debug 
containers just like non-debug containers.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 547 (patched)


s/, also/even/


- Vinod Kone


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58263/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7433
> https://issues.apache.org/jira/browse/MESOS-7433
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> c30b1fc659ee9b3cd343899638ced6408d8b51a2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58263/diff/4/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58821/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58758: Added secret to Volume protobuf.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On April 28, 2017, 11:49 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58758/
> ---
> 
> (Updated April 28, 2017, 11:49 a.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-7418
> https://issues.apache.org/jira/browse/MESOS-7418
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The volume/secret isolator will use a secret-fetcher module (third-party or 
> internal) to download the secret and makes it available at container_path.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 46bb7813cc402f2a71879d26d4bbc62abd852d52 
>   include/mesos/v1/mesos.proto f7c05a82f8265aedc0bd8fd20dd30e21af46e775 
> 
> 
> Diff: https://reviews.apache.org/r/58758/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 58820: Explained how container working directory differs from its sandbox.

[Vinod Kone]

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




include/mesos/slave/containerizer.proto
Line 186 (original), 186 (patched)


s/sandbox/sandbox in the host (or is it container?) mount namespace/ ?

also, not clear what "Mesos sandbox" means? do you mean the value of 
'MESOS_SANDBOX' env variable?



include/mesos/slave/containerizer.proto
Lines 187 (patched)


s/the image/an image/


- Vinod Kone


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58820/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> c30b1fc659ee9b3cd343899638ced6408d8b51a2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
> 
> 
> Diff: https://reviews.apache.org/r/58820/diff/2/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58819: Clarified the comment about Container.directory.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58819/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
> 
> 
> Diff: https://reviews.apache.org/r/58819/diff/2/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58818: Ensured DEBUG container shares MESOS_SANDBOX with its parent.

[Vinod Kone]

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


Ship it!




Nice test.

- Vinod Kone


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58818/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7355
> https://issues.apache.org/jira/browse/MESOS-7355
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58818/diff/2/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS and Fedora 24
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58817: Captured AgentID from the offer by reference in tests.

[Vinod Kone]

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


Ship it!




Thanks for fixing this!

- Vinod Kone


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58817/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp ec0d5ee78a94991c68f38174d09b41e8f2e4be35 
>   src/tests/slave_tests.cpp 8c97dc6d088708d301dc3ccf90d413fd785b782f 
> 
> 
> Diff: https://reviews.apache.org/r/58817/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Vinod Kone]

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


Fix it, then Ship it!




LGTM modulo Jie's comments.


src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 244 (patched)


s/, also/even/



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 301 (patched)


s/access/accesses/ ?


- Vinod Kone


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/6/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58487: Fix allocation quantities when shared resources are removed.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58485, 58486, 58487]

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 April 28, 2017, 3:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58487/
> ---
> 
> (Updated April 28, 2017, 3:42 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7308
> https://issues.apache.org/jira/browse/MESOS-7308
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When shared resources are removed from the `allocations` in the sorter,
> we remove the scalar quantity only when the shared resource is actually
> removed (i.e. shared count goes down to 0). Similarly, we increase the
> scalar quantity only when this is a new shared resource that is being
> added to the `allocations`.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> fee58d6d1f08163e2a06a4a20c891fe535c3dcff 
> 
> 
> Diff: https://reviews.apache.org/r/58487/diff/3/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58847: Checkpointed and recovered ContainerLaunchInfo for non-orphans.

[Vinod Kone]

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


Ship it!




LGTM modulo Jie's comments.

- Vinod Kone


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58847/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In mesos containerizer, cache each container's launch info and
> recover it for non-orphan containers. This is a prerequisite for
> persisting some container characteristics across agent failover.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/slave/containerizer/mesos/paths.hpp 
> d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
>   src/slave/containerizer/mesos/paths.cpp 
> ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
> 
> 
> Diff: https://reviews.apache.org/r/58847/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 58763: Added volume/secret isolator.

[Kapil Arya]

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

Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description
---

This is still Work in Progress.


Diffs
-

  src/Makefile.am 29da17bee13226e18757e2ad3a7a091427fd35f4 
  src/slave/containerizer/containerizer.hpp 
4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41 
  src/slave/containerizer/containerizer.cpp 
9024371b6c4228f0903cfeef3bbec736e1a425f8 
  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
56db7eb1193c9812b62b9149c9c2b2dd9b66701c 
  src/slave/containerizer/mesos/isolators/volume/secret.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/volume/secret.cpp PRE-CREATION 
  src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d 
  src/tests/cluster.cpp a4f57e0e9c4f221c42bdbb36d8925f688b3d742f 
  src/tests/mock_slave.hpp 767ed3d0444258f30b7338981ccc0080166ac5a7 
  src/tests/mock_slave.cpp c435ec76ac44775bf8a87421640649495c16187e 


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


Testing
---


Thanks,

Kapil Arya

Review Request 58762: Use SecretFetcher to fetch secret executor environment.

[Kapil Arya]

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

Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description
---

Secrets specified in the executor environment variables are resolved using a 
secret-fetcher module. If no secret-fetcher module is specified, an internal 
default implementation is used to resolve the secrets.


Diffs
-

  src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
  src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4 
  src/tests/hook_tests.cpp 02d8f800c3eb9b1e617a14c78c2ef1e45d1c72bb 


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


Testing
---


Thanks,

Kapil Arya

Re: Review Request 58761: Added --secret-fetcher flag for agent.

[Kapil Arya]

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

(Updated April 28, 2017, 2:51 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

The flag accepts a module name. The module should be specified via `--modules` 
or `--modules_dir` flag.


Diffs (updated)
-

  src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3 
  src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94 
  src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173 
  src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52 
  src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
  src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 


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

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


Testing
---


Thanks,

Kapil Arya

Re: Review Request 58760: Added default secret fetcher module.

[Kapil Arya]

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

(Updated April 28, 2017, 2:50 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

The default module doesn't attempt to interpret Secret::reference and simply 
returns Secret::value.


Diffs (updated)
-

  src/Makefile.am 29da17bee13226e18757e2ad3a7a091427fd35f4 
  src/secret/fetcher.hpp PRE-CREATION 
  src/secret/fetcher.cpp PRE-CREATION 


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

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


Testing
---


Thanks,

Kapil Arya

Re: Review Request 58759: Introduced SecretFetcher module interface.

[Kapil Arya]

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

(Updated April 28, 2017, 2:50 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description
---

Introduced SecretFetcher module interface.


Diffs (updated)
-

  include/mesos/module/secretfetcher.hpp PRE-CREATION 
  include/mesos/secret/secretfetcher.hpp PRE-CREATION 
  src/Makefile.am 29da17bee13226e18757e2ad3a7a091427fd35f4 
  src/module/manager.cpp 7d875fcb7fcec0d57274e644b0a3b67b333ac193 


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

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


Testing
---


Thanks,

Kapil Arya

Re: Review Request 58758: Added secret to Volume protobuf.

[Kapil Arya]

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

(Updated April 28, 2017, 2:49 p.m.)


Review request for mesos and Gilbert Song.


Summary (updated)
-

Added secret to Volume protobuf.


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


Repository: mesos


Description (updated)
---

The volume/secret isolator will use a secret-fetcher module (third-party or 
internal) to download the secret and makes it available at container_path.


Diffs (updated)
-

  include/mesos/mesos.proto 46bb7813cc402f2a71879d26d4bbc62abd852d52 
  include/mesos/v1/mesos.proto f7c05a82f8265aedc0bd8fd20dd30e21af46e775 


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

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


Testing
---


Thanks,

Kapil Arya

Re: Review Request 58673: Fix FlagsFileTest to check for absolute path properly on Windows.

[Andrew Schwartzmeyer]

> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 92-108 (patched)
> > 
> >
> > I'm hoping Joe approves of this style; it's what I came up with while 
> > working with Jeff. It seems to be the most readable.
> > 
> > For what it's worth, we also tried `std::regex`, and profiled it. Even 
> > in release mode with explicit optimization, it was 70x slower.
> 
> Jeff Coffler wrote:
> Yeah. Before this, the code was essentially the same (a bunch of boolean 
> conditionals), but rather "ugly" (three or four lines to represent the 
> conditions with `||` and `&&`). At first I resisted Andy's suggestion, but 
> ultimately decided it was a lot more readable, so I adopted it.
> 
> Andy strongly recommended using a RegEx. While use of a RegEx allows for 
> a nice one-liner on Windows, it was problematic:
> 
> 1. Windows and Linux behaved differently. Indeed, Linux would fail given 
> a Regex that was perfectly valid and worked fine on Windows. Ultimately, this 
> didn't matter, though, since the Windows solution was Windows-specific.
> 
> 2. I've worked with RegEx engines before, and I knew that it wouldn't 
> perform. RegEx engines are great for a lot of cases, but this one (a few 
> hard-coded boolean comparisons) isn't one of those cases. And indeed, a RegEx 
> for this was > 70x slower, which is pretty dramatic. This code is ultimately 
> a few boolean conditions, although it's 15 lines of C++ code. It is nice and 
> readable, though, if you understand lambda expressions.
> 
> I liked this in the end, so this was what I submitted. But behind it was 
> performance analysis and all! :-)
> 
> Joseph Wu wrote:
> Hm... I'm not seeing why the lambda is necessary here...
> 
> Don't we get the same result by putting all four boolean conditions in 
> sequence?
> 
> ```
> if (strings::startsWith(path, "\\")) {
>   return true;
> }
> 
> if (path.length() < 3) {
>   return false;
> }
> 
> std::string letter = path.substr(0, 1);
> if (!((letter >= "A" && letter <= "Z") || 
>   (letter >= "a" && letter <= "z"))) {
>   return false;
> }
> 
> std::string colon = path.substr(1, 2);
> return colon == ":\" || colon == ":/";
> ```
> 
> Jeff Coffler wrote:
> We could do that, and that's plenty readable. The original code did the 
> same sort of thing, but in a more compressed way. I don't have the original, 
> but it was along the line of:
> 
> ```
> return strings::startsWith(path, "\\")
>   || ((path.length() >= 3 && (path.substr(1, 2) == ":\" || path.substr(1, 
> 2) == ":/")
>   && ((path.substr(0, 1) >= "A" && path.substr(0, 1) <= "Z")
>   || (path.substr(0, 1) >= "a" && path.substr(0, 1) <= "b")))
> ```
> 
> Something like that, anyway. I thought that the lambda was much better 
> than that. Your suggestion is fine, too, and is clear without the lambda. 
> I'll implement that and post another patch.

Heh, this is why more eyes are better. Go with Joe's, it's much better.


- Andrew


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


On April 25, 2017, 6:24 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58673/
> ---
> 
> (Updated April 25, 2017, 6:24 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-5937
> https://issues.apache.org/jira/browse/MESOS-5937
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 3rdparty/stout/tests/path_tests.cpp tests to Windows platform,
> but disabled tests that did not pass. Enabled absolute path tests,
> adding tests that were appropriate for the Windows platform.
> 
> Note that, for Windows, absolute paths may not be valid. For example,
> a path like "\\?\abc:file.txt" is not valid. In this case, the
> path::absolute method is undefined; the expectation is that it is
> called with valid paths (absolute or relative, but valid).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/flags/parse.hpp 
> 65edd86372596c2107e9f29cf27301e025e6620e 
>   3rdparty/stout/include/stout/path.hpp 
> 2d2088aadfa1ea82c59424242671c4fb655dede1 
>   3rdparty/stout/tests/CMakeLists.txt 
> 4bbe713f259e7858d423dcb33956d41e62a915eb 
>   3rdparty/stout/tests/flags_tests.cpp 
> e2681f8f68f6478d4c8a20c1e75ddb050d16b56d 
>   3rdparty/stout/tests/path_tests.cpp 
> 0490d93908566c46a10d91b05790e5a7f2f289bc 
> 
> 
> Diff: 

Re: Review Request 58673: Fix FlagsFileTest to check for absolute path properly on Windows.

[Jeff Coffler]

> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/flags/parse.hpp
> > Line 96 (original), 96 (patched)
> > 
> >
> > There are more instances of erroneous absolute path checking through 
> > the code base, some of which we should probably fix.
> > 
> > That said, this change should probably be in a separate commit/patch to 
> > address the particular bug, followed by more patches for the other 
> > instances (which don't need to come immediately). But I don't think I'd 
> > include this directly with the porting of `path::absolute`.
> 
> Joseph Wu wrote:
> This particular chunk of code is something long deprecated, so even if 
> the "correct" thing is to use `path::absolute`, we shouldn't change it.
> 
> Instead (in a separate patch, and only if you want to), we should simply 
> `#ifndef __WINDOWS__` the entire conditional and the body of the if-statement.

I don't quite understand this, as the end result is identical (on non-Windows 
platforms). The old line here, as you can see, is:

```
 if (strings::startsWith(value, "/")) {
```

This change calls `path::absolute`. That routine, for Linux, is doing the 
identical thing:

```
#ifndef __WINDOWS__
  return strings::startsWith(path, os::PATH_SEPARATOR);
```

It feels like changing the code here to `#ifndef __WINDOWS__` is adding 
changes/complexity when the end result is exactly what you're asking for (no 
behaviorial change).

Is your concern here that, for Linux, we may change what we believe an 
"absolute path" to be? While possible, that strikes me as exceedingly unlikely. 
For a VERY long time, the way to tell (on Linux) that a path is absolute is to 
just check the first byte of the string.

Please clarify what your concern is here. I'd like to understand what you're 
thinkin' here, thanks.


> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 92-108 (patched)
> > 
> >
> > I'm hoping Joe approves of this style; it's what I came up with while 
> > working with Jeff. It seems to be the most readable.
> > 
> > For what it's worth, we also tried `std::regex`, and profiled it. Even 
> > in release mode with explicit optimization, it was 70x slower.
> 
> Jeff Coffler wrote:
> Yeah. Before this, the code was essentially the same (a bunch of boolean 
> conditionals), but rather "ugly" (three or four lines to represent the 
> conditions with `||` and `&&`). At first I resisted Andy's suggestion, but 
> ultimately decided it was a lot more readable, so I adopted it.
> 
> Andy strongly recommended using a RegEx. While use of a RegEx allows for 
> a nice one-liner on Windows, it was problematic:
> 
> 1. Windows and Linux behaved differently. Indeed, Linux would fail given 
> a Regex that was perfectly valid and worked fine on Windows. Ultimately, this 
> didn't matter, though, since the Windows solution was Windows-specific.
> 
> 2. I've worked with RegEx engines before, and I knew that it wouldn't 
> perform. RegEx engines are great for a lot of cases, but this one (a few 
> hard-coded boolean comparisons) isn't one of those cases. And indeed, a RegEx 
> for this was > 70x slower, which is pretty dramatic. This code is ultimately 
> a few boolean conditions, although it's 15 lines of C++ code. It is nice and 
> readable, though, if you understand lambda expressions.
> 
> I liked this in the end, so this was what I submitted. But behind it was 
> performance analysis and all! :-)
> 
> Joseph Wu wrote:
> Hm... I'm not seeing why the lambda is necessary here...
> 
> Don't we get the same result by putting all four boolean conditions in 
> sequence?
> 
> ```
> if (strings::startsWith(path, "\\")) {
>   return true;
> }
> 
> if (path.length() < 3) {
>   return false;
> }
> 
> std::string letter = path.substr(0, 1);
> if (!((letter >= "A" && letter <= "Z") || 
>   (letter >= "a" && letter <= "z"))) {
>   return false;
> }
> 
> std::string colon = path.substr(1, 2);
> return colon == ":\" || colon == ":/";
> ```

We could do that, and that's plenty readable. The original code did the same 
sort of thing, but in a more compressed way. I don't have the original, but it 
was along the line of:

```
return strings::startsWith(path, "\\")
  || ((path.length() >= 3 && (path.substr(1, 2) == ":\" || path.substr(1, 2) == 
":/")
  && ((path.substr(0, 1) >= "A" && path.substr(0, 1) <= "Z")
  || (path.substr(0, 1) >= "a" && path.substr(0, 1) <= "b")))
```

Something like that, anyway. I thought that the lambda was much better than 
that. Your suggestion is fine, too, and is clear without the lambda. I'll 
implement that and post another patch.


> On 

Re: Review Request 58766: Fixed a path join issue in ProvisionerDockerBackendTest.

[Jie Yu]

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




src/tests/containerizer/provisioner_docker_tests.cpp
Line 812 (original), 812 (patched)


You can use os::write(hostFile, "abc") here


- Jie Yu


On April 26, 2017, 11:32 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58766/
> ---
> 
> (Updated April 26, 2017, 11:32 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced a string concatenation with path::join in
> ProvisionerDockerBackendTest.ROOT_INTERNET_CURL_Overwrite.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 52e57cef81f79ebae4d5305708929396c5d75680 
> 
> 
> Diff: https://reviews.apache.org/r/58766/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58824: Disabled unit test relying on AliCloud Registry.

[Jie Yu]

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




src/tests/containerizer/provisioner_docker_tests.cpp
Line 464 (original)


Let's comment it out, instead of removing it. Ideally, if we can control 
this by a testing flag, that'll be even better.


- Jie Yu


On April 27, 2017, 11:16 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58824/
> ---
> 
> (Updated April 27, 2017, 11:16 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Disabled unit test relying on AliCloud Registry.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 4341621767a9fa5be2c66e77ef60f0c65dae58ca 
> 
> 
> Diff: https://reviews.apache.org/r/58824/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 58263: Inherited working dir from parent when launching a DEBUG container.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.hpp
Lines 338 (patched)


See my comments in the other review. Let's keep `launchInfo` here (similar 
to that we keep `config`)


- Jie Yu


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58263/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7433
> https://issues.apache.org/jira/browse/MESOS-7433
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> c30b1fc659ee9b3cd343899638ced6408d8b51a2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58263/diff/4/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58821/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.hpp
Lines 367 (patched)


I think we should store `Option` here so that we don't 
end up adding more and more field in Info struct.


- Jie Yu


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/6/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58821: Added a test that verifies a task and its check share the work dir.

[Alexander Rukletsov]

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




src/tests/check_tests.cpp
Lines 612-616 (patched)


This test appears to be flaky on some Ubuntu and CentOS machines (but nor 
Fedora 24). I will investigate and update accordingly.


- Alexander Rukletsov


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58821/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7433
> https://issues.apache.org/jira/browse/MESOS-7433
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp ec0d5ee78a94991c68f38174d09b41e8f2e4be35 
> 
> 
> Diff: https://reviews.apache.org/r/58821/diff/2/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS and Fedora 24
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58847: Checkpointed and recovered ContainerLaunchInfo for non-orphans.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/paths.hpp
Lines 47 (patched)


I'd remove CONTAINER prefix:
```
constexpr char LAUNCH_INFO_FILE[] = "launch_info";
```


- Jie Yu


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58847/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In mesos containerizer, cache each container's launch info and
> recover it for non-orphan containers. This is a prerequisite for
> persisting some container characteristics across agent failover.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/slave/containerizer/mesos/paths.hpp 
> d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
>   src/slave/containerizer/mesos/paths.cpp 
> ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
> 
> 
> Diff: https://reviews.apache.org/r/58847/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

> On April 27, 2017, 11:15 p.m., Jie Yu wrote:
> >
> 
> Jie Yu wrote:
> See my comments in https://reviews.apache.org/r/58263/
> 
> We should probably just checkpoint `ContainerLaunchInfo`.

Yeah, this sounds like a good idea. Checkpoint the whole `ContainerLaunchInfo`, 
but restore only environment and working directory for now. I'll update the 
chain.


> On April 27, 2017, 11:15 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1468-1469 (patched)
> > 
> >
> > Should we return failure here? If we launch a debug container later 
> > under this container, will it silently miss some environment variables?

I think it is not fatal if it's just environment, however, this is 
questionable. In any way, since we now will be checkpointing the whole 
`ContainerLaunchInfo`, I think it makes sense to propagate the checkpointing 
failure if any. Dropping this since the code will be migrated.


- Alexander


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


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/6/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58821: Added a test that verifies a task and its check share the work dir.

[Alexander Rukletsov]

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

(Updated April 28, 2017, 4:14 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/check_tests.cpp ec0d5ee78a94991c68f38174d09b41e8f2e4be35 


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

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


Testing
---

`make check` on Mac OS and Fedora 24


Thanks,

Alexander Rukletsov

Re: Review Request 58820: Explained how container working directory differs from its sandbox.

[Alexander Rukletsov]

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

(Updated April 28, 2017, 4:14 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Addressed comments.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
c30b1fc659ee9b3cd343899638ced6408d8b51a2 
  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 


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

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 58263: Inherited working dir from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated April 28, 2017, 4:14 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


Changes
---

Recover via ContainerLaunchInfo.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
c30b1fc659ee9b3cd343899638ced6408d8b51a2 
  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
29007898ec04e922266068a8519731b13d351a82 


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

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


Testing
---

See https://reviews.apache.org/r/58821/


Thanks,

Alexander Rukletsov

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated April 28, 2017, 4:14 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
29007898ec04e922266068a8519731b13d351a82 


Diff: https://reviews.apache.org/r/58262/diff/6/

Changes: https://reviews.apache.org/r/58262/diff/5-6/


Testing
---

See https://reviews.apache.org/r/58718/


Thanks,

Alexander Rukletsov

Re: Review Request 58817: Captured AgentID from the offer by reference in tests.

[Alexander Rukletsov]

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

(Updated April 28, 2017, 4:14 p.m.)


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


Changes
---

Rebased.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/check_tests.cpp ec0d5ee78a94991c68f38174d09b41e8f2e4be35 
  src/tests/slave_tests.cpp 8c97dc6d088708d301dc3ccf90d413fd785b782f 


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

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 58819: Clarified the comment about Container.directory.

[Alexander Rukletsov]

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

(Updated April 28, 2017, 4:14 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Addressed comments.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 


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

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 58718: Added a test that verifies a task's env var is seen by its check.

[Alexander Rukletsov]

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

(Updated April 28, 2017, 4:14 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/check_tests.cpp ec0d5ee78a94991c68f38174d09b41e8f2e4be35 


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

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


Testing
---

`make check` on various Linux distributions.


Thanks,

Alexander Rukletsov

Re: Review Request 58818: Ensured DEBUG container shares MESOS_SANDBOX with its parent.

[Alexander Rukletsov]

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

(Updated April 28, 2017, 4:14 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
29007898ec04e922266068a8519731b13d351a82 


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

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


Testing
---

`make check` on Mac OS and Fedora 24


Thanks,

Alexander Rukletsov

Review Request 58847: Checkpointed and recovered ContainerLaunchInfo for non-orphans.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.


Repository: mesos


Description
---

In mesos containerizer, cache each container's launch info and
recover it for non-orphan containers. This is a prerequisite for
persisting some container characteristics across agent failover.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/slave/containerizer/mesos/paths.hpp 
d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
  src/slave/containerizer/mesos/paths.cpp 
ed4bbd2491e71ad1e4a41e0575b514377d02da9b 


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


Testing
---

See https://reviews.apache.org/r/58718/


Thanks,

Alexander Rukletsov

Re: Review Request 58487: Fix allocation quantities when shared resources are removed.

[Anindya Sinha]

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

(Updated April 28, 2017, 3:42 p.m.)


Review request for mesos, James Peach and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

When shared resources are removed from the `allocations` in the sorter,
we remove the scalar quantity only when the shared resource is actually
removed (i.e. shared count goes down to 0). Similarly, we increase the
scalar quantity only when this is a new shared resource that is being
added to the `allocations`.


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.hpp 
fee58d6d1f08163e2a06a4a20c891fe535c3dcff 


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

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 58486: Fixed a race in `updateAllocation()` on DESTORY of a shared volume.

[Anindya Sinha]

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

(Updated April 28, 2017, 3:42 p.m.)


Review request for mesos, James Peach and Jiang Yan Xu.


Changes
---

Rebased. Yet to address the review comments.


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


Repository: mesos


Description
---

In `updateAllocation()`, it is possible for the flattened scalar
quantites of original framework allocation to differ from the updated
framework allocation. This can happen when an offer with a shared
persistent volume is sent out after a DESTROY has been received. If
that is the case, we rescind the pending offers from all frameworks
which contain the volumes to be destroyed.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
6eda1b8619269c1501a935045b18b1deaf845b33 
  src/master/allocator/mesos/allocator.hpp 
57b54b86c43c7731e64d422d285c4b8ca7e27a60 
  src/master/allocator/mesos/hierarchical.hpp 
219f508dacb3b372c12da3f15e07a3bf5f27e6e6 
  src/master/allocator/mesos/hierarchical.cpp 
84dc31dd6e567e9316a73c61e03a1daf6f556829 
  src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
  src/master/master.cpp e8c2a96ff3407fb429e60cd9e66a8c4dc52b391b 
  src/tests/allocator.hpp 6b71c574e0e4facd1795ef50ee0869c03b87833d 
  src/tests/hierarchical_allocator_tests.cpp 
84bb6f302f6ff6f8a93fa891795df9ef7380629f 


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

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 58485: Avoid a corruption while rescinding offers.

[Anindya Sinha]

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

(Updated April 28, 2017, 3:42 p.m.)


Review request for mesos, James Peach and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

When a `DESTROY` is processed, we rescind all pending offers to which
persistent volumes have been offered. As soon as we rescind an offer,
the `Offer` object is freed; and hence, we should move on to the next
offer (even though there are multiple volumes being destroyed in
the same `ACCEPT` call).


Diffs (updated)
-

  src/master/master.cpp e8c2a96ff3407fb429e60cd9e66a8c4dc52b391b 
  src/tests/persistent_volume_tests.cpp 
3eb7afe3de8e72ffb6502dfe12ef37ccd4ca2125 


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

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57652: Allow authenticators to return any http Response.

[Alexander Rojas]

> On April 11, 2017, 10:17 a.m., Adam B wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp
> > Lines 94-95 (original), 94-95 (patched)
> > 
> >
> > I don't think either of these fields should be used to return a 
> > redirect. If we want to allow an authenticator module to return a 
> > (temporary) redirect, then we should a) add a new redirect field to the 
> > struct, and b) teach any users of the http authenticator (i.e. master, 
> > agent, etc.) to handle/log such a result and return the appropriate 
> > response.

I completely agree with adam's view. We should probably add an 
`Option redirect` field instead of making the existing ones 
`Option`. Note that this was done on purpose so authenticators just 
have a limited set of actions it can take, therefore not being allowed to do 
anything but authorization. Moreover, transforming the fields to 
`Option` can allow very weird things to be done like `unauthorized` 
suddenly containing an `OK` message.


- Alexander


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


On April 5, 2017, 9:13 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57652/
> ---
> 
> (Updated April 5, 2017, 9:13 p.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Alexander Rojas, and Greg 
> Mann.
> 
> 
> Bugs: MESOS-7247
> https://issues.apache.org/jira/browse/MESOS-7247
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously only allowed Unauthorized/Forbidden.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> 272d92117547512328c09dfc04c6ca4bf7b6b937 
> 
> 
> Diff: https://reviews.apache.org/r/57652/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 58830: Updated Suppress / Revive calls to take multiple roles.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58828, 58829, 58830]

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 April 28, 2017, 12:48 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58830/
> ---
> 
> (Updated April 28, 2017, 12:48 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 6eda1b8619269c1501a935045b18b1deaf845b33 
>   include/mesos/scheduler/scheduler.proto 
> cee0a175fcdcbb8b56d41a5b4b6fd965afd822bd 
>   include/mesos/v1/scheduler/scheduler.proto 
> 00a54eceb435de8e8fea3ccab285f39ed13c6fee 
>   src/master/allocator/mesos/allocator.hpp 
> 57b54b86c43c7731e64d422d285c4b8ca7e27a60 
>   src/master/allocator/mesos/hierarchical.hpp 
> 219f508dacb3b372c12da3f15e07a3bf5f27e6e6 
>   src/master/allocator/mesos/hierarchical.cpp 
> 984a0a4e2671ee7bb2a3515849342f49f2c4e3aa 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
>   src/master/master.cpp a0cb505a1300637f42fa8062c57deb1590779dd4 
>   src/messages/messages.proto 6cc06eb0f30cddac0a15c777dcd7f0db2ff6 
>   src/tests/allocator.hpp 6b71c574e0e4facd1795ef50ee0869c03b87833d 
>   src/tests/hierarchical_allocator_tests.cpp 
> 84bb6f302f6ff6f8a93fa891795df9ef7380629f 
> 
> 
> Diff: https://reviews.apache.org/r/58830/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 58754: Altered the task command used in an agent test.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58754]

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 April 28, 2017, 2:13 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58754/
> ---
> 
> (Updated April 28, 2017, 2:13 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the test `SlaveTest.RestartSlaveRequireExecutorAuthentication`
> used the command 'cat' in an attempt to run a long-lived task. However,
> this command seems to yield a task that will terminate prematurely in some
> testing environments. This patch updates the task to use `sleep 120` instead.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 8c97dc6d088708d301dc3ccf90d413fd785b782f 
> 
> 
> Diff: https://reviews.apache.org/r/58754/diff/1/
> 
> 
> Testing
> ---
> 
> Run in CI to verify that the test is no longer flaky.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 58357: Support more test frameworks in test-upgrade script.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58357]

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 April 27, 2017, 5:58 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58357/
> ---
> 
> (Updated April 27, 2017, 5:58 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Greg Mann.
> 
> 
> Bugs: MESOS-7228
> https://issues.apache.org/jira/browse/MESOS-7228
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch added support to java and python based test framework in
> `test-upgrade.py` script.
> 
> 
> Diffs
> -
> 
>   support/test-upgrade.py 84df21dd6e63653e1a18e700ef904aa3a04b2b45 
> 
> 
> Diff: https://reviews.apache.org/r/58357/diff/4/
> 
> 
> Testing
> ---
> 
> Ran this on all three languages options for cpp, java and python.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 58830: Updated Suppress / Revive calls to take multiple roles.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On April 27, 2017, 5:48 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58830/
> ---
> 
> (Updated April 27, 2017, 5:48 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 6eda1b8619269c1501a935045b18b1deaf845b33 
>   include/mesos/scheduler/scheduler.proto 
> cee0a175fcdcbb8b56d41a5b4b6fd965afd822bd 
>   include/mesos/v1/scheduler/scheduler.proto 
> 00a54eceb435de8e8fea3ccab285f39ed13c6fee 
>   src/master/allocator/mesos/allocator.hpp 
> 57b54b86c43c7731e64d422d285c4b8ca7e27a60 
>   src/master/allocator/mesos/hierarchical.hpp 
> 219f508dacb3b372c12da3f15e07a3bf5f27e6e6 
>   src/master/allocator/mesos/hierarchical.cpp 
> 984a0a4e2671ee7bb2a3515849342f49f2c4e3aa 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
>   src/master/master.cpp a0cb505a1300637f42fa8062c57deb1590779dd4 
>   src/messages/messages.proto 6cc06eb0f30cddac0a15c777dcd7f0db2ff6 
>   src/tests/allocator.hpp 6b71c574e0e4facd1795ef50ee0869c03b87833d 
>   src/tests/hierarchical_allocator_tests.cpp 
> 84bb6f302f6ff6f8a93fa891795df9ef7380629f 
> 
> 
> Diff: https://reviews.apache.org/r/58830/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 58828: Replaced std::set with hashset for framework roles.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On April 27, 2017, 5:43 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58828/
> ---
> 
> (Updated April 27, 2017, 5:43 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since we do not need ordering, this updates the framework roles to
> be stored using hashset instead of std::set.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 
>   src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
>   src/master/allocator/mesos/hierarchical.hpp 
> 219f508dacb3b372c12da3f15e07a3bf5f27e6e6 
>   src/master/allocator/mesos/hierarchical.cpp 
> 984a0a4e2671ee7bb2a3515849342f49f2c4e3aa 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
>   src/master/master.cpp a0cb505a1300637f42fa8062c57deb1590779dd4 
>   src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 
>   src/tests/protobuf_utils_tests.cpp 5239182812835b93a28e85146b2df2b20ae77328 
> 
> 
> Diff: https://reviews.apache.org/r/58828/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 58829: Fixed the implementation of per-role Suppress / Revive calls.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On April 27, 2017, 5:44 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58829/
> ---
> 
> (Updated April 27, 2017, 5:44 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7430
> https://issues.apache.org/jira/browse/MESOS-7430
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The existing implementation uses a single suppression boolean which
> results in incorrect handling of per-role suppression / revival.
> 
> See: https://issues.apache.org/jira/browse/MESOS-7430
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 219f508dacb3b372c12da3f15e07a3bf5f27e6e6 
>   src/master/allocator/mesos/hierarchical.cpp 
> 984a0a4e2671ee7bb2a3515849342f49f2c4e3aa 
> 
> 
> Diff: https://reviews.apache.org/r/58829/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 58828: Replaced std::set with hashset for framework roles.

[Michael Park]

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



It's true that we don't use `std::set` for its ordering in the general case,
however, I think the ordering does help produce easier-to-read error messages.


What do you think of preserving that property?

- Michael Park


On April 27, 2017, 5:43 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58828/
> ---
> 
> (Updated April 27, 2017, 5:43 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since we do not need ordering, this updates the framework roles to
> be stored using hashset instead of std::set.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 
>   src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
>   src/master/allocator/mesos/hierarchical.hpp 
> 219f508dacb3b372c12da3f15e07a3bf5f27e6e6 
>   src/master/allocator/mesos/hierarchical.cpp 
> 984a0a4e2671ee7bb2a3515849342f49f2c4e3aa 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
>   src/master/master.cpp a0cb505a1300637f42fa8062c57deb1590779dd4 
>   src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 
>   src/tests/protobuf_utils_tests.cpp 5239182812835b93a28e85146b2df2b20ae77328 
> 
> 
> Diff: https://reviews.apache.org/r/58828/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>