Re: Review Request 52877: Fixed wrong float serialization in JSON in locales different from C.

[Joris Van Remoortere]

> On Oct. 14, 2016, 11:23 p.m., Joris Van Remoortere wrote:
> > I have a feeling that if you kept around a stringstream with the local set 
> > your benchmarks would look rather different.
> > I also suggest using callgrind to get the instruction count / # of library 
> > calls made.
> 
> Alexander Rojas wrote:
> That occurred to me, but then some issues appeared with the idea. The 
> version in `json.hpp` uses a free function, the way to keep the 
> `stringstream` is by making it global (either having a global variable or 
> marking it `static`), however that makes the function non thread safe and a 
> mutex will be needed. It probably will still perform better in a single 
> thread situation though.
> 
> The `jsonify.hpp` version has more or less the same concerns. I though 
> moving the `stringstream` to the constructor of the `Number` object, which 
> would fix the thread safety issue, but at the end one is just moving the 
> object construction penalty from serialization to construction time. But 
> given the usage of `jsonify`, it wouldn't have any performance issue.
> 
> Do you have any other idea on how to keep the `stringstream` alive?
> 
> Joris Van Remoortere wrote:
> Thread Local?
> 
> Alexander Rojas wrote:
> From the `stout/thread_local.hpp`
> 
> ```c++
> // We required that THREAD_LOCAL is only used with POD types as this
> // is a requirement of `__thread`.
> ```
> 
> `std::stringstream` is definitely not a POD.

`std::stringstream*` is


- Joris


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


On Oct. 14, 2016, 12:50 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52877/
> ---
> 
> (Updated Oct. 14, 2016, 12:50 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Michael Park.
> 
> 
> Bugs: MESOS-6349
> https://issues.apache.org/jira/browse/MESOS-6349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resolves an isse with the JSON serialization functions where floats
> would use the process active locale as a decimal separator instead
> of the JSON conformant period (`.`).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/json.hpp 
> 9734561549eea867d57f14a0ab71febeb9dddc06 
>   3rdparty/stout/include/stout/jsonify.hpp 
> 8220d001001e8b9f247c05be58534f1174611aad 
> 
> Diff: https://reviews.apache.org/r/52877/diff/
> 
> 
> Testing
> ---
> 
> Three types of tests were used for this issue. The first just run our current
> test suite (`make check`). The second verifies that the proposed solutions
> are able to generate a locale independent output and the third consist of a
> benchmark of all the solutions.
> 
> The verification that the proposed solutions produce locale independent JSON,
> the following program was used (with manual verification of the generated
> output):
> 
> ```c++
> #include 
> 
> #include 
> #include 
> #include 
> 
> int main(int argc, char **argv) {
>   // Set locale to German.
>   std::setlocale(LC_ALL, "de_DE.UTF-8");
>   std::cout.imbue(std::locale("de_DE.UTF-8"));
> 
>   JSON::Object object;
>   object.values["float"] = 1234567890.12345;
> 
>   std::cout << stringify(object) << '\n';
>   return 0;
> }
> 
> ```
> 
> In order to test the different options the following benchmarking program was
> written using [Google benchmark](https://github.com/google/benchmark):
> 
> ```c++
> #include 
> #include 
> 
> #include 
> #include 
> #include 
> #include 
> 
> static void BM_Jsonify(benchmark::State& state) {
>   while (state.KeepRunning()) {
> state.PauseTiming();
> JSON::Object object;
> object.values["float00"] = 1234567890.12345;
> object.values["float01"] = 123456789.012345;
> object.values["float02"] = 12345678.9012345;
> object.values["float03"] = 1234567.89012345;
> object.values["float04"] = 123456.789012345;
> object.values["float05"] = 12345.6789012345;
> object.values["float06"] = 1234.56789012345;
> object.values["float07"] = 123.456789012345;
> object.values["float08"] = 12.3456789012345;
> object.values["float09"] = 1.23456789012345;
> object.values["float10"] = 1234567890.12345;
> object.values["float11"] = 123456789.012345;
> object.values["float12"] = 12345678.9012345;
> object.values["float13"] = 1234567.89012345;
> object.values["float14"] = 123456.789012345;
> object.values["float15"] = 12345.6789012345;
> object.values["float16"] = 1234.56789012345;
> object.values["float17"] = 123.456789012345;
> object.values["float18"] = 12.3456789012345;
> object.values["float19"] = 1.23456789012345;
>

Re: Review Request 53275: Added a test for duplicate frameworks in "unregistered_frameworks".

[Vinod Kone]

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

(Updated Oct. 28, 2016, 11:19 p.m.)


Review request for mesos, Anand Mazumdar and Jiang Yan Xu.


Changes
---

anand's comments. NNFR.


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


Repository: mesos


Description
---

Added a test for duplicate frameworks in "unregistered_frameworks".


Diffs (updated)
-

  src/tests/master_tests.cpp 968192c414c9a5db9b2dd038bc83677230bf467c 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 53275: Added a test for duplicate frameworks in "unregistered_frameworks".

[Anand Mazumdar]

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


Fix it, then Ship it!





src/tests/master_tests.cpp (line 2941)


s/This to/This is used to



src/tests/master_tests.cpp (line 2945)


Kill this. It's self explanatory.



src/tests/master_tests.cpp (line 2984)


Remove this. I thought we stopped doing `Times(1)` since it's the default?



src/tests/master_tests.cpp (line 3020)


Ditto as above.



src/tests/master_tests.cpp (lines 3048 - 3050)


Kill this and also the `pause()` on L3041. `StartMaster()` should already 
be doing a `settle()` ensuring `_recover()` is executed.



src/tests/master_tests.cpp (lines 3058 - 3083)


You might want to scope this for readability:

```cpp
// Ensure that there are 2 orphan tasks and 1 unregistered framework
  // in "/state" endpoint.
{
  Future ... = ...;
}

// Ensure that there is 1 unregistered framework in "/frameworks" endpoint.
{
  Future ... = ...;
}
```



src/tests/master_tests.cpp (line 3111)


Kill this.


- Anand Mazumdar


On Oct. 28, 2016, 10:30 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53275/
> ---
> 
> (Updated Oct. 28, 2016, 10:30 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6501
> https://issues.apache.org/jira/browse/MESOS-6501
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for duplicate frameworks in "unregistered_frameworks".
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 968192c414c9a5db9b2dd038bc83677230bf467c 
> 
> Diff: https://reviews.apache.org/r/53275/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 53005: Updated release guide.

[Vinod Kone]

> On Oct. 20, 2016, 1:34 p.m., Till Toenshoff wrote:
> > docs/release-guide.md, lines 213-214
> > 
> >
> > "Also, make sure to add the names of the release managers in 
> > "Description" section?"
> > 
> > What are we trying to achieve here?

Just a way to keep track of the release managers.


- Vinod


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


On Oct. 18, 2016, 10:47 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53005/
> ---
> 
> (Updated Oct. 18, 2016, 10:47 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added information about release branches among other things.
> 
> 
> Diffs
> -
> 
>   docs/release-guide.md c5a12c5a9f36e07334b59edf0788359b42a3125f 
> 
> Diff: https://reviews.apache.org/r/53005/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 53275: Added a test for duplicate frameworks in "unregistered_frameworks".

[Vinod Kone]

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

Review request for mesos, Anand Mazumdar and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added a test for duplicate frameworks in "unregistered_frameworks".


Diffs
-

  src/tests/master_tests.cpp 968192c414c9a5db9b2dd038bc83677230bf467c 

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


Testing
---

make check


Thanks,

Vinod Kone

Review Request 53264: Added test for CNI port-mapper plugin.

[Avinash sridharan]

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

Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

Added test for CNI port-mapper plugin.


Diffs
-

  src/tests/containerizer/cni_isolator_tests.cpp 
b032c4345683813bca5f9a5eec09f73d860299cc 

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


Testing
---

sudo make check


Thanks,

Avinash sridharan

Re: Review Request 53266: Made DefaultExecutorTests pass when running on hosts without Docker.

[Vinod Kone]

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



The diff is hard to review because in addition to making tests into fixture 
methods, there seem to be movement of tests around? Am I understanding it 
correct? If yes, please do any such moves in a different (dependent) review.

More importantly, can we just add ROOT_DOCKER prefix to all these tests 
instead? That way these tests will only be run on machines with docker which is 
a bit unfortunate but not too bad.

Alternatively, lets pick one test which we want to test with both the "mesos" 
and "docker,mesos" containerizers. For all other tests just test with mesos 
containerizer. That way most tests won't even need root privileges to be run 
and can be run in ASF CI.


src/tests/default_executor_tests.cpp (line 379)





- Vinod Kone


On Oct. 28, 2016, 6:27 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53266/
> ---
> 
> (Updated Oct. 28, 2016, 6:27 p.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6455
> https://issues.apache.org/jira/browse/MESOS-6455
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some of the default executor tests use a parameterized value to choose
> which containerizers to use. This results in tests that require Docker,
> but don't contain "DOCKER_" in their names.
> 
> This patch uses fixtures instead of parameterized values for this tests,
> in order to be able to specify a custom name for the ones that use the
> Docker containerizer.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 70779e8d94fbb6e664cad4ddbfeb19e560176cfe 
> 
> Diff: https://reviews.apache.org/r/53266/diff/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh --gtest_filter=DefaultExecutorTest.* 
> --docker=/tmp/foo`
> `sudo bin/mesos-tests.sh --gtest_filter=DefaultExecutorTest.*`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 53115: Implemented handling AUFS whiteouts for copy backend.

[Jie Yu]

> On Oct. 28, 2016, 2:03 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/backends/copy.cpp, line 213
> > 
> >
> > Hum, IIUC, looks like this is not entirely correct. According to this:
> > 
> > https://github.com/opencontainers/image-spec/blob/master/layer.md#whiteouts
> > 
> > "Files that are present in the same layer as a whiteout file can only 
> > be hidden by whiteout files in subsequent layers."
> > 
> > So we cannot cp first and then process whiteouts. Looks like before cp 
> > each layer, we need to do a pre-processing to handle whiteouts first (and 
> > remove the whiteouts), and then do the cp.
> 
> Qian Zhang wrote:
> Thanks for pointing this out! I think the flow should be:
> 1. Delete the related files in rootfs based on the whiteout files in the 
> layer.
> 2. Copy the layer to rootfs.
> 3. Once we have done 1 and 2 for all layers, traverse rootfs to remove 
> all the whiteout files.
> Or maybe in 2 we can do a seletively copy by `find` + `cp` to only copy 
> non-whiteout files from layer to rootfs? In this way, we do not need 3.

While you're doing 1, you can keep a list of paths to whiteout files as you 
walk the tree. You can erase them before you do 2. Will that work?


- Jie


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


On Oct. 25, 2016, 3:47 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53115/
> ---
> 
> (Updated Oct. 25, 2016, 3:47 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented handling AUFS whiteouts for copy backend.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/backend.hpp 
> 7257d3a962ecdf87fe9d52facbd6a2619311a018 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.hpp 
> a3c924195ae5eecc1caca9cd6fc0f6dc0df0a741 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.cpp 
> c92c6c7174421158b9190ed0bfb00c1c97ef0f7b 
>   src/slave/containerizer/mesos/provisioner/backends/bind.hpp 
> 54c88d940aa64d13114fc5d79ecbc1d474d169a6 
>   src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
> 94dac40a12b6fd2e7d9733444d84763c77785402 
>   src/slave/containerizer/mesos/provisioner/backends/copy.hpp 
> 131d75521ca38afae651e8d885ebedad77d86a3e 
>   src/slave/containerizer/mesos/provisioner/backends/copy.cpp 
> 9c5354e5fea488618ebdecf0aef9dd2fce555d20 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
> 4d591c5f988d87e0e905f973df5ab15a3386d676 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
> b71a31323aef376c9a28e1d52322a1802fb212ad 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> de2c12140652244bd3de9763ced203b144688ab2 
>   src/tests/containerizer/provisioner_backend_tests.cpp 
> 727bf645dd9337a94f098ab0a816b7e0e0651083 
> 
> Diff: https://reviews.apache.org/r/53115/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53115: Implemented handling AUFS whiteouts for copy backend.

[Jie Yu]

> On Oct. 28, 2016, 2:03 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/backends/copy.cpp, line 133
> > 
> >
> > I am not sure if we need to pass in 'image' to backend. If we added OCI 
> > support, do we need to add another check here?
> > 
> > I think we can simply assume the layer's whiteout is in aufs whiteout 
> > format, no matter what image type it is. I think APPC, we probably needs to 
> > do the same, right?
> > 
> > You probably add a NOTE there saying that we assume all image type uses 
> > aufs whiteout format.
> > 
> > In that way, no need to pass 'image' around to backend.
> 
> Qian Zhang wrote:
> The reason I passed in `image` and added this check is that previously we 
> have such check in `ProvisionerProcess::__provision()`:
> 
> https://github.com/apache/mesos/blob/1.0.1/src/slave/containerizer/mesos/provisioner/provisioner.cpp#L334
> 
> I think one of the purposes of this check is to ensure we will NOT go 
> through the logic to handle whiteout files for APPC. I think this is correct 
> because APPC seems have a totally different way for whiteout, in its image 
> manifest (https://github.com/appc/spec/blob/master/spec/aci.md ), there is a 
> field `pathWhitelist`:
> ```
> pathWhitelist (list of strings, optional): whitelist of absolute paths 
> that will exist in the app's rootfs after rendering. This must be a complete 
> and absolute set. An empty list is equivalent to an absent value and means 
> that all files in this image and any dependencies will be available in the 
> rootfs.
> ```
> And there is PR in APPC to translate Docker whiteout files to 
> `pathWhitelist`: https://github.com/appc/docker2aci/pull/36
> 
> So I think if we do not pass in `image`, then copy backend will try to 
> handle whiteout files for APPC image which is not necessary since there is no 
> whiteout files to handle, instead, we should support `pathWhitelist` which 
> seems missed in 
> https://github.com/apache/mesos/blob/1.0.1/include/mesos/appc/spec.proto , 
> that is another issue.

If you think about that in a different way: can we convert the APPC whiteout to 
be the same as OCI and Docker? I don't want each backend to have different ways 
to handle whiteouts depending on the type. At the end of the day, one needs to 
convert whiteout to aufs or overlayfs format, right?


- Jie


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


On Oct. 25, 2016, 3:47 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53115/
> ---
> 
> (Updated Oct. 25, 2016, 3:47 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented handling AUFS whiteouts for copy backend.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/backend.hpp 
> 7257d3a962ecdf87fe9d52facbd6a2619311a018 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.hpp 
> a3c924195ae5eecc1caca9cd6fc0f6dc0df0a741 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.cpp 
> c92c6c7174421158b9190ed0bfb00c1c97ef0f7b 
>   src/slave/containerizer/mesos/provisioner/backends/bind.hpp 
> 54c88d940aa64d13114fc5d79ecbc1d474d169a6 
>   src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
> 94dac40a12b6fd2e7d9733444d84763c77785402 
>   src/slave/containerizer/mesos/provisioner/backends/copy.hpp 
> 131d75521ca38afae651e8d885ebedad77d86a3e 
>   src/slave/containerizer/mesos/provisioner/backends/copy.cpp 
> 9c5354e5fea488618ebdecf0aef9dd2fce555d20 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
> 4d591c5f988d87e0e905f973df5ab15a3386d676 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
> b71a31323aef376c9a28e1d52322a1802fb212ad 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> de2c12140652244bd3de9763ced203b144688ab2 
>   src/tests/containerizer/provisioner_backend_tests.cpp 
> 727bf645dd9337a94f098ab0a816b7e0e0651083 
> 
> Diff: https://reviews.apache.org/r/53115/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

[Jie Yu]

> On Oct. 27, 2016, 5:03 p.m., Jie Yu wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, lines 71-72
> > 
> >
> > I think James made a valid point there that errno might not be 
> > preserved across delete. So using a vector sounds better.
> 
> Qian Zhang wrote:
> I think `delete` will not change `errno`, I verified it by a small 
> program:
> ```
> #include 
> #include 
> 
> int main()
> {
>   char* temp = new char[4];
> 
>   if (open("/xxx", 0) < 0) {
> delete[] temp;
> return errno;
>   }
>  
>   delete[] temp;
>   return 0;
> }
> ```
> This program tries to open a file `/xxx` which does not exist, and when I 
> run it, its exit code is 2 (i.e., `ENOENT`). And actually in stout I see 
> there are also some other codes using `delete` and `ErrnoError` in the same 
> way, e.g.:
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/mktemp.hpp#L40:L41
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/posix/mkdtemp.hpp#L37:L38
> 
> However, I do see there are also some codes which tries to avoid `delete` 
> affecting `ErrnoError` in this way:
> ```
> Error error = ErrnoError();
> delete[] temp;
> return error;
> ```
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/read.hpp#L67:L69
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/sysctl.hpp#L200:L202
> 
> 
> So the codes in stout are inconsistent regarding this issue. I think we 
> should fix it by making them handle this issue in a consistent way, I would 
> prefer the following way:
> ```
> Error error = ErrnoError();
> delete[] temp;
> return error;
> ```
> 
> Let me know your comments :-)
> 
> James Peach wrote:
> I thought about it and I think that Qian Zhan is right that `delete` 
> can't set `errno` because it never fails. If there was an error, it would 
> either throw or segv.

Ah, ok. Sounds good.


- Jie


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


On Oct. 25, 2016, 3:41 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 25, 2016, 3:41 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53159: Fixed duplicate framework ids in "unregistered_frameworks".

[Vinod Kone]

> On Oct. 28, 2016, 8:47 p.m., Jiang Yan Xu wrote:
> > Hey
> > 
> > We now have `mesos::internal::master::Master::Frameworks::recovered` to 
> > track precisely this right?

yea, we unfortunately cannot rely on it because 0.28.0 agents don't send 
framework infos to master. i'll send another review to deprecate 
"unregistered_frameworks" (which contains just the frameworks ids) and add a 
new "recovered_frameworks" which includes the whole framework info. one of the 
cases where backwards compatibility makes it messy.


- Vinod


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


On Oct. 28, 2016, 7:22 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53159/
> ---
> 
> (Updated Oct. 28, 2016, 7:22 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4973 and MESOS-6461
> https://issues.apache.org/jira/browse/MESOS-4973
> https://issues.apache.org/jira/browse/MESOS-6461
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The existing test (MasterTest.OrphanTasks) continues to pass after
> the change. I will try to write another test that spawns multiple
> agents to ensure the duplicate framework ids are not shown.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 716077a23defd84a9fac185c96aa52073718c58d 
> 
> Diff: https://reviews.apache.org/r/53159/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 53124: Add documentation on Windows support

[Joseph Wu]

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



Also consider watching this:
https://www.youtube.com/watch?v=bbyK1EBxrek=27=PLGeM09tlguZQVL7ZsfNMffX9h1rGNVqnC

Many of the limitations discussed there are still applicable.


docs/windows.md (line 8)


Considering how different the two getting-started pages are (lots of manual 
steps), I'd actually like to move the content over here.  You can leave a link 
in the getting-started page to this file.



docs/windows.md (line 12)


Nit: s/Linux/Posix/



docs/windows.md (line 13)


Micro-nit: s/launcher_dir/--launcher_dir/

And backticks around `MAX_PATH`



docs/windows.md (line 14)


Technically, this is due to Marathon appending a UUID (36 characters) onto 
the TaskID.  So an ID of < 40 characters is still possible.


- Joseph Wu


On Oct. 23, 2016, 3:09 p.m., Lior Zeno wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53124/
> ---
> 
> (Updated Oct. 23, 2016, 3:09 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Alex Clemmer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds documentation on Windows support in Mesos.
> Currently considered a WIP, published in order to receive inital feedback.
> 
> 
> Diffs
> -
> 
>   docs/home.md f47f7f9 
>   docs/windows.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53124/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Lior Zeno
> 
>

Re: Review Request 53159: Fixed duplicate framework ids in "unregistered_frameworks".

[Jiang Yan Xu]

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



Hey

We now have `mesos::internal::master::Master::Frameworks::recovered` to track 
precisely this right?

- Jiang Yan Xu


On Oct. 28, 2016, 12:22 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53159/
> ---
> 
> (Updated Oct. 28, 2016, 12:22 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4973 and MESOS-6461
> https://issues.apache.org/jira/browse/MESOS-4973
> https://issues.apache.org/jira/browse/MESOS-6461
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The existing test (MasterTest.OrphanTasks) continues to pass after
> the change. I will try to write another test that spawns multiple
> agents to ensure the duplicate framework ids are not shown.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 716077a23defd84a9fac185c96aa52073718c58d 
> 
> Diff: https://reviews.apache.org/r/53159/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 53159: Fixed duplicate framework ids in "unregistered_frameworks".

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53159]

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 Oct. 28, 2016, 7:22 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53159/
> ---
> 
> (Updated Oct. 28, 2016, 7:22 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4973 and MESOS-6461
> https://issues.apache.org/jira/browse/MESOS-4973
> https://issues.apache.org/jira/browse/MESOS-6461
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The existing test (MasterTest.OrphanTasks) continues to pass after
> the change. I will try to write another test that spawns multiple
> agents to ensure the duplicate framework ids are not shown.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 716077a23defd84a9fac185c96aa52073718c58d 
> 
> Diff: https://reviews.apache.org/r/53159/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 53271: Combined stdout/stdout in ASF CI builds.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 28, 2016, 8:27 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53271/
> ---
> 
> (Updated Oct. 28, 2016, 8:27 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Mesos CI running on Apache infrastructure uses this file to build.
> 
> These builds sometimes show interleaved stdout/stderr, especially
> when running tests; where gtest primarily prints test statuses to
> stdout and glog prints verbose output to stderr.  This output becomes 
> less useful for debugging test failures when the start of glog output
> does not line up with the gtest output.
> 
> 
> Diffs
> -
> 
>   support/docker_build.sh 6ba9cf0af20f1dd45ddde393c8fb6622fac42436 
> 
> Diff: https://reviews.apache.org/r/53271/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 53271: Combined stdout/stdout in ASF CI builds.

[Joseph Wu]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

The Mesos CI running on Apache infrastructure uses this file to build.

These builds sometimes show interleaved stdout/stderr, especially
when running tests; where gtest primarily prints test statuses to
stdout and glog prints verbose output to stderr.  This output becomes 
less useful for debugging test failures when the start of glog output
does not line up with the gtest output.


Diffs
-

  support/docker_build.sh 6ba9cf0af20f1dd45ddde393c8fb6622fac42436 

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


Testing
---


Thanks,

Joseph Wu

Re: Review Request 53270: Fixed MesosNativeLibrary to use '_NUM' MESOS_VERSION macros.

[Anand Mazumdar]

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


Ship it!




- Anand Mazumdar


On Oct. 28, 2016, 7:53 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53270/
> ---
> 
> (Updated Oct. 28, 2016, 7:53 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6502
> https://issues.apache.org/jira/browse/MESOS-6502
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed MesosNativeLibrary to use '_NUM' MESOS_VERSION macros.
> 
> 
> Diffs
> -
> 
>   src/java/jni/org_apache_mesos_MesosNativeLibrary.cpp 
> ff7f3c9c2737b074866781bd58c1ba501694a95b 
> 
> Diff: https://reviews.apache.org/r/53270/diff/
> 
> 
> Testing
> ---
> 
> loaded libmesos
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Review Request 53270: Fixed MesosNativeLibrary to use '_NUM' MESOS_VERSION macros.

[Joris Van Remoortere]

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

Review request for mesos, Anand Mazumdar and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fixed MesosNativeLibrary to use '_NUM' MESOS_VERSION macros.


Diffs
-

  src/java/jni/org_apache_mesos_MesosNativeLibrary.cpp 
ff7f3c9c2737b074866781bd58c1ba501694a95b 

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


Testing
---

loaded libmesos


Thanks,

Joris Van Remoortere

Re: Review Request 53159: Fixed duplicate framework ids in "unregistered_frameworks".

[Vinod Kone]

> On Oct. 28, 2016, 6:33 p.m., Anand Mazumdar wrote:
> > LGTM!
> > 
> > Do you want to file another issue to track adding the test or keep the 
> > existing issue open?

will create another ticket for the test.


- Vinod


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


On Oct. 28, 2016, 7:22 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53159/
> ---
> 
> (Updated Oct. 28, 2016, 7:22 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4973 and MESOS-6461
> https://issues.apache.org/jira/browse/MESOS-4973
> https://issues.apache.org/jira/browse/MESOS-6461
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The existing test (MasterTest.OrphanTasks) continues to pass after
> the change. I will try to write another test that spawns multiple
> agents to ensure the duplicate framework ids are not shown.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 716077a23defd84a9fac185c96aa52073718c58d 
> 
> Diff: https://reviews.apache.org/r/53159/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 53159: Fixed duplicate framework ids in "unregistered_frameworks".

[Vinod Kone]

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

(Updated Oct. 28, 2016, 7:22 p.m.)


Review request for mesos, Anand Mazumdar, Gilbert Song, and Jiang Yan Xu.


Changes
---

added MESOS-4973 to bugs.


Bugs: MESOS-4973 and MESOS-6461
https://issues.apache.org/jira/browse/MESOS-4973
https://issues.apache.org/jira/browse/MESOS-6461


Repository: mesos


Description
---

The existing test (MasterTest.OrphanTasks) continues to pass after
the change. I will try to write another test that spawns multiple
agents to ensure the duplicate framework ids are not shown.


Diffs
-

  src/master/http.cpp 716077a23defd84a9fac185c96aa52073718c58d 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 53267: Added log and counter for tracking subscribers.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53267]

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 Oct. 28, 2016, 5 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53267/
> ---
> 
> (Updated Oct. 28, 2016, 5 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar.
> 
> 
> Bugs: MESOS-6499
> https://issues.apache.org/jira/browse/MESOS-6499
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added log and counter for tracking subscribers.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 87186c6e733a686f96528b1722fda1c287e9c881 
>   src/master/master.cpp 23ddb995b4ad0fcdb589974308a2e81ececdad31 
>   src/master/metrics.hpp 056d290c1e14a6afd5056873ef7bb07b5892ed32 
>   src/master/metrics.cpp 1f049f3794c1bca45d2684cbbec3b08c1a78c494 
> 
> Diff: https://reviews.apache.org/r/53267/diff/
> 
> 
> Testing
> ---
> 
> Ran this on master with a client keeps subscribing and disconnecting. 
> Observes that gauge gets updated and new log line is printed.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 53159: Fixed duplicate framework ids in "unregistered_frameworks".

[Anand Mazumdar]

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


Ship it!




LGTM!

Do you want to file another issue to track adding the test or keep the existing 
issue open?

- Anand Mazumdar


On Oct. 28, 2016, 6:23 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53159/
> ---
> 
> (Updated Oct. 28, 2016, 6:23 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6461
> https://issues.apache.org/jira/browse/MESOS-6461
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The existing test (MasterTest.OrphanTasks) continues to pass after
> the change. I will try to write another test that spawns multiple
> agents to ensure the duplicate framework ids are not shown.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 716077a23defd84a9fac185c96aa52073718c58d 
> 
> Diff: https://reviews.apache.org/r/53159/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 53266: Made DefaultExecutorTests pass when running on hosts without Docker.

[Vinod Kone]

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



I don't follow what is happening in this review. Can you please expand the 
description?

- Vinod Kone


On Oct. 28, 2016, 3:28 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53266/
> ---
> 
> (Updated Oct. 28, 2016, 3:28 p.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6455
> https://issues.apache.org/jira/browse/MESOS-6455
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See the summary.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 70779e8d94fbb6e664cad4ddbfeb19e560176cfe 
> 
> Diff: https://reviews.apache.org/r/53266/diff/
> 
> 
> Testing
> ---
> 
> sudo bin/mesos-tests.sh --gtest_filter=DefaultExecutorTest.* --docker=/tmp/foo
> sudo bin/mesos-tests.sh --gtest_filter=DefaultExecutorTest.*
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 53266: Made DefaultExecutorTests pass when running on hosts without Docker.

[Gastón Kleiman]

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

(Updated Oct. 28, 2016, 6:27 p.m.)


Review request for mesos, Till Toenshoff and Jiang Yan Xu.


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


Repository: mesos


Description
---

Some of the default executor tests use a parameterized value to choose
which containerizers to use. This results in tests that require Docker,
but don't contain "DOCKER_" in their names.

This patch uses fixtures instead of parameterized values for this tests,
in order to be able to specify a custom name for the ones that use the
Docker containerizer.


Diffs
-

  src/tests/default_executor_tests.cpp 70779e8d94fbb6e664cad4ddbfeb19e560176cfe 

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


Testing (updated)
---

`sudo bin/mesos-tests.sh --gtest_filter=DefaultExecutorTest.* --docker=/tmp/foo`
`sudo bin/mesos-tests.sh --gtest_filter=DefaultExecutorTest.*`


Thanks,

Gastón Kleiman

Re: Review Request 53266: Made DefaultExecutorTests pass when running on hosts without Docker.

[Gastón Kleiman]

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

(Updated Oct. 28, 2016, 6:27 p.m.)


Review request for mesos, Till Toenshoff and Jiang Yan Xu.


Changes
---

Added a better description.


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


Repository: mesos


Description (updated)
---

Some of the default executor tests use a parameterized value to choose
which containerizers to use. This results in tests that require Docker,
but don't contain "DOCKER_" in their names.

This patch uses fixtures instead of parameterized values for this tests,
in order to be able to specify a custom name for the ones that use the
Docker containerizer.


Diffs
-

  src/tests/default_executor_tests.cpp 70779e8d94fbb6e664cad4ddbfeb19e560176cfe 

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


Testing
---

sudo bin/mesos-tests.sh --gtest_filter=DefaultExecutorTest.* --docker=/tmp/foo
sudo bin/mesos-tests.sh --gtest_filter=DefaultExecutorTest.*


Thanks,

Gastón Kleiman

Re: Review Request 53159: Fixed duplicate framework ids in "unregistered_frameworks".

[Vinod Kone]

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

(Updated Oct. 28, 2016, 6:22 p.m.)


Review request for mesos, Anand Mazumdar and Gilbert Song.


Changes
---

Fixed /state as well.


Summary (updated)
-

Fixed duplicate framework ids in "unregistered_frameworks".


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


Repository: mesos


Description
---

The existing test (MasterTest.OrphanTasks) continues to pass after
the change. I will try to write another test that spawns multiple
agents to ensure the duplicate framework ids are not shown.


Diffs (updated)
-

  src/master/http.cpp 716077a23defd84a9fac185c96aa52073718c58d 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 53266: Made DefaultExecutorTests pass when running on hosts without Docker.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53266]

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 Oct. 28, 2016, 3:28 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53266/
> ---
> 
> (Updated Oct. 28, 2016, 3:28 p.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6455
> https://issues.apache.org/jira/browse/MESOS-6455
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See the summary.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 70779e8d94fbb6e664cad4ddbfeb19e560176cfe 
> 
> Diff: https://reviews.apache.org/r/53266/diff/
> 
> 
> Testing
> ---
> 
> sudo bin/mesos-tests.sh --gtest_filter=DefaultExecutorTest.* --docker=/tmp/foo
> sudo bin/mesos-tests.sh --gtest_filter=DefaultExecutorTest.*
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Review Request 53267: Added log and counter for tracking subscribers.

[Zhitao Li]

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

Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar.


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


Repository: mesos


Description
---

Added log and counter for tracking subscribers.


Diffs
-

  src/master/master.hpp 87186c6e733a686f96528b1722fda1c287e9c881 
  src/master/master.cpp 23ddb995b4ad0fcdb589974308a2e81ececdad31 
  src/master/metrics.hpp 056d290c1e14a6afd5056873ef7bb07b5892ed32 
  src/master/metrics.cpp 1f049f3794c1bca45d2684cbbec3b08c1a78c494 

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


Testing
---

Ran this on master with a client keeps subscribing and disconnecting. Observes 
that gauge gets updated and new log line is printed.


Thanks,

Zhitao Li

Review Request 53266: Made DefaultExecutorTests succeed when running on hosts without Docker.

[Gastón Kleiman]

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

Review request for mesos, Till Toenshoff and Jiang Yan Xu.


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


Repository: mesos


Description
---

See the summary.


Diffs
-

  src/tests/default_executor_tests.cpp 70779e8d94fbb6e664cad4ddbfeb19e560176cfe 

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


Testing
---

sudo bin/mesos-tests.sh --gtest_filter=DefaultExecutorTest.* --docker=/tmp/foo
sudo bin/mesos-tests.sh --gtest_filter=DefaultExecutorTest.*


Thanks,

Gastón Kleiman

Re: Review Request 53266: Made DefaultExecutorTests pass when running on hosts without Docker.

[Gastón Kleiman]

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

(Updated Oct. 28, 2016, 3:28 p.m.)


Review request for mesos, Till Toenshoff and Jiang Yan Xu.


Summary (updated)
-

Made DefaultExecutorTests pass when running on hosts without Docker.


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


Repository: mesos


Description
---

See the summary.


Diffs
-

  src/tests/default_executor_tests.cpp 70779e8d94fbb6e664cad4ddbfeb19e560176cfe 

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


Testing
---

sudo bin/mesos-tests.sh --gtest_filter=DefaultExecutorTest.* --docker=/tmp/foo
sudo bin/mesos-tests.sh --gtest_filter=DefaultExecutorTest.*


Thanks,

Gastón Kleiman

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

[James Peach]

> On Oct. 27, 2016, 5:03 p.m., Jie Yu wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, lines 71-72
> > 
> >
> > I think James made a valid point there that errno might not be 
> > preserved across delete. So using a vector sounds better.
> 
> Qian Zhang wrote:
> I think `delete` will not change `errno`, I verified it by a small 
> program:
> ```
> #include 
> #include 
> 
> int main()
> {
>   char* temp = new char[4];
> 
>   if (open("/xxx", 0) < 0) {
> delete[] temp;
> return errno;
>   }
>  
>   delete[] temp;
>   return 0;
> }
> ```
> This program tries to open a file `/xxx` which does not exist, and when I 
> run it, its exit code is 2 (i.e., `ENOENT`). And actually in stout I see 
> there are also some other codes using `delete` and `ErrnoError` in the same 
> way, e.g.:
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/mktemp.hpp#L40:L41
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/posix/mkdtemp.hpp#L37:L38
> 
> However, I do see there are also some codes which tries to avoid `delete` 
> affecting `ErrnoError` in this way:
> ```
> Error error = ErrnoError();
> delete[] temp;
> return error;
> ```
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/read.hpp#L67:L69
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/sysctl.hpp#L200:L202
> 
> 
> So the codes in stout are inconsistent regarding this issue. I think we 
> should fix it by making them handle this issue in a consistent way, I would 
> prefer the following way:
> ```
> Error error = ErrnoError();
> delete[] temp;
> return error;
> ```
> 
> Let me know your comments :-)

I thought about it and I think that Qian Zhan is right that `delete` can't set 
`errno` because it never fails. If there was an error, it would either throw or 
segv.


- James


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


On Oct. 25, 2016, 3:41 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 25, 2016, 3:41 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53247: Populated `MasterInfo` in the v0 Java adapter.

[Joris Van Remoortere]

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


Ship it!




Ship It!

- Joris Van Remoortere


On Oct. 27, 2016, 11:11 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53247/
> ---
> 
> (Updated Oct. 27, 2016, 11:11 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Vinod Kone.
> 
> 
> Bugs: MESOS-6497
> https://issues.apache.org/jira/browse/MESOS-6497
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Populated `MasterInfo` in the v0 Java adapter.
> 
> 
> Diffs
> -
> 
>   src/examples/java/V1TestFramework.java 
> ec269196671004d7faac500b7267cff756efe1fd 
>   src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
> 7febe95aa23960a3e1ca5e5be44e9ccf9fd840b8 
> 
> Diff: https://reviews.apache.org/r/53247/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 53246: Populated `MasterInfo` evolving from v0 framework registered message.

[Joris Van Remoortere]

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


Ship it!




Ship It!

- Joris Van Remoortere


On Oct. 27, 2016, 11:11 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53246/
> ---
> 
> (Updated Oct. 27, 2016, 11:11 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Vinod Kone.
> 
> 
> Bugs: MESOS-6497
> https://issues.apache.org/jira/browse/MESOS-6497
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.cpp 8d5824fcb0f8026d290d0b47d4b6863f4e455fed 
>   src/tests/scheduler_tests.cpp 6e876a7184204407c8ba4cb2300e095c29a6c8fb 
> 
> Diff: https://reviews.apache.org/r/53246/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 53084: Support `LIBPROCESS_SSL_ENABLED` in the default executor and scheduler.

[Till Toenshoff]

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


Ship it!




Ship It!

- Till Toenshoff


On Oct. 28, 2016, 11:41 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53084/
> ---
> 
> (Updated Oct. 28, 2016, 11:41 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, haosdent huang, Jie Yu, Till 
> Toenshoff, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6494
> https://issues.apache.org/jira/browse/MESOS-6494
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `LIBPROCESS_SSL_ENABLED` is preferred over `SSL_ENABLED`, and the latter
> will be deprecated soon.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp af4a97f7de5f2157aa65fdab742455b0683c40a4 
>   src/scheduler/scheduler.cpp e282d419119dd1f01e170acf5cc2c6175b59791d 
> 
> Diff: https://reviews.apache.org/r/53084/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52840: Silenced a GMock warning in ROOT_DOCKER_DockerInspectDiscard.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Oct. 20, 2016, 9:31 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52840/
> ---
> 
> (Updated Oct. 20, 2016, 9:31 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Silenced a GMock warning in ROOT_DOCKER_DockerInspectDiscard.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/52840/diff/
> 
> 
> Testing
> ---
> 
> Without this patch, `"sudo ./src/mesos-tests 
> --gtest_filter="DockerContainerizerTest.ROOT_DOCKER_DockerInspectDiscard"` 
> consistently results in:
> 
> ```
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DockerInspectDiscard
> 
> GMOCK WARNING:
> Uninteresting mock function call - returning directly.
> Function call: executorLost(0x7ffe1aad15e0, @0x7f2018000b00 e1, 
> @0x7f2018000c30 0aec7dbf-3ce3-4d3a-a086-a95bd1e8dada-S0, -1)
> Stack trace:
> [   OK ] DockerContainerizerTest.ROOT_DOCKER_DockerInspectDiscard (624 ms)
> ```
> 
> With this patch, running the test 100 times in a loop does not produce a 
> warning.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 52879: Cleaned up the way in which the executors load configuration options.

[Gastón Kleiman]

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

(Updated Oct. 28, 2016, 11:43 a.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
Jie Yu, and Jiang Yan Xu.


Changes
---

Yet another rebase.


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


Repository: mesos


Description
---

Updated the executors so that they use only `stout::flags` to load
config options.

They used to use a mix of `stout::flags` and `os::getenv`.


Diffs (updated)
-

  src/CMakeLists.txt 639f8678ba23c4d9a2ea0bf84fbc3d6fc9286ef3 
  src/Makefile.am 769e998d80fde17bcb1ee6c5091ce13a1ad16137 
  src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 
  src/docker/executor.cpp ab3f0473fdc9105d1c425f0dbe7b81c566d541e8 
  src/exec/exec.cpp 1dc20390907253a466b7272b7f8b33ea14afb236 
  src/exec/flags.hpp PRE-CREATION 
  src/exec/flags.cpp PRE-CREATION 
  src/executor/executor.cpp 1d47b52d89eedee59d160badd6313d34e3bdb6d2 
  src/executor/flags.hpp PRE-CREATION 
  src/executor/flags.cpp PRE-CREATION 
  src/launcher/default_executor.cpp af4a97f7de5f2157aa65fdab742455b0683c40a4 
  src/launcher/executor.hpp 217680d99d6e8c31130d7dc714f8cd730f360852 
  src/launcher/executor.cpp 0544121e679db503fe4eaf23a24e315eb188a520 
  src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
  src/tests/containerizer.cpp 69b93c70dc883e1edff3e6c2d7c8da9ef05e42f8 

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


Testing
---

`make check`


Thanks,

Gastón Kleiman

Re: Review Request 53084: Support `LIBPROCESS_SSL_ENABLED` in the default executor and scheduler.

[Gastón Kleiman]

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

(Updated Oct. 28, 2016, 11:41 a.m.)


Review request for mesos, Alexander Rukletsov, haosdent huang, Jie Yu, Till 
Toenshoff, and Jiang Yan Xu.


Changes
---

Made the patch a bit smaller =).


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


Repository: mesos


Description
---

`LIBPROCESS_SSL_ENABLED` is preferred over `SSL_ENABLED`, and the latter
will be deprecated soon.


Diffs (updated)
-

  src/launcher/default_executor.cpp af4a97f7de5f2157aa65fdab742455b0683c40a4 
  src/scheduler/scheduler.cpp e282d419119dd1f01e170acf5cc2c6175b59791d 

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


Testing
---

make check


Thanks,

Gastón Kleiman

Re: Review Request 53115: Implemented handling AUFS whiteouts for copy backend.

[Qian Zhang]

> On Oct. 28, 2016, 10:03 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/backends/copy.cpp, line 213
> > 
> >
> > Hum, IIUC, looks like this is not entirely correct. According to this:
> > 
> > https://github.com/opencontainers/image-spec/blob/master/layer.md#whiteouts
> > 
> > "Files that are present in the same layer as a whiteout file can only 
> > be hidden by whiteout files in subsequent layers."
> > 
> > So we cannot cp first and then process whiteouts. Looks like before cp 
> > each layer, we need to do a pre-processing to handle whiteouts first (and 
> > remove the whiteouts), and then do the cp.

Thanks for pointing this out! I think the flow should be:
1. Delete the related files in rootfs based on the whiteout files in the layer.
2. Copy the layer to rootfs.
3. Once we have done 1 and 2 for all layers, traverse rootfs to remove all the 
whiteout files.
Or maybe in 2 we can do a seletively copy by `find` + `cp` to only copy 
non-whiteout files from layer to rootfs? In this way, we do not need 3.


> On Oct. 28, 2016, 10:03 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/backends/copy.cpp, line 133
> > 
> >
> > I am not sure if we need to pass in 'image' to backend. If we added OCI 
> > support, do we need to add another check here?
> > 
> > I think we can simply assume the layer's whiteout is in aufs whiteout 
> > format, no matter what image type it is. I think APPC, we probably needs to 
> > do the same, right?
> > 
> > You probably add a NOTE there saying that we assume all image type uses 
> > aufs whiteout format.
> > 
> > In that way, no need to pass 'image' around to backend.

The reason I passed in `image` and added this check is that previously we have 
such check in `ProvisionerProcess::__provision()`:
https://github.com/apache/mesos/blob/1.0.1/src/slave/containerizer/mesos/provisioner/provisioner.cpp#L334

I think one of the purposes of this check is to ensure we will NOT go through 
the logic to handle whiteout files for APPC. I think this is correct because 
APPC seems have a totally different way for whiteout, in its image manifest 
(https://github.com/appc/spec/blob/master/spec/aci.md ), there is a field 
`pathWhitelist`:
```
pathWhitelist (list of strings, optional): whitelist of absolute paths that 
will exist in the app's rootfs after rendering. This must be a complete and 
absolute set. An empty list is equivalent to an absent value and means that all 
files in this image and any dependencies will be available in the rootfs.
```
And there is PR in APPC to translate Docker whiteout files to `pathWhitelist`: 
https://github.com/appc/docker2aci/pull/36

So I think if we do not pass in `image`, then copy backend will try to handle 
whiteout files for APPC image which is not necessary since there is no whiteout 
files to handle, instead, we should support `pathWhitelist` which seems missed 
in https://github.com/apache/mesos/blob/1.0.1/include/mesos/appc/spec.proto , 
that is another issue.


- Qian


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


On Oct. 25, 2016, 11:47 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53115/
> ---
> 
> (Updated Oct. 25, 2016, 11:47 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented handling AUFS whiteouts for copy backend.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/backend.hpp 
> 7257d3a962ecdf87fe9d52facbd6a2619311a018 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.hpp 
> a3c924195ae5eecc1caca9cd6fc0f6dc0df0a741 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.cpp 
> c92c6c7174421158b9190ed0bfb00c1c97ef0f7b 
>   src/slave/containerizer/mesos/provisioner/backends/bind.hpp 
> 54c88d940aa64d13114fc5d79ecbc1d474d169a6 
>   src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
> 94dac40a12b6fd2e7d9733444d84763c77785402 
>   src/slave/containerizer/mesos/provisioner/backends/copy.hpp 
> 131d75521ca38afae651e8d885ebedad77d86a3e 
>   src/slave/containerizer/mesos/provisioner/backends/copy.cpp 
> 9c5354e5fea488618ebdecf0aef9dd2fce555d20 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
> 4d591c5f988d87e0e905f973df5ab15a3386d676 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
> 

Re: Review Request 53083: Made `cgroups::create` create cgroup recursively as default behaviour.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53252, 53253, 51031, 51185, 53082, 53083]

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 Oct. 28, 2016, 5:15 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53083/
> ---
> 
> (Updated Oct. 28, 2016, 5:15 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `cgroups::create` create cgroup recursively as default behaviour.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp cfce09cb57501f2c988a8d997d7c6150280ed53d 
>   src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
>   src/tests/containerizer/cgroups_tests.cpp 
> 0afaec6ae948cabf1472bf01103210d8f9809cb1 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> e6c690c411f57138207044f31b4816bd4090c1b7 
> 
> Diff: https://reviews.apache.org/r/53083/diff/
> 
> 
> Testing
> ---
> 
> ```
> sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="*CGROUPS*" --verbose
> sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="*Cgroups*" --verbose
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 53230: Added test cases for HTTPS health check.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/tests/health_check_tests.cpp (lines 1662 - 1664)


Is that `https_server.py` something that you wrote modified? If so, could 
you please post here links to the dockerfile and the test server?

We should make tests transparent and reproducible for anyone.



src/tests/health_check_tests.cpp (lines 1978 - 1980)


Same here.


- Alexander Rukletsov


On Oct. 27, 2016, 4:06 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53230/
> ---
> 
> (Updated Oct. 27, 2016, 4:06 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6493
> https://issues.apache.org/jira/browse/MESOS-6493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for HTTPS health check.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp b1a8942eb1841f5f795b01e08cb017eedf62c8f4 
> 
> Diff: https://reviews.apache.org/r/53230/diff/
> 
> 
> Testing
> ---
> 
> ```
> sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="*HealthCheckTest*" 
> --verbose
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

[Qian Zhang]

> On Oct. 28, 2016, 1:03 a.m., Jie Yu wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, lines 71-72
> > 
> >
> > I think James made a valid point there that errno might not be 
> > preserved across delete. So using a vector sounds better.

I think `delete` will not change `errno`, I verified it by a small program:
```
#include 
#include 

int main()
{
  char* temp = new char[4];

  if (open("/xxx", 0) < 0) {
delete[] temp;
return errno;
  }
 
  delete[] temp;
  return 0;
}
```
This program tries to open a file `/xxx` which does not exist, and when I run 
it, its exit code is 2 (i.e., `ENOENT`). And actually in stout I see there are 
also some other codes using `delete` and `ErrnoError` in the same way, e.g.:
https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/mktemp.hpp#L40:L41
https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/posix/mkdtemp.hpp#L37:L38

However, I do see there are also some codes which tries to avoid `delete` 
affecting `ErrnoError` in this way:
```
Error error = ErrnoError();
delete[] temp;
return error;
```
https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/read.hpp#L67:L69
https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/sysctl.hpp#L200:L202


So the codes in stout are inconsistent regarding this issue. I think we should 
fix it by making them handle this issue in a consistent way, I would prefer the 
following way:
```
Error error = ErrnoError();
delete[] temp;
return error;
```

Let me know your comments :-)


- Qian


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


On Oct. 25, 2016, 11:41 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 25, 2016, 11:41 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53058: Added tests for whole protobuf message based authorization.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52600, 53057, 53058]

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 Oct. 28, 2016, 4:11 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53058/
> ---
> 
> (Updated Oct. 28, 2016, 4:11 a.m.)
> 
> 
> Review request for mesos, Adam B, Kapil Arya, and Till Toenshoff.
> 
> 
> Bugs: MESOS-6401
> https://issues.apache.org/jira/browse/MESOS-6401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for the authorization of the authorization request
> which makes use of whole protobuf messages.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 5d7e17b67821357b8cb538798acc883945c8f8fd 
> 
> Diff: https://reviews.apache.org/r/53058/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>