date:20150907

Re: Review Request 36908: Added QuotaInfo Protobuf.

2015-09-07 Thread Qian Zhang


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

Ship it!


Ship It!

- Qian Zhang


On Sept. 2, 2015, 9:38 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36908/
> ---
> 
> (Updated Sept. 2, 2015, 9:38 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3164
> https://issues.apache.org/jira/browse/MESOS-3164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added QuotaInfo Protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/quota.hpp PRE-CREATION 
>   include/mesos/master/quota.proto PRE-CREATION 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
> 
> Diff: https://reviews.apache.org/r/36908/diff/
> 
> 
> Testing
> ---
> 
> make distcheck
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Review Request 38172: Stout: Simplified hashset implementation.

2015-09-07 Thread Joris Van Remoortere


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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp 
1839d28638cd82dae10ba9b0f99c1a97cf34f9c9 

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


Testing
---


Thanks,

Joris Van Remoortere

Review Request 38171: Stout: Refactored set to use initializer list for variadic constructor.

2015-09-07 Thread Joris Van Remoortere


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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/set.hpp 
85466dbb36b3ac545562eafe8041ad79993fdf9f 

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


Testing
---


Thanks,

Joris Van Remoortere

Review Request 38173: Stout: Simplified hashmap implementation.

2015-09-07 Thread Joris Van Remoortere


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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp 
f09bea125035aa3621402b83608b233e42877559 

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


Testing
---

make check


Thanks,

Joris Van Remoortere

Re: Review Request 38046: Replace hard-coded reap interval with a constant

2015-09-07 Thread Guangya Liu


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

(Updated 九月 8, 2015, 6:16 a.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


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


Repository: mesos


Description
---

Replace hard-coded reap interval with a constant


Diffs (updated)
-

  src/tests/containerizer/launch_tests.cpp 
d211fc0f665988068c67836ef80916828a0df2bd 
  src/tests/gc_tests.cpp ec27ac7f1fca7f425abcea0eafaac4fae905fb8f 
  src/tests/slave_recovery_tests.cpp 6aae14a3c39b9aee76147b691b0170946e1120b5 
  src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37993: Add explanatory comments for Allocator interface

2015-09-07 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Sept. 8, 2015, 3:18 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 8, 2015, 3:18 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38168: Replace hard-coded reap interval with a constant for 3rdparty

2015-09-07 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [38168]

All tests passed.

- Mesos ReviewBot


On Sept. 8, 2015, 2:41 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38168/
> ---
> 
> (Updated Sept. 8, 2015, 2:41 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-1935
> https://issues.apache.org/jira/browse/MESOS-1935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace hard-coded reap interval with a constant for 3rdparty
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/reap_tests.cpp 
> 642ab97b28a6085dc9837f14ff36a3124387a03b 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> ab7515325e5db0a4fd222bb982f51243d7b7e39d 
> 
> Diff: https://reviews.apache.org/r/38168/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

2015-09-07 Thread Guangya Liu


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

(Updated 九月 8, 2015, 3:18 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.


Changes
---

Update comments for whitelist, only hosts in white list can offer resources.


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


Repository: mesos


Description
---

Add explanatory comments for Allocator interface


Diffs (updated)
-

  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 38046: Replace hard-coded reap interval with a constant

2015-09-07 Thread Guangya Liu



> On 九月 7, 2015, 4:22 p.m., Alexander Rukletsov wrote:
> > I would encourage you to check the reaping concept in 
> > `libprocess/reap.{hpp|cpp}`. Comments in those files should be sufficient 
> > to understand what's reaping and how we use it in Mesos. Though some of the 
> > changes you've made may be related to the reaping interval (I haven't deep 
> > dived into reviewing yet), some of them are definitely not (see examples 
> > below).
> > 
> > How did you choose candidates for replacing? I thought you did `grep`, but 
> > surprisingly there is no `reap_tests.cpp` in your RR. I would love us to 
> > carefully look at each `Seconds(1)` timeout in tests and either replace it 
> > by the reap interval or leave a comment about the nature of that "one 
> > second". What do you think?

I found that we cannot upload one patch with both mesos core part and 3rdparty 
code, I have just uploaded a patch for 3rd party for this: 
https://reviews.apache.org/r/38168/

it is good to add comments for "Seconds(1)", can we handle this in another 
patch? I want to focus on reap part for this patch, hope it is OK. ;-)


> On 九月 7, 2015, 4:22 p.m., Alexander Rukletsov wrote:
> > src/tests/gc_tests.cpp, line 807
> > 
> >
> > This can be related to reaping, but then the comment is misleading. Can 
> > you convince me? : )

The comments is now updated to: Now advance time until the reaper reaps the 
framework.


- Guangya


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


On 九月 8, 2015, 2:41 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38046/
> ---
> 
> (Updated 九月 8, 2015, 2:41 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-1935
> https://issues.apache.org/jira/browse/MESOS-1935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace hard-coded reap interval with a constant
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/launch_tests.cpp 
> d211fc0f665988068c67836ef80916828a0df2bd 
>   src/tests/gc_tests.cpp ec27ac7f1fca7f425abcea0eafaac4fae905fb8f 
>   src/tests/slave_recovery_tests.cpp 6aae14a3c39b9aee76147b691b0170946e1120b5 
>   src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 
> 
> Diff: https://reviews.apache.org/r/38046/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38046: Replace hard-coded reap interval with a constant

2015-09-07 Thread Guangya Liu


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

(Updated 九月 8, 2015, 2:41 a.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


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


Repository: mesos


Description
---

Replace hard-coded reap interval with a constant


Diffs (updated)
-

  src/tests/containerizer/launch_tests.cpp 
d211fc0f665988068c67836ef80916828a0df2bd 
  src/tests/gc_tests.cpp ec27ac7f1fca7f425abcea0eafaac4fae905fb8f 
  src/tests/slave_recovery_tests.cpp 6aae14a3c39b9aee76147b691b0170946e1120b5 
  src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 

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


Testing
---


Thanks,

Guangya Liu

Review Request 38168: Replace hard-coded reap interval with a constant for 3rdparty

2015-09-07 Thread Guangya Liu


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

Review request for mesos, Alexander Rukletsov and Ben Mahler.


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


Repository: mesos


Description
---

Replace hard-coded reap interval with a constant for 3rdparty


Diffs
-

  3rdparty/libprocess/src/tests/reap_tests.cpp 
642ab97b28a6085dc9837f14ff36a3124387a03b 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
ab7515325e5db0a4fd222bb982f51243d7b7e39d 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 38165: MESOS-3377 - Adding CONTAINER_NAME as additional env variable

2015-09-07 Thread Klaus Ma


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



src/docker/docker.cpp (line 414)


Should we provide UT for this fix? please refer to the other test in 
"src/tests".


- Klaus Ma


On Sept. 7, 2015, 6:47 p.m., Wojciech Sielski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38165/
> ---
> 
> (Updated Sept. 7, 2015, 6:47 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3377
> https://issues.apache.org/jira/browse/MESOS-3377
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3377 - Adding CONTAINER_NAME as additional env variable
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp ec2de5436ef5261a7d04eebeeded58435774cd9e 
> 
> Diff: https://reviews.apache.org/r/38165/diff/
> 
> 
> Testing
> ---
> 
> Every docker container that is created by mesos,
> gonna get conatiner dynamic env variable CONTAINER_NAME.
> So it is easy to use by 3rd party application,
> for example: when use with registartor/consul 
> so is it's much easier to deregister itself
> (or put into maintanance mode) before being killed.
> 
> 
> Tested and verified afetr build locally.
> 
> 
> Thanks,
> 
> Wojciech Sielski
> 
>

Re: Review Request 38141: Refactor shared paths in provisioners.

2015-09-07 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [38141]

All tests passed.

- Mesos ReviewBot


On Sept. 7, 2015, 9:31 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38141/
> ---
> 
> (Updated Sept. 7, 2015, 9:31 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactor shared paths in provisioners.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5fdca0f574e7e08c4b1aebed0fac39140c19adfe 
>   src/slave/containerizer/provisioners/appc.cpp 
> fc5ee19df51a3543aaf01c2301b976700610ff57 
>   src/slave/containerizer/provisioners/appc/paths.hpp 
> fb3a1a7295809d745dd15bee6db1f7e8dd99ab33 
>   src/slave/containerizer/provisioners/appc/paths.cpp 
> e6be851e24886d7c886adad4c7ea29ded17bdcbe 
>   src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38141/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

2015-09-07 Thread Timothy Chen


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

(Updated Sept. 7, 2015, 11:31 p.m.)


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


Repository: mesos


Description
---

Joining all the commits around provisioner and local store into one review so 
it's easier to review, as patches
are changing code on top of each other.

All the commits are going to committed together.


Diffs
-

  src/Makefile.am 5fdca0f 
  src/messages/docker_provisioner.hpp PRE-CREATION 
  src/messages/docker_provisioner.proto PRE-CREATION 
  src/slave/containerizer/provisioner.hpp 9e0e0b8 
  src/slave/containerizer/provisioner.cpp 95894c0 
  src/slave/containerizer/provisioners/appc.cpp fc5ee19 
  src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7 
  src/slave/containerizer/provisioners/appc/paths.cpp e6be851 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
  src/slave/flags.hpp b8335aa 
  src/slave/flags.cpp 7539441 
  src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38165: MESOS-3377 - Adding CONTAINER_NAME as additional env variable

2015-09-07 Thread Guangya Liu


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

Ship it!


Ship It!

- Guangya Liu


On 九月 7, 2015, 6:47 p.m., Wojciech Sielski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38165/
> ---
> 
> (Updated 九月 7, 2015, 6:47 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3377
> https://issues.apache.org/jira/browse/MESOS-3377
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3377 - Adding CONTAINER_NAME as additional env variable
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp ec2de5436ef5261a7d04eebeeded58435774cd9e 
> 
> Diff: https://reviews.apache.org/r/38165/diff/
> 
> 
> Testing
> ---
> 
> Every docker container that is created by mesos,
> gonna get conatiner dynamic env variable CONTAINER_NAME.
> So it is easy to use by 3rd party application,
> for example: when use with registartor/consul 
> so is it's much easier to deregister itself
> (or put into maintanance mode) before being killed.
> 
> 
> Tested and verified afetr build locally.
> 
> 
> Thanks,
> 
> Wojciech Sielski
> 
>

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

2015-09-07 Thread Joris Van Remoortere

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

Hi Marco,

Sorry for the delay in looking at your review. I was able to dig in and 
hopefully identify the root cause of the issues you've left open with 
`FIXME(marco)`. Feel free to check with MPark or me offline for more 
information.
I left some preliminary explanatation at line 199 in `subprocess.cpp`.

I also pointed out some quick style things I noticed along the way. I figured I 
would share these early as it will give you the opportunity to clean up the 
review some more and reduce the number of iterations at the end :-)

3rdparty/libprocess/include/process/subprocess.hpp (lines 103 - 110)

Does it make sense to aggregate these into a `Result`? The 
`Some` case would be stdout, the `Error` case would be stederr, and the `None` 
case would represent not known yet?
If you don't need the `None` case then you can use a `Try`.

3rdparty/libprocess/include/process/subprocess.hpp (line 254)

According to the doxygen style guide we also end `@return` comments with a 
period. Here and elsehwere in this patch.

3rdparty/libprocess/include/process/subprocess.hpp (lines 350 - 354)

The indentation is a little different from your comment in 
`subprocess.hpp`. Let's be consistent between them, ideally also with the rest 
of the codebase if you can find some good examples.

3rdparty/libprocess/src/subprocess.cpp (lines 185 - 186)

Here are 2 options:
```
return out().isSome() ? 
  io::read(out().get()) : 
  Failure("Cannot obtain stdout for PID: " + stringify(data->pid));
```

```
if (out().isNone()) {
  return Failure(...);
}

return io::read(out().get());
```

3rdparty/libprocess/src/subprocess.cpp (line 197)

I don't think it hurts to take timeout by const ref here. It helps the 
reader understand you don't intend to copy and modify it.
`const Duration& timeout`.
I acknowledge your have knowledge of the internal layout of the 
datastructure and know it's equally cheap to copy it. If someone came along in 
the future and added to the internal state of `Duration` then they wouldn't 
have to refactor your code :-)

3rdparty/libprocess/src/subprocess.cpp (line 199)

I believe the issues you are running into regarding `FIXME(marco)` are 
rooted in your promise not having a future associated with it.

Usually the patter we follow is:
```
// Create a promise.
Promise* promise = new promise();

// Get the future from the promise.
Future future = promise->future();

// Attach any mandatory chained functions to the future.
future.then([](){ ...; });

// Schedule our async action, and fulfill the promise inside that action.
io::read(fd).then([=]() {
  promise->set(...);
}).onFailed([=](const std::string& err) {
  promise->fail(err);
});

// Return the future to the user for them to attach any of their own 
callbacks.
return future.
```

Feel free to sync with MPark or me to understand this further offline.

3rdparty/libprocess/src/subprocess.cpp (line 207)

You can indent by 2 spaces for an expression continuation.

3rdparty/libprocess/src/subprocess.cpp (lines 207 - 209)

I think your lambda indentation is off
```
[this](const tuple>, 
   Future, 
   Future>& futuresTuple)
```

3rdparty/libprocess/src/subprocess.cpp (lines 232 - 233)

indent by 2, not 4. Elsewhere as well.

3rdparty/libprocess/src/subprocess.cpp (lines 239 - 240)

I would leave a space before the return.

- Joris Van Remoortere

On Aug. 20, 2015, 8:03 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> ---
> 
> (Updated Aug. 20, 2015, 8:03 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3035
> https://issues.apache.org/jira/browse/MESOS-3035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-3035
> 
> The original API for `process::Subprocess` still left a lot of legwork
> to do for the cal

Re: Review Request 38137: Added Docker provisioner and local store

2015-09-07 Thread Mesos ReviewBot


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


Bad patch!

Reviews applied: [38141, 38137]

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

Error:
 2015-09-07 22:18:35 URL:https://reviews.apache.org/r/38137/diff/raw/ 
[86583/86583] -> "38137.patch" [1]
error: patch failed: src/Makefile.am:483
error: src/Makefile.am: patch does not apply
error: patch failed: src/slave/containerizer/provisioners/appc.cpp:33
error: src/slave/containerizer/provisioners/appc.cpp: patch does not apply
error: patch failed: src/slave/containerizer/provisioners/appc/paths.hpp:47
error: src/slave/containerizer/provisioners/appc/paths.hpp: patch does not apply
error: patch failed: src/slave/containerizer/provisioners/appc/paths.cpp:85
error: src/slave/containerizer/provisioners/appc/paths.cpp: patch does not apply
error: src/slave/containerizer/provisioners/paths.hpp: already exists in index
error: src/slave/containerizer/provisioners/paths.cpp: already exists in index
Failed to apply patch

- Mesos ReviewBot


On Sept. 7, 2015, 9:58 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 7, 2015, 9:58 p.m.)
> 
> 
> Review request for mesos, Jojy Varghese, Till Toenshoff, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Joining all the commits around provisioner and local store into one review so 
> it's easier to review, as patches
> are changing code on top of each other.
> 
> All the commits are going to committed together.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5fdca0f 
>   src/messages/docker_provisioner.hpp PRE-CREATION 
>   src/messages/docker_provisioner.proto PRE-CREATION 
>   src/slave/containerizer/provisioner.hpp 9e0e0b8 
>   src/slave/containerizer/provisioner.cpp 95894c0 
>   src/slave/containerizer/provisioners/appc.cpp fc5ee19 
>   src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7 
>   src/slave/containerizer/provisioners/appc/paths.cpp e6be851 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
>   src/slave/flags.hpp b8335aa 
>   src/slave/flags.cpp 7539441 
>   src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

2015-09-07 Thread Timothy Chen


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

(Updated Sept. 7, 2015, 9:58 p.m.)


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


Repository: mesos


Description
---

Joining all the commits around provisioner and local store into one review so 
it's easier to review, as patches
are changing code on top of each other.

All the commits are going to committed together.


Diffs (updated)
-

  src/Makefile.am 5fdca0f 
  src/messages/docker_provisioner.hpp PRE-CREATION 
  src/messages/docker_provisioner.proto PRE-CREATION 
  src/slave/containerizer/provisioner.hpp 9e0e0b8 
  src/slave/containerizer/provisioner.cpp 95894c0 
  src/slave/containerizer/provisioners/appc.cpp fc5ee19 
  src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7 
  src/slave/containerizer/provisioners/appc/paths.cpp e6be851 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
  src/slave/flags.hpp b8335aa 
  src/slave/flags.cpp 7539441 
  src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38137: Added Docker provisioner and local store

2015-09-07 Thread Timothy Chen


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

(Updated Sept. 7, 2015, 9:32 p.m.)


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


Summary (updated)
-

Added Docker provisioner and local store


Repository: mesos


Description
---

Joining all the commits around provisioner and local store into one review so 
it's easier to review, as patches
are changing code on top of each other.

All the commits are going to committed together.


Diffs
-

  src/Makefile.am 5fdca0f 
  src/messages/docker_provisioner.hpp PRE-CREATION 
  src/messages/docker_provisioner.proto PRE-CREATION 
  src/slave/containerizer/provisioner.hpp 9e0e0b8 
  src/slave/containerizer/provisioner.cpp 95894c0 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/flags.hpp b8335aa 
  src/slave/flags.cpp 7539441 
  src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38141: Refactor shared paths in provisioners.

2015-09-07 Thread Timothy Chen


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

(Updated Sept. 7, 2015, 9:31 p.m.)


Review request for mesos and Jiang Yan Xu.


Repository: mesos


Description
---

Refactor shared paths in provisioners.


Diffs (updated)
-

  src/Makefile.am 5fdca0f574e7e08c4b1aebed0fac39140c19adfe 
  src/slave/containerizer/provisioners/appc.cpp 
fc5ee19df51a3543aaf01c2301b976700610ff57 
  src/slave/containerizer/provisioners/appc/paths.hpp 
fb3a1a7295809d745dd15bee6db1f7e8dd99ab33 
  src/slave/containerizer/provisioners/appc/paths.cpp 
e6be851e24886d7c886adad4c7ea29ded17bdcbe 
  src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38158: Refactored Value::Ranges coalesce().

2015-09-07 Thread Alexander Rukletsov


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


I would suggest to write a summarizing comment about how coalescing works, 
preferably with algorithmic complexity : ). This may help us to evaluate 
performance and maybe tune the algorithm later on when we have more usage 
experience and feedback from operators.


src/common/values.cpp (line 91)


Mention it's an in-place operation, please



src/common/values.cpp (lines 92 - 93)


s/is/are

I would suggest we write high level comments. How about this: "We assume 
ranges are sorted in ascending order of the lower endpoint."



src/common/values.cpp (line 94)


A high level comment.

`RepeatedPtrField` is a linear data structure, removing elements from it 
causes relocating all elements beyond the one being deleted.

I would suggest one of the alternatives:
* create a copy
* do two passes, instead of deleting mark the ranges and remove them at 
once during the second pass.

Anyway, a short comment summarizing the algorithm would be great for 
posterity!



src/common/values.cpp (line 109)


s/delete range/delete current ?

How about rewriting this in active voice for clarity? "If `range` subsumes 
`current`, delete `current`"



src/common/values.cpp (line 138)


An idea to re-use code here. This function boils down to inserting an 
element into a sorted range and then performing one step of coalescing from 
this element till the end of the sequence. We can keep the insetion "virual" 
and move the elements once during the second coalescing pass (see my comment 
above).



src/common/values.cpp (line 140)


Let's pull it up to the function comment.



src/common/values.cpp (line 165)


Want to transform this into a high level comment before the block?



src/common/values.cpp (lines 171 - 174)


Nice diagram! Helps a lot.



src/common/values.cpp (lines 184 - 185)


Since you're editing here, mind explaining what an "urange" is?



src/common/values.cpp (line 237)


I though you don't like post-increments : )



src/common/values.cpp (lines 344 - 346)


Let's let compiler do it's job: how about passing `left` by value and not 
creating a copy manually?



src/common/values.cpp (line 360)


Does it make sense to optimize out this call? IIUC, `Values::Ranges` should 
be always in sorted and coalesced state ("normalized") unless modified manually 
via protobuf methods. We can require normalization for `operator+=()` to 
succeed. Similar to how you require sorting for coalescing. I would suggest 
let's define invariants (or pre-conditions and post-conditions) for some 
crucial methods, like operators, `sort()`, `coalesce()` and document them.

If you prefer to leave it, then IIUC you should also `sort()` before 
`coalesce()`.



src/common/values.cpp (line 546)


Just to make sure, you have added just `2` lines to this function (not easy 
on RB):
```
  sort(ranges);
  coalesce(ranges);
```
Correct?



src/common/values.cpp (lines 630 - 637)


I've got confused by two syntacticly same by semantically different 
`begin()`s and `end()`s. Let's leave a note for posterity hinting the 
difference.

Also, `clang-format` suggests the following:
```
void sort(Value::Ranges* ranges)
{
  std::sort(ranges->mutable_range()->begin(),
ranges->mutable_range()->end(),
[](const Value::Range& a,
   const Value::Range& b) { return a.begin() < b.begin(); });
}
```



src/tests/values_tests.cpp (line 128)


Not sure I follow the comment. Did you mean you cannot use `parse()` 
because it will `coalesce()`, while you want to test `sort()` in isolation?



src/tests/values_tests.cpp (line 131)


Let's document what range is created here, like in the `RangesCoalesce` 
test.



src/tests/values_tests.cpp (line 158)


How about an ov

Re: Review Request 38165: MESOS-3377 - Adding CONTAINER_NAME as additional env variable

2015-09-07 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [38165]

All tests passed.

- Mesos ReviewBot


On Sept. 7, 2015, 6:47 p.m., Wojciech Sielski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38165/
> ---
> 
> (Updated Sept. 7, 2015, 6:47 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3377
> https://issues.apache.org/jira/browse/MESOS-3377
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3377 - Adding CONTAINER_NAME as additional env variable
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp ec2de5436ef5261a7d04eebeeded58435774cd9e 
> 
> Diff: https://reviews.apache.org/r/38165/diff/
> 
> 
> Testing
> ---
> 
> Every docker container that is created by mesos,
> gonna get conatiner dynamic env variable CONTAINER_NAME.
> So it is easy to use by 3rd party application,
> for example: when use with registartor/consul 
> so is it's much easier to deregister itself
> (or put into maintanance mode) before being killed.
> 
> 
> Tested and verified afetr build locally.
> 
> 
> Thanks,
> 
> Wojciech Sielski
> 
>

Re: Review Request 38164: Use reverse umount order in LinuxFilesystemIsolatorProcess::cleanup.

2015-09-07 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [38164]

All tests passed.

- Mesos ReviewBot


On Sept. 7, 2015, 6:26 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38164/
> ---
> 
> (Updated Sept. 7, 2015, 6:26 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Jie Yu.
> 
> 
> Bugs: MESOS-3379
> https://issues.apache.org/jira/browse/MESOS-3379
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use reverse umount order in LinuxFilesystemIsolatorProcess::cleanup.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
> 
> Diff: https://reviews.apache.org/r/38164/diff/
> 
> 
> Testing
> ---
> 
> sudo GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="LinuxFilesystemIsolatorTest.*" --verbose
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37200: Refactored DockerImage struct to store a list of layer ids instead of linked list of DockerLayers.

2015-09-07 Thread Timothy Chen



> On Sept. 7, 2015, 3:49 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/provisioners/docker.cpp, line 280
> > 
> >
> > One space indentation too many :)

Also no longer valid :)


> On Sept. 7, 2015, 3:49 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 419
> > 
> >
> > Repeat what? ... seems this comment needs adjustment or removal now 
> > that the previous comment, this one was referring to, got removed.

Also no longer valid :)


> On Sept. 7, 2015, 3:49 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 432
> > 
> >
> > Error checking?!

Moved to this mkdir on create.


- Timothy


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


On Aug. 27, 2015, 11:41 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37200/
> ---
> 
> (Updated Aug. 27, 2015, 11:41 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> Timothy Chen, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored DockerImage struct to store a list of layer ids instead of linked 
> list of DockerLayers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37198: Add Docker image provisioner and copy backend.

2015-09-07 Thread Timothy Chen



> On Sept. 7, 2015, 3:48 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/provisioner.cpp, line 45
> > 
> >
> > This should fit into 80 chars without a linebreak.

Also no longer valid :)


- Timothy


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


On Aug. 27, 2015, 11:40 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37198/
> ---
> 
> (Updated Aug. 27, 2015, 11:40 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> Timothy Chen, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2850
> https://issues.apache.org/jira/browse/MESOS-2850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Docker image provisioner.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 4ff1c463efc527e239317ffa2d8a5607b11d2607 
>   src/slave/containerizer/provisioner.hpp 
> 541dd4e0b2f0c92a45c00cab6132a2be69654838 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/provisioner.hpp 
> a26b8138d8cc3086058b15a797dd15354a84019f 
> 
> Diff: https://reviews.apache.org/r/37198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37198: Add Docker image provisioner and copy backend.

2015-09-07 Thread Timothy Chen


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



src/slave/containerizer/provisioners/docker.hpp (lines 19 - 20)


I'm going to follow Appc's provisioner style for now, which is 
__MESOS_DOCKER_HPP__

We can fix all later.



src/slave/containerizer/provisioners/docker.cpp (lines 182 - 183)


Also no longer valid :)



src/slave/containerizer/provisioners/docker.cpp (line 229)


This is no longer valid.


- Timothy Chen


On Aug. 27, 2015, 11:40 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37198/
> ---
> 
> (Updated Aug. 27, 2015, 11:40 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> Timothy Chen, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2850
> https://issues.apache.org/jira/browse/MESOS-2850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Docker image provisioner.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 4ff1c463efc527e239317ffa2d8a5607b11d2607 
>   src/slave/containerizer/provisioner.hpp 
> 541dd4e0b2f0c92a45c00cab6132a2be69654838 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/provisioner.hpp 
> a26b8138d8cc3086058b15a797dd15354a84019f 
> 
> Diff: https://reviews.apache.org/r/37198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

2015-09-07 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [38158]

All tests passed.

- Mesos ReviewBot


On Sept. 7, 2015, 6:04 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 7, 2015, 6:04 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38165: MESOS-3377 - Adding CONTAINER_NAME as additional env variable

2015-09-07 Thread Wojciech Sielski


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

(Updated Sept. 7, 2015, 6:47 p.m.)


Review request for mesos.


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


Repository: mesos


Description (updated)
---

MESOS-3377 - Adding CONTAINER_NAME as additional env variable


Diffs
-

  src/docker/docker.cpp ec2de5436ef5261a7d04eebeeded58435774cd9e 

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


Testing
---

Every docker container that is created by mesos,
gonna get conatiner dynamic env variable CONTAINER_NAME.
So it is easy to use by 3rd party application,
for example: when use with registartor/consul 
so is it's much easier to deregister itself
(or put into maintanance mode) before being killed.


Tested and verified afetr build locally.


Thanks,

Wojciech Sielski

Re: Review Request 38165: MESOS-3377 - Adding CONTAINER_NAME as additional env variable

2015-09-07 Thread Wojciech Sielski


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

(Updated Sept. 7, 2015, 6:46 p.m.)


Review request for mesos.


Changes
---

correct linking


Summary (updated)
-

MESOS-3377 - Adding CONTAINER_NAME as additional env variable


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


Repository: mesos


Description
---

MOS-3377 - Adding CONTAINER_NAME as additional env variable


Diffs
-

  src/docker/docker.cpp ec2de5436ef5261a7d04eebeeded58435774cd9e 

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


Testing
---

Every docker container that is created by mesos,
gonna get conatiner dynamic env variable CONTAINER_NAME.
So it is easy to use by 3rd party application,
for example: when use with registartor/consul 
so is it's much easier to deregister itself
(or put into maintanance mode) before being killed.


Tested and verified afetr build locally.


Thanks,

Wojciech Sielski

Review Request 38165: MOS-3377 - Adding CONTAINER_NAME as additional env variable

2015-09-07 Thread Wojciech Sielski


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

Review request for mesos.


Bugs: MOS-3377
https://issues.apache.org/jira/browse/MOS-3377


Repository: mesos


Description
---

MOS-3377 - Adding CONTAINER_NAME as additional env variable


Diffs
-

  src/docker/docker.cpp ec2de5436ef5261a7d04eebeeded58435774cd9e 

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


Testing
---

Every docker container that is created by mesos,
gonna get conatiner dynamic env variable CONTAINER_NAME.
So it is easy to use by 3rd party application,
for example: when use with registartor/consul 
so is it's much easier to deregister itself
(or put into maintanance mode) before being killed.


Tested and verified afetr build locally.


Thanks,

Wojciech Sielski

Review Request 38164: Use reverse umount order in LinuxFilesystemIsolatorProcess::cleanup.

2015-09-07 Thread haosdent huang


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

Review request for mesos, Ben Mahler and Jie Yu.


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


Repository: mesos


Description
---

Use reverse umount order in LinuxFilesystemIsolatorProcess::cleanup.


Diffs
-

  src/slave/containerizer/isolators/filesystem/linux.cpp 
0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 

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


Testing
---

sudo GLOG_v=1 ./bin/mesos-tests.sh 
--gtest_filter="LinuxFilesystemIsolatorTest.*" --verbose


Thanks,

haosdent huang

Re: Review Request 37247: Added Docker image reference store.

2015-09-07 Thread Timothy Chen


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


Thanks till these will be fixed and pushed in my big review!

- Timothy Chen


On Aug. 27, 2015, 11:41 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37247/
> ---
> 
> (Updated Aug. 27, 2015, 11:41 p.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Timothy Chen.
> 
> 
> Bugs: MESOS-3021
> https://issues.apache.org/jira/browse/MESOS-3021
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Docker image reference store.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/messages/docker_provisioner.hpp PRE-CREATION 
>   src/messages/docker_provisioner.proto PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37247/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tests will be added in a later review.
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

2015-09-07 Thread Joerg Schad


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

(Updated Sept. 7, 2015, 6:04 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


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


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38158: Refactored Value::Ranges coalesce().

2015-09-07 Thread Joerg Schad


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

(Updated Sept. 7, 2015, 4:57 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


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


Repository: mesos


Description (updated)
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38160: Documented how to expedite event firing.

2015-09-07 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [38161, 38160]

All tests passed.

- Mesos ReviewBot


On Sept. 7, 2015, 4:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38160/
> ---
> 
> (Updated Sept. 7, 2015, 4:04 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 
> 
> Diff: https://reviews.apache.org/r/38160/diff/
> 
> 
> Testing
> ---
> 
> doc rendered in Markdown
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38046: Replace hard-coded reap interval with a constant

2015-09-07 Thread Alexander Rukletsov

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

I would encourage you to check the reaping concept in 
`libprocess/reap.{hpp|cpp}`. Comments in those files should be sufficient to 
understand what's reaping and how we use it in Mesos. Though some of the 
changes you've made may be related to the reaping interval (I haven't deep 
dived into reviewing yet), some of them are definitely not (see examples below).

How did you choose candidates for replacing? I thought you did `grep`, but 
surprisingly there is no `reap_tests.cpp` in your RR. I would love us to 
carefully look at each `Seconds(1)` timeout in tests and either replace it by 
the reap interval or leave a comment about the nature of that "one second". 
What do you think?

src/tests/fault_tolerance_tests.cpp (line 41)

As per comment below, this is no longer needed in this file.

src/tests/fault_tolerance_tests.cpp (line 1566)

This timeout is not related to reaping, please remove. I have filed 
https://issues.apache.org/jira/browse/MESOS-3378 to follow up with the clean-up 
here.

src/tests/gc_tests.cpp (line 664)

Same here, not sure it's reaping-related.

src/tests/gc_tests.cpp (line 807)

This can be related to reaping, but then the comment is misleading. Can you 
convince me? : )

- Alexander Rukletsov

On Sept. 7, 2015, 3:25 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38046/
> ---
> 
> (Updated Sept. 7, 2015, 3:25 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-1935
> https://issues.apache.org/jira/browse/MESOS-1935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace hard-coded reap interval with a constant
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/launch_tests.cpp 
> d211fc0f665988068c67836ef80916828a0df2bd 
>   src/tests/fault_tolerance_tests.cpp 
> 89cb18be96cd60fb77fbcc4acd08cebdcf1ba075 
>   src/tests/gc_tests.cpp ec27ac7f1fca7f425abcea0eafaac4fae905fb8f 
>   src/tests/health_check_tests.cpp 157a56aa06677d8b7a2cef53b29ed05cb4b5d8ea 
>   src/tests/log_tests.cpp f2dd47cfbe73fb18c360a637db009b7d391a782e 
>   src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1 
>   src/tests/rate_limiting_tests.cpp f3aeddee00c7bb7905092aa8a760603468063126 
>   src/tests/slave_recovery_tests.cpp 4d137e0f1278fdacf71f101b1967df35bfbcdd23 
>   src/tests/slave_tests.cpp 24119183ef04961fe2fdac73de7672cbe5b2408c 
> 
> Diff: https://reviews.apache.org/r/38046/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 38160: Documented how to expedite event firing.

2015-09-07 Thread Alexander Rukletsov


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

Review request for mesos, Bernd Mathiske and Ben Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 

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


Testing
---

doc rendered in Markdown


Thanks,

Alexander Rukletsov

Review Request 38161: Replaced a hard-coded number for registration backoff with a proper constant and extended comments.

2015-09-07 Thread Alexander Rukletsov


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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/fault_tolerance_tests.cpp 89cb18be96cd60fb77fbcc4acd08cebdcf1ba075 

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


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 37496: Move docker provisioner local store into dedicated folders.

2015-09-07 Thread Till Toenshoff


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



src/Makefile.am (lines 758 - 760)


Spacing seems off?!



src/slave/containerizer/provisioners/docker/local_store.hpp (line 19)


See my earlier comment on the include-guard naming. In this particular case 
we are missing the `_HPP__` again.


- Till Toenshoff


On Aug. 27, 2015, 11:43 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37496/
> ---
> 
> (Updated Aug. 27, 2015, 11:43 p.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Move docker provisioner local store into dedicated folders.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37496/diff/
> 
> 
> Testing
> ---
> 
> sudo make check 
> ./bin/mesos-tests.sh --gtest_filter="*DockerProvisioner*" --gtest_repeat=20 
> --gtest_shuffle=1
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37495: Docker provisioner local store unit tests.

2015-09-07 Thread Till Toenshoff


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



src/tests/containerizer/docker_provisioner_tests.cpp (line 66)


s/docker Image/Docker image/


- Till Toenshoff


On Aug. 27, 2015, 11:42 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37495/
> ---
> 
> (Updated Aug. 27, 2015, 11:42 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker provisioner local store unit tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37495/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> ./bin/mesos-tests.sh --gtest_filter="*DockerProvisioner*" --gtest_repeat=20 
> --gtest_shuffle=1
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37247: Added Docker image reference store.

2015-09-07 Thread Till Toenshoff

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

I did not realize that we had a combined RR for this chain - please forgive my 
ignorance for now - I shall review the combined one once you fixed the locally 
mentioned issues.

src/Makefile.am (line 234)

Lets wrap this to fit, putting the backslash at column 72.

```
DOCKER_PROVISIONER_PROTOS = 
\
  messages/docker_provisioner.pb.cc \
  messages/docker_provisioner.pb.h
```

src/Makefile.am (line 402)

Tabs still off?!

src/Makefile.am (line 446)

Tabs still off?!

src/Makefile.am (line 730)

Tabs still off?!

src/Makefile.am (line 758)

Tabs still off?!

src/Makefile.am (line 759)

Not yours but tab spacing is off, please correct.

src/slave/containerizer/provisioners/docker/reference_store.hpp (lines 98 - 99)

Can we move the process definition into the implementation file to make the 
headers fully "need to know" only?

src/slave/containerizer/provisioners/docker/store.cpp (line 115)

This seems rather dangerous - you are using the factory method for the 
initializer here but you ignore the fact that there is indeed chances for it to 
return a failed try. I see why you went the way it is now as that resulted in 
very little code. Still, I think we should be a bit more careful here and test 
for failures.

- Till Toenshoff

On Aug. 27, 2015, 11:41 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37247/
> ---
> 
> (Updated Aug. 27, 2015, 11:41 p.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Timothy Chen.
> 
> 
> Bugs: MESOS-3021
> https://issues.apache.org/jira/browse/MESOS-3021
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Docker image reference store.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/messages/docker_provisioner.hpp PRE-CREATION 
>   src/messages/docker_provisioner.proto PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37247/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tests will be added in a later review.
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37200: Refactored DockerImage struct to store a list of layer ids instead of linked list of DockerLayers.

2015-09-07 Thread Till Toenshoff


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


I did not realize that we had a combined RR for this chain - please forgive my 
ignorance for now - I shall review the combined one once you fixed the locally 
mentioned issues.


src/slave/containerizer/provisioners/docker.cpp (line 272)


One space indentation too many :)



src/slave/containerizer/provisioners/docker/store.cpp (line 330)


One more space for the indentation.



src/slave/containerizer/provisioners/docker/store.cpp (line 334)


How about checking for errors here?

```
 const string path = path::join(staging, id, "rootfs");

 Try mkdir = os::mkdir(path);
  if (mkdir.isError()) {
return Error("Failed to create directory '" +
 path + "': " + mkdir.error());
  }
```



src/slave/containerizer/provisioners/docker/store.cpp (line 384)


Repeat what? ... seems this comment needs adjustment or removal now that 
the previous comment, this one was referring to, got removed.



src/slave/containerizer/provisioners/docker/store.cpp (line 397)


Error checking?!



src/slave/containerizer/provisioners/docker/store.cpp (line 401)


Error checking?!


- Till Toenshoff


On Aug. 27, 2015, 11:41 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37200/
> ---
> 
> (Updated Aug. 27, 2015, 11:41 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> Timothy Chen, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored DockerImage struct to store a list of layer ids instead of linked 
> list of DockerLayers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37198: Add Docker image provisioner and copy backend.

2015-09-07 Thread Till Toenshoff


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


I did not realize that we had a combined RR for this chain - please forgive my 
ignorance for now - I shall review the combined one once you fixed the locally 
mentioned issues.


src/slave/containerizer/provisioner.cpp (line 45)


This should fit into 80 chars without a linebreak.



src/slave/containerizer/provisioners/docker.hpp (lines 19 - 20)


I am not sure I like the way this include guard is named as it seems likely 
to conflict with other docker-related headers. Also we commonly include the 
extension in that guard (hpp).

I would suggest using `__SLAVE_CONTAINERIZER_PROVISIONER_DOCKER_HPP__` or 
`__MESOS_SLAVE_CONTAINERIZER_PROVISIONER_DOCKER_HPP__` which would strictly 
follow google's guide.

Too bad we never reached a consensus on MESOS-2211 but given that I see 
potential for a conflict, I would like to ask you to fix this risk no matter 
which scheme you chose.



src/slave/containerizer/provisioners/docker.hpp (line 94)


The ordering for factory/methods and constructors/destructors is not 
matching google's styleguide here but given that we are rather inconsistent 
within the codebase already, I guess I should not start marking those things as 
issues -- please note but ignore for now :)



src/slave/containerizer/provisioners/docker.cpp (lines 182 - 183)


Fits into a single line.



src/slave/containerizer/provisioners/docker.cpp (line 229)


Why 3 slashes?


- Till Toenshoff


On Aug. 27, 2015, 11:40 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37198/
> ---
> 
> (Updated Aug. 27, 2015, 11:40 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> Timothy Chen, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2850
> https://issues.apache.org/jira/browse/MESOS-2850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Docker image provisioner.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 4ff1c463efc527e239317ffa2d8a5607b11d2607 
>   src/slave/containerizer/provisioner.hpp 
> 541dd4e0b2f0c92a45c00cab6132a2be69654838 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/provisioner.hpp 
> a26b8138d8cc3086058b15a797dd15354a84019f 
> 
> Diff: https://reviews.apache.org/r/37198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37827: Added a test for converting JSON arrays to repeated protobufs.

2015-09-07 Thread haosdent huang


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

Ship it!


Ship It!

- haosdent huang


On Sept. 7, 2015, 9:12 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37827/
> ---
> 
> (Updated Sept. 7, 2015, 9:12 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> c56d6a3098293eb3659b3066f10e875927ec3ac3 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
> cfc2803e42284f641879fb24bce1282215c8ea52 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
> a1d4084661345f9367c75f9db61279f032b93e69 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
> bbd36d39e9588eb8eea6d739451ad3bab029ca08 
> 
> Diff: https://reviews.apache.org/r/37827/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> **NOTE**: Filed 
> [MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) to clean up 
> protobuf generation.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 35984: Added tests for /reserve and /unreserve HTTP endpoints.

2015-09-07 Thread Guangya Liu


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



src/tests/reservation_endpoints_tests.cpp (lines 78 - 101)


This piece of code also has some duplicate, is it possible to add merge to 
the helper function? Such as a startMaster and startSlave help function or just 
one helper function as startMesos? Thanks.


- Guangya Liu


On 八月 5, 2015, 9:55 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35984/
> ---
> 
> (Updated 八月 5, 2015, 9:55 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris 
> Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-2600
> https://issues.apache.org/jira/browse/MESOS-2600
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec 
>   src/tests/reservation_endpoints_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/35984/diff/
> 
> 
> Testing
> ---
> 
> (1) Added `src/tests/reservation_endpoints_tests.cpp`
> (2) `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 38154: Switched to type traits for checking whether a type is a protobuf message.

2015-09-07 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [37826, 37827, 37830, 38154]

All tests passed.

- Mesos ReviewBot


On Sept. 7, 2015, 11:24 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38154/
> ---
> 
> (Updated Sept. 7, 2015, 11:24 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 57d5fdf45273c620655b44b5f5572290cffa4bf6 
> 
> Diff: https://reviews.apache.org/r/38154/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 38154: Switched to type traits for checking whether a type is a protobuf message.

2015-09-07 Thread Alexander Rukletsov


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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
57d5fdf45273c620655b44b5f5572290cffa4bf6 

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


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 35983: Added /unreserve HTTP endpoint to the master.

2015-09-07 Thread Guangya Liu


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



src/master/http.cpp (lines 1909 - 1914)


get ur point, here just checkig if there are offers can be unreserved.


- Guangya Liu


On 九月 4, 2015, 11:20 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35983/
> ---
> 
> (Updated 九月 4, 2015, 11:20 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris 
> Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-2600
> https://issues.apache.org/jira/browse/MESOS-2600
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 94e97a2898106579434e8cdec04b7b0e130a810e 
>   src/master/master.hpp e1331851c19e3372a4a525dcfd7ba2a01c3e97a6 
>   src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac 
>   src/master/validation.cpp ffb7bf07b8a40d6e14f922eabcf46045462498b5 
>   src/tests/master_validation_tests.cpp 
> 3513bca6fd6773f712d1a647b1757766dc34f948 
> 
> Diff: https://reviews.apache.org/r/35983/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 37830: Added a test for converting JSON arrays to Resources.

2015-09-07 Thread Alexander Rukletsov


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

(Updated Sept. 7, 2015, 9:12 a.m.)


Review request for mesos, Joseph Wu and Michael Park.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 

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


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 37827: Added a test for converting JSON arrays to repeated protobufs.

2015-09-07 Thread Alexander Rukletsov


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

(Updated Sept. 7, 2015, 9:12 a.m.)


Review request for mesos, haosdent huang, Joseph Wu, and Michael Park.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
c56d6a3098293eb3659b3066f10e875927ec3ac3 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
cfc2803e42284f641879fb24bce1282215c8ea52 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
a1d4084661345f9367c75f9db61279f032b93e69 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
bbd36d39e9588eb8eea6d739451ad3bab029ca08 

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


Testing
---

make check (Mac OS 10.10.4)

**NOTE**: Filed [MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) 
to clean up protobuf generation.


Thanks,

Alexander Rukletsov

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

2015-09-07 Thread Alexander Rukletsov


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

(Updated Sept. 7, 2015, 9:12 a.m.)


Review request for mesos, Joseph Wu and Michael Park.


Changes
---

Michael's comments.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
57d5fdf45273c620655b44b5f5572290cffa4bf6 

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


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 37827: Added a test for converting JSON arrays to repeated protobufs.

2015-09-07 Thread Alexander Rukletsov



> On Sept. 3, 2015, 9:29 p.m., Michael Park wrote:
> > Perhaps a dumb question, but may I ask why you needed to introduce 
> > `SimpleMessage`?

I would like to compare protobuf messages with `EXPECT_EQ`, therefore I need an 
equality operator, which will be too long for the existing `Message` proto.


- Alexander


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


On Sept. 3, 2015, 1:33 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37827/
> ---
> 
> (Updated Sept. 3, 2015, 1:33 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> c56d6a3098293eb3659b3066f10e875927ec3ac3 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
> cfc2803e42284f641879fb24bce1282215c8ea52 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
> a1d4084661345f9367c75f9db61279f032b93e69 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
> bbd36d39e9588eb8eea6d739451ad3bab029ca08 
> 
> Diff: https://reviews.apache.org/r/37827/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> **NOTE**: Filed 
> [MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) to clean up 
> protobuf generation.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

2015-09-07 Thread Alexander Rukletsov



> On Sept. 3, 2015, 11:32 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 594
> > 
> >
> > Do we want to support the following?
> > 
> > ```cpp
> > using google::protobuf::RepeatedPtrField;
> > 
> > auto parse = 
> > protobuf::parse>>(array_of_array);
> > ```
> > 
> > I think we might want to disallow it since it seems that protobuf 
> > definitions cannot express "array of array" (maybe there actually is a way 
> > to do that?). If we do want to disallow it, we should include the following 
> > check at the top.
> > 
> > ```cpp
> > { google::protobuf::Message* message = (T*) NULL; (void) message; }
> > ```

A brilliant suggestion, Michael!


- Alexander


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


On Sept. 3, 2015, 1:32 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> ---
> 
> (Updated Sept. 3, 2015, 1:32 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 57d5fdf45273c620655b44b5f5572290cffa4bf6 
> 
> Diff: https://reviews.apache.org/r/37826/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

2015-09-07 Thread Alexander Rukletsov



> On Sept. 3, 2015, 8:37 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 593
> > 
> >
> > The `Parse` above uses `value`, rather than `json`. Can we 
> > consolidate to one? I think `value` would be more fitting, since we name 
> > `JSON::Object` as `object`, `JSON::Array` as `array`, etc.

I have a nested `foreach` loop where I use `value` already. I'll rename that 
one, if you prefer to keep `value` in the signature.


- Alexander


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


On Sept. 3, 2015, 1:32 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> ---
> 
> (Updated Sept. 3, 2015, 1:32 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 57d5fdf45273c620655b44b5f5572290cffa4bf6 
> 
> Diff: https://reviews.apache.org/r/37826/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

54 matches

Mail list logo