Re: Review Request 30774: Fetcher Cache

[Ben Mahler]

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


Just took a cursory glance since this is a huge diff, could it have been broken 
apart further? We've found large diffs like this one are next to impossible to 
review thoroughly :)


src/slave/containerizer/fetcher.hpp


We might be able to get away with more descriptive names here to avoid the 
need for these comments:

```
Bytes capacity;
Bytes reserved;
unsigned long fileCounter;
```

'space' seems to suggest available space (to me), whereas 'capacity' seems 
pretty standard as a name for this. For 'tally', I can't tell from the name 
what is being tallied, but if we change the name to 'reserved' I have an 
understanding that this is the reserved space, not necessarily occupied but 
reserved for a purpose.



src/slave/containerizer/fetcher.hpp


Hard to tell why shared_ptr here is needed rather than Shared, or just 
Cache::Entry directly. Is there concurrent modification happening, or?



src/slave/containerizer/fetcher.cpp


No need for the stringify here and below.



src/slave/containerizer/fetcher.cpp


CHECK_LT will print the two numbers for you :)



src/slave/containerizer/fetcher.cpp


Seems like an odd message format, since normally a meaning follows from a 
':'

```
Fetcher cache space overflow - space used: 2GB, exceeds total fetcher cache 
space: 1GB
```

Here's another format where the meaning is described after the colon:

```
Fetcher cache space overflow: 2GB used vs 1GB capacity
```



src/slave/containerizer/mesos/containerizer.hpp


I can't tell why slave id is being passed here, is there something subtle 
going on?


- Ben Mahler


On May 21, 2015, 4:05 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated May 21, 2015, 4:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f 
>   include/mesos/type_utils.hpp 837be6f1844d5fa01c0fd84a585e7ff2cc0c987b 
>   src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp ed4ee19c85387882ab2e31baa5610acb8e222d50 
>   src/slave/containerizer/docker.cpp 408a4435a6f11973992486eac1659beeccc4beac 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 3e18617b0dbac58176bfd41dc550898eb6a4a79e 
>   src/s

Re: Review Request 34970: Cleaned up and generalized NoExecutorScheduler to be more configurable.

[Ben Mahler]

> On June 5, 2015, 9:37 p.m., Vinod Kone wrote:
> > src/examples/no_executor_framework.cpp, line 219
> > 
> >
> > LOG(FATAL)? because this is not possible with a command executor?

Sure, this was to stay consistent with the other examples (e.g. 
persistent_volume_framework.cpp), but I'll change it.


> On June 5, 2015, 9:37 p.m., Vinod Kone wrote:
> > src/examples/no_executor_framework.cpp, line 253
> > 
> >
> > s/active/launched/ ?

This is all non-terminal (i.e. active) tasks. Launched seems to imply 
everything ever launched, no?


- Ben


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


On June 5, 2015, 7:56 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34970/
> ---
> 
> (Updated June 5, 2015, 7:56 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-2655
> https://issues.apache.org/jira/browse/MESOS-2655
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now the NoExecutorScheduler can take a custom command, as well as the 
> resources for the task and how many tasks to run.
> 
> This allows us to continue using it for an end-to-end test as part of make 
> check, but also allows it to be used against a cluster in a long-lived 
> fashion.
> 
> 
> Diffs
> -
> 
>   src/examples/no_executor_framework.cpp 
> 1fb853d6e4a3deb3c36e6e661689697c8ebf898f 
>   src/tests/no_executor_framework_test.sh 
> d2d395595b778bc543e1baaa0fd415dc622b647f 
>   src/tests/script.cpp 515e3141e1d517da4cfc1421d5301d0a3bd6ad51 
> 
> Diff: https://reviews.apache.org/r/34970/diff/
> 
> 
> Testing
> ---
> 
> The existing no executor framework test picks this up.
> 
> Since we do not have an non-zero estimator yet, I modified the slave to send 
> revocable resources in order to manually test the revocable case.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 35165: Extend fq_codel to allow parent and handle to be specified at runtime.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35150, 35152, 35165]

All tests passed.

- Mesos ReviewBot


On June 6, 2015, 12:02 a.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35165/
> ---
> 
> (Updated June 6, 2015, 12:02 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Bugs: MESOS-2332
> https://issues.apache.org/jira/browse/MESOS-2332
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extend fq_codel to allow parent and handle to be specified at runtime.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/queueing/fq_codel.hpp 
> 412af2d7da2c70324234ee75df75c71319b1365a 
>   src/linux/routing/queueing/fq_codel.cpp 
> 3db5b9390bb5ae759eb1b7c9d1ac87cfca8d531e 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 871e9cf1625d96d1feef50edd4081972c097d191 
>   src/tests/routing_tests.cpp db5b5df48634ff322baf9328fc605b2667b56eed 
> 
> Diff: https://reviews.apache.org/r/35165/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 35157: Added unit tests for fetching ResoruceUsage in both QoS Controller and Resource Estimator .

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35164, 35157]

All tests passed.

- Mesos ReviewBot


On June 5, 2015, 11:53 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> ---
> 
> (Updated June 5, 2015, 11:53 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, 
> and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage 
> from ResourceMonitor.
> 
> This is the unit test for https://reviews.apache.org/r/34980/ and 
> https://reviews.apache.org/r/35164/
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 
>   src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a 
>   src/tests/oversubscription_tests.cpp 
> afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 
> 
> Diff: https://reviews.apache.org/r/35157/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 34894: Add new message for Traffic Control statistics

[Jie Yu]

> On June 6, 2015, 12:10 a.m., Ian Downes wrote:
> > include/mesos/mesos.proto, line 533
> > 
> >
> > What's the set of possible ids? If it's the well-defined set that we 
> > discussed offline then we should probably enumerate them? @jieyu, thoughts? 
> > Otherwise we'll be string matching on id. For example (with not very good 
> > naming):
> > ```
> > message TrafficControlStatistics {
> >   enum Type {
> >  tx_internal = 1;
> >  tx_external = 2;
> >  rx_internal = 3;
> >  rx_external = 4;
> >   }
> >   required Type type = 1;
> >   optional ...
> > }
> > ```

Ian, I think it's hard to enumerate eventually. What if in the future, we want 
to change the qdisc hierarchy to be more sophisticated (e.g., more intermediate 
filters/qdiscs), and we want to get statistics for all of them? I just feel 
that it'll be very hard to enumerate.

Also, disk usage for volumes, we can not enumerate anyway.


- Jie


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


On June 3, 2015, 9:54 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> ---
> 
> (Updated June 3, 2015, 9:54 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add new message for Traffic Control statistics
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 6186c92c6fe9cf8fa136d93d5af43e0377406baf 
> 
> Diff: https://reviews.apache.org/r/34894/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 34894: Add new message for Traffic Control statistics

[Ian Downes]

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


Much cleaner! Just a question about enumerating the ids/types.


include/mesos/mesos.proto


What's the set of possible ids? If it's the well-defined set that we 
discussed offline then we should probably enumerate them? @jieyu, thoughts? 
Otherwise we'll be string matching on id. For example (with not very good 
naming):
```
message TrafficControlStatistics {
  enum Type {
 tx_internal = 1;
 tx_external = 2;
 rx_internal = 3;
 rx_external = 4;
  }
  required Type type = 1;
  optional ...
}
```


- Ian Downes


On June 3, 2015, 2:54 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> ---
> 
> (Updated June 3, 2015, 2:54 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add new message for Traffic Control statistics
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 6186c92c6fe9cf8fa136d93d5af43e0377406baf 
> 
> Diff: https://reviews.apache.org/r/34894/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 35152: Document and consolidate qdisc handles

[Jie Yu]

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



src/slave/containerizer/isolators/network/port_mapping.cpp


... and filters



src/slave/containerizer/isolators/network/port_mapping.cpp


This is not correct. lo(s) do not talk to each other.


- Jie Yu


On June 5, 2015, 11:55 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35152/
> ---
> 
> (Updated June 5, 2015, 11:55 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Bugs: MESOS-2821
> https://issues.apache.org/jira/browse/MESOS-2821
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document and consolidate qdisc handles
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 871e9cf1625d96d1feef50edd4081972c097d191 
> 
> Diff: https://reviews.apache.org/r/35152/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 35152: Document and consolidate qdisc handles

[Cong Wang]

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



src/slave/containerizer/isolators/network/port_mapping.cpp


s/data/packet/



src/slave/containerizer/isolators/network/port_mapping.cpp


s/filter/class/



src/slave/containerizer/isolators/network/port_mapping.cpp


s/CLASS_ID/HTB_CLASS_ID/ ?


- Cong Wang


On June 5, 2015, 11:55 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35152/
> ---
> 
> (Updated June 5, 2015, 11:55 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Bugs: MESOS-2821
> https://issues.apache.org/jira/browse/MESOS-2821
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document and consolidate qdisc handles
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 871e9cf1625d96d1feef50edd4081972c097d191 
> 
> Diff: https://reviews.apache.org/r/35152/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Review Request 35165: Extend fq_codel to allow parent and handle to be specified at runtime.

[Paul Brett]

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

Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.


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


Repository: mesos


Description
---

Extend fq_codel to allow parent and handle to be specified at runtime.


Diffs
-

  src/linux/routing/queueing/fq_codel.hpp 
412af2d7da2c70324234ee75df75c71319b1365a 
  src/linux/routing/queueing/fq_codel.cpp 
3db5b9390bb5ae759eb1b7c9d1ac87cfca8d531e 
  src/slave/containerizer/isolators/network/port_mapping.cpp 
871e9cf1625d96d1feef50edd4081972c097d191 
  src/tests/routing_tests.cpp db5b5df48634ff322baf9328fc605b2667b56eed 

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


Testing
---

make check


Thanks,

Paul Brett

Re: Review Request 35131: Replaced os::dirname and os::basename with Path::dirname and Path::basename.

[Joerg Schad]

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

Ship it!


Ship It!

- Joerg Schad


On June 5, 2015, 1:20 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35131/
> ---
> 
> (Updated June 5, 2015, 1:20 p.m.)
> 
> 
> Review request for mesos and Cody Maloney.
> 
> 
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da 
> 
> Diff: https://reviews.apache.org/r/35131/diff/
> 
> 
> Testing
> ---
> 
> make check on trailing RR.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 35152: Document and consolidate qdisc handles

[Paul Brett]

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

(Updated June 5, 2015, 11:55 p.m.)


Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.


Changes
---

Address reviewers comments


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


Repository: mesos


Description
---

Document and consolidate qdisc handles


Diffs (updated)
-

  src/slave/containerizer/isolators/network/port_mapping.cpp 
871e9cf1625d96d1feef50edd4081972c097d191 

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


Testing
---

make check


Thanks,

Paul Brett

Re: Review Request 34943: Added validation to flags.

[Marco Massenzio]

> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 184
> > 
> >
> > too much choice, IMO - there are (if I counted them right) 8 different 
> > `add()` variants to choose from (none of them documented :)
> > 
> > It's virtually impossible for folks to figure out which to pick and 
> > this leads, I believe, to the current 'code by copying' approach.
> > 
> > I would cull it down to no more than 2-3 different options, with some 
> > sensible default values.
> 
> Benjamin Hindman wrote:
> I completely agree this needs documentation, which I'll take on in a 
> different review. But there are really only 4 variants here:
> 
> (1) The flag _is_ a class member.
> (A) The flag has a default value (i.e., is _not_ an Option).
> (B) The flag does not have a default value (i.e., _is_ an Option).
> 
> (2) The flag is _not_ a class member.
> (A) The flag has a default value (i.e., is _not_ an Option).
> (B) The flag does not have a default value (i.e., _is_ an Option).
> 
> All 4 of these options can optionally have a validation function, but 
> since we can't capture the default validation function with the existing 4 
> functions we have to do the delegating function trick, resulting in 8 
> functions. Bottom line, this needs to be documented (and if someone has a 
> better suggestion to avoid the delgating function trick I'm all ears).

Why should the callers care if it has a default value, type-wise?

My thinking was, let's always use an `Option` for the flag; a, possibly 
`None()`, default value and a `required` bool flag:
```
template
void add(Option* option, 
 const std::string& name, 
 const std::string& help,
 const Option& const default = None(),
 bool required = false)
```
and similarly for the class member.

So, all we have are *two* options, and no doubt as to which one to use.
I'm sure I'm missing a ton here, though... especially given that:

```
  Option foo;
  Option bar;

  Option def_foo{"default-foo-val"};

  flags.add(&foo, "foo", "this is foo!", def_foo, true);
  flags.add1(&bar, "bar", "this is BAR");
```
works as intended, but:
```
flags.add(&foo, "foo", "this is foo!", "default-foo-val", true);
```
causes a compile error (even though, I can totally see an `Option(const T& _t)` 
constructor that should "just work" (as it very much does for `def_foo`).


> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 500-504
> > 
> >
> > so, with your approach, everyone, every time that they add a required 
> > flag, will have to, essentially, copy & paste this code.
> > 
> > what I had envisioned, was a simpler way (eg, by adding a `bool 
> > required = false` arg) that, if true would automatically do this validation 
> > step in `FlagsBase`
> > 
> > (of course, we could add another `add()` method that has that 
> > signature, that just internally implements this and calls one of the other 
> > `add()` ones - making it the ninth option :) )
> 
> Benjamin Hindman wrote:
> I wasn't trying to solve the "required" flag problem, I was just trying 
> to show an example of a validation function. But I violently agree that this 
> is therefore a horribly bad example that sets a bad precedent, so I've 
> replaced this with a different validation function.
> 
> I also agree that we should introduce a simplier way, such as a bool as 
> you suggested (or better an an enumeration). This doesn't completley solve 
> the problem, however, since a flag without a default value will still be an 
> Option and still require one to do 'CHECK_SOME(flags.flag)'. Got any other 
> ideas?

> horribly bad example
I never said that :)

As mentioned above, a flag should *always* be an `Option` with the 
difference that, if we had marked it as `required` then we'd be confident that, 
once loaded, it does have a value (so, no need to `CHECK_SOME()`) or it would 
have already failed.

For the optional (aka `non-required`) ones (without a default value) obviously 
they could be `None()` but that is an "expected outcome" and the caller can 
take whatever action they deem appropriate.

>From the 'client' perspective:

1. I told you it was `required` --> there must be a value (or you'd have failed 
before getting back to me)
2. I gave you a default value --> there must be a value (possibly the default 
one)
3. It's neither required, nor has a default --> its absence (`isNone() == 
true`) has a meaning to me and I'll take appropriate action
4. It's required, and I gave you a default value --> I'm bad at logical 
thinking ;-)

I would also propose (similarly to the --help case) to add an 
`onMissingRequired()` "hoo

Re: Review Request 35157: Added unit tests for fetching ResoruceUsage in both QoS Controller and Resource Estimator .

[Bartek Plotka]

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

(Updated June 5, 2015, 11:53 p.m.)


Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, 
and Vinod Kone.


Changes
---

Added QoS Controller test.


Summary (updated)
-

Added unit tests for fetching ResoruceUsage in both QoS Controller and Resource 
Estimator .


Repository: mesos


Description (updated)
---

Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage 
from ResourceMonitor.

This is the unit test for https://reviews.apache.org/r/34980/ and 
https://reviews.apache.org/r/35164/


Diffs (updated)
-

  src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 
  src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a 
  src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 

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


Testing
---

make check


Thanks,

Bartek Plotka

Re: Review Request 35152: Document and consolidate qdisc handles

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35150, 35152]

All tests passed.

- Mesos ReviewBot


On June 5, 2015, 10:35 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35152/
> ---
> 
> (Updated June 5, 2015, 10:35 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Bugs: MESOS-2821
> https://issues.apache.org/jira/browse/MESOS-2821
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document and consolidate qdisc handles
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 871e9cf1625d96d1feef50edd4081972c097d191 
> 
> Diff: https://reviews.apache.org/r/35152/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Review Request 35164: Added callback to the QoS Controller to retrieve usages from the monitor.

[Bartek Plotka]

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

Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod 
Kone.


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


Repository: mesos


Description
---

Passed callback to the QoS Controller to retrieve ResourceUsage from Resource 
Monitor on demand.

This is neccessary, since QoS Controller needs data (current statistics for 
each executor) on which it will base its potential corrections.


Diffs
-

  include/mesos/slave/qos_controller.hpp 
1d89acfd9c742b044674e0a0815f9f01eccb69b3 
  src/slave/qos_controller.hpp b37798303561eb79aee202b9c110794517eeed06 
  src/slave/qos_controller.cpp 81c4b3e658902be0438f42d9e86911e424828a73 
  src/slave/slave.cpp 054929b156374a8929ac9fffb032045f13c3eb43 
  src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 

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


Testing
---

make check


Thanks,

Bartek Plotka

Re: Review Request 35150: Add output stream operation for handle to use in port_mapping.cpp

[Paul Brett]

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

(Updated June 5, 2015, 11:33 p.m.)


Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.


Changes
---

Incorporated reviewer feedback.


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


Repository: mesos


Description
---

Add output stream operation for handle to use in port_mapping.cpp


Diffs (updated)
-

  src/linux/routing/handle.hpp c107a7ef16f5060210b16b91e3a881af84ba6c10 

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


Testing
---

make check


Thanks,

Paul Brett

Re: Review Request 35131: Replaced os::dirname and os::basename with Path::dirname and Path::basename.

[Vinod Kone]

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

Ship it!


Ship It!

- Vinod Kone


On June 5, 2015, 1:20 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35131/
> ---
> 
> (Updated June 5, 2015, 1:20 p.m.)
> 
> 
> Review request for mesos and Cody Maloney.
> 
> 
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da 
> 
> Diff: https://reviews.apache.org/r/35131/diff/
> 
> 
> Testing
> ---
> 
> make check on trailing RR.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Vinod Kone]

> On June 5, 2015, 6:42 p.m., Vinod Kone wrote:
> > I made some minor comments below but I think a better way to do this is to 
> > *not* write custom masterinfo json <-> protobuf converters. I prefer we 
> > just add a new optional field (say ipAddress of type string). Then you can 
> > just leverage the already existing generic protobuf <-> json converters.
> 
> Niklas Nielsen wrote:
> We have been working on this for a while now and we are using 
> JSON::Protobuf() aldready and can enhance it further with your suggestion. 
> However, the current approach isn't semantically different from the one you 
> suggest and we would like to move forward and make that change later. Is that 
> OK with you?
> 
> Marco Massenzio wrote:
> Hey Vinod - as Niklas pointed out, we have invested a significant amount 
> of time on this one, including the manual testing I've done (as summarized on 
> MESOS-2304) and I'd be really reluctant to start again from scratch.
> 
> As I don't really think there is any semantic difference; my approach 
> does not introduce any performance penalty (in fact, I believe it may even be 
> better than a 'generic' one); and, finally, that this does not impact the 
> readability of the code, we should commit the code 'as is' (with your 
> suggestions below) and move on.
> 
> There are a ton of features and work to do, and I'd like us to focus on 
> important issues.
> 
> Vinod Kone wrote:
> Marco. I appreciate that you invested significant effort to making sure 
> the backwards compatibility story is airtight, but I urge you to spend some 
> extra cycles to simplify the code and avoid tech debt. I honestly don't think 
> it would take too much time to simplify this. I would really like to 
> understand what's the most time consuming part here? The compatibility 
> testing? Can you automate it with niklas's compatibility script?
> 
> As an aside, going through earlier reviews I saw that BenH made similar 
> comments but it got sidetracked with deprecating "ip". Also, my fault for not 
> jumping on this sooner, but in my defense, I wasn't marked as a reviewer. I 
> would be happy to shepherd this change for you going forward.
> 
> Marco Massenzio wrote:
> The upgrade/change of MasterInfo is tracked in 
> https://issues.apache.org/jira/browse/MESOS-2736
> which I believe is a different topic than what is addressed here (which 
> is tracked here: https://issues.apache.org/jira/browse/MESOS-2807 - I'll 
> update this patch description in a sec).
> 
> In any event, as `ip` is a required int32 field, we MUST support it, no 
> matter what - this patch does this, correctly, without introducing "tech 
> debt" (I don't see where I'm doing that).
> 
> Thanks in advance for your understanding.

My suggestion is to support current ip field by keeping it asis, i.e., let it 
be a number in json string. The reasoning is that, if we end up deciding that 
the easiest way to support ipv6 is by having a string field in the protobuf, 
then we would've 2 redundant string representations of the ip address in the 
resulting json.

The tech debt part i was referring to was that, now there is are a set of 
custom functions for protobuf <-> json conversions for this protobuf which need 
to be maintained (while model() is not bad, parse() is doing a lot of work and 
seems to have written without the knowledge that there is an existing generic 
parse function IIRC?). For example, if someone adds new fields to MasterInfo 
they have to come and update these functions. Not to mention, you have added a 
bunch of tests to test the custom parsing logic, which could likely be 
eliminated if you can reuse the existing generic functions.

Feel free to ping me on IRC if you want to discuss further.


- Vinod


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


On June 5, 2015, 8:18 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 5, 2015, 8:18 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2807
> https://issues.apache.org/jira/browse/MESOS-2807
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associa

Re: Review Request 35150: Add output stream operation for handle to use in port_mapping.cpp

[Jie Yu]

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

Ship it!



src/linux/routing/handle.hpp


2 lines apart.



src/linux/routing/handle.hpp


Ditto.


- Jie Yu


On June 5, 2015, 8:30 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35150/
> ---
> 
> (Updated June 5, 2015, 8:30 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Bugs: MESOS-2821
> https://issues.apache.org/jira/browse/MESOS-2821
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add output stream operation for handle to use in port_mapping.cpp
> 
> 
> Diffs
> -
> 
>   src/linux/routing/handle.hpp c107a7ef16f5060210b16b91e3a881af84ba6c10 
> 
> Diff: https://reviews.apache.org/r/35150/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 34894: Add new message for Traffic Control statistics

[Jie Yu]

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

Ship it!


LGTM. I'll wait for Ian's shipit.

- Jie Yu


On June 3, 2015, 9:54 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34894/
> ---
> 
> (Updated June 3, 2015, 9:54 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add new message for Traffic Control statistics
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 6186c92c6fe9cf8fa136d93d5af43e0377406baf 
> 
> Diff: https://reviews.apache.org/r/34894/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 35157: Added test for ResourceEstimator fetching ResoruceUsage from ResourceMonitor.

[Bartek Plotka]

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


Above ReviewBot message refers to the initial patch before ReviewRequest 
change! ):

- Bartek Plotka


On June 5, 2015, 10:52 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> ---
> 
> (Updated June 5, 2015, 10:52 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, 
> and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
> 
> This is the unit test for https://reviews.apache.org/r/34980/
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 
>   src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a 
>   src/tests/oversubscription_tests.cpp 
> afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 
> 
> Diff: https://reviews.apache.org/r/35157/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 35157: Added test for ResourceEstimator fetching ResoruceUsage from ResourceMonitor.

[Bartek Plotka]

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

(Updated June 5, 2015, 10:52 p.m.)


Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, 
and Vinod Kone.


Changes
---

Added forgotten files.


Repository: mesos


Description
---

Added test for ResourceEstimator fetching ResoruceUsage from ResourceMonitor.

This is the unit test for https://reviews.apache.org/r/34980/


Diffs (updated)
-

  src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 
  src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a 
  src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 

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


Testing
---

make check


Thanks,

Bartek Plotka

Re: Review Request 35152: Document and consolidate qdisc handles

[Chi Zhang]

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



src/slave/containerizer/isolators/network/port_mapping.cpp


Somewhere we should mention icmp and arp are mirrored, but ip traffic is 
redirected, which takes precedence than the former two in filtering.



src/slave/containerizer/isolators/network/port_mapping.cpp


Should be more clear that this is inside the container.



src/slave/containerizer/isolators/network/port_mapping.cpp


"reduce performance interference" seems a bit too general to me. Can we 
touch a bit on the specifics such as "each container's outbound traffic is 
given a separate flow, and also the catch-all flow for traffic originated from 
host and everything else", etc


- Chi Zhang


On June 5, 2015, 10:35 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35152/
> ---
> 
> (Updated June 5, 2015, 10:35 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Bugs: MESOS-2821
> https://issues.apache.org/jira/browse/MESOS-2821
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document and consolidate qdisc handles
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 871e9cf1625d96d1feef50edd4081972c097d191 
> 
> Diff: https://reviews.apache.org/r/35152/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 35152: Document and consolidate qdisc handles

[Paul Brett]

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

(Updated June 5, 2015, 10:35 p.m.)


Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.


Changes
---

Incorporate review comments.


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


Repository: mesos


Description
---

Document and consolidate qdisc handles


Diffs (updated)
-

  src/slave/containerizer/isolators/network/port_mapping.cpp 
871e9cf1625d96d1feef50edd4081972c097d191 

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


Testing
---

make check


Thanks,

Paul Brett

Re: Review Request 35119: Introduced metrics for revocable resources.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35118, 35119]

All tests passed.

- Mesos ReviewBot


On June 5, 2015, 9:13 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35119/
> ---
> 
> (Updated June 5, 2015, 9:13 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> state.json changes are in a subsequent review.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f 
>   src/master/master.cpp be0db42da3c59761aa154439653d715556465256 
>   src/master/metrics.hpp 833033c1912daee429b45423d24d365d8699a428 
>   src/master/metrics.cpp 264252c5159990fdf7a4569933a305d07bd7dd6e 
>   src/tests/oversubscription_tests.cpp 
> afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 
> 
> Diff: https://reviews.apache.org/r/35119/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> - Modified a test to test the `total` resources metrics.
> - We don't have unit tests that use the revocable resources yet, when we add 
> that we should check `used` resources metrics too.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 35119: Introduced metrics for revocable resources.

[Vinod Kone]

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

Ship it!



src/master/metrics.hpp


s/irrevocable/non-revocable/



src/master/metrics.cpp


ditto.


- Vinod Kone


On June 5, 2015, 9:13 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35119/
> ---
> 
> (Updated June 5, 2015, 9:13 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> state.json changes are in a subsequent review.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f 
>   src/master/master.cpp be0db42da3c59761aa154439653d715556465256 
>   src/master/metrics.hpp 833033c1912daee429b45423d24d365d8699a428 
>   src/master/metrics.cpp 264252c5159990fdf7a4569933a305d07bd7dd6e 
>   src/tests/oversubscription_tests.cpp 
> afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 
> 
> Diff: https://reviews.apache.org/r/35119/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> - Modified a test to test the `total` resources metrics.
> - We don't have unit tests that use the revocable resources yet, when we add 
> that we should check `used` resources metrics too.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 35118: Made updateSlave() update its 'totalResources'.

[Vinod Kone]

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



src/master/master.cpp


woah. didn't realize this was handled automagically by the install handler.



src/master/master.cpp


while you are at it, can you just send slave->totalResources here instead 
of oversubscribedResources? this will address my TODO on this method.


- Vinod Kone


On June 5, 2015, 9:09 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35118/
> ---
> 
> (Updated June 5, 2015, 9:09 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This way Master::Slave::totalResources includes revocable resources, which 
> we need for metrics for revocable resources.
> - Changed updateSlave() argument to use `const Resources& 
> oversubscribedResources` instead of `const std::vector& 
> oversubscribedResources` because `Resources` provides convenience methods 
> such as `revocable()`.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f 
>   src/master/master.cpp be0db42da3c59761aa154439653d715556465256 
> 
> Diff: https://reviews.apache.org/r/35118/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 34984: Added help for files

[Niklas Nielsen]

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

Ship it!


I will make the change for you below. Thanks Aditi!


src/files/files.cpp


Should be:

```
using process::DESCRIPTION;
using process::HELP;
using process::TLDR;
using process::USAGE;
using process::wait; // Necessary on some OS's to disambiguate.
```

Underneath 'using namespace process;' :)



src/files/files.cpp


s/Path/path/


- Niklas Nielsen


On June 4, 2015, 2:40 a.m., Aditi Dixit wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34984/
> ---
> 
> (Updated June 4, 2015, 2:40 a.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-2277
> https://issues.apache.org/jira/browse/MESOS-2277
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added help for files
> 
> 
> Diffs
> -
> 
>   src/files/files.cpp ce02411c5e579d7551b4325ec141fd89e4ee7255 
> 
> Diff: https://reviews.apache.org/r/34984/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Aditi Dixit
> 
>

Review Request 35157: Added test for ResourceEstimator fetching ResoruceUsage from ResourceMonitor.

[Bartek Plotka]

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

Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, 
and Vinod Kone.


Repository: mesos


Description
---

Added test for ResourceEstimator fetching ResoruceUsage from ResourceMonitor.

This is the unit test for https://reviews.apache.org/r/34980/


Diffs
-

  src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 

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


Testing
---

make check


Thanks,

Bartek Plotka

Re: Review Request 34970: Cleaned up and generalized NoExecutorScheduler to be more configurable.

[Vinod Kone]

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

Ship it!



src/examples/no_executor_framework.cpp


I think it is easier to follow if this is written as:

```
while(remaining.contains(taskResources) &&
  (totalTasks.isNone() || tasksLaunched < totalTasks.get())) {
  
}
```



src/examples/no_executor_framework.cpp


LOG(FATAL)? because this is not possible with a command executor?



src/examples/no_executor_framework.cpp


s/active/launched/ ?



src/examples/no_executor_framework.cpp


Also mention that, if this is not specified, as many tasks as possible are 
launched?



src/examples/no_executor_framework.cpp


Only set this if flags.task_revocable_resources.isSome() ?


- Vinod Kone


On June 5, 2015, 7:56 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34970/
> ---
> 
> (Updated June 5, 2015, 7:56 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-2655
> https://issues.apache.org/jira/browse/MESOS-2655
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now the NoExecutorScheduler can take a custom command, as well as the 
> resources for the task and how many tasks to run.
> 
> This allows us to continue using it for an end-to-end test as part of make 
> check, but also allows it to be used against a cluster in a long-lived 
> fashion.
> 
> 
> Diffs
> -
> 
>   src/examples/no_executor_framework.cpp 
> 1fb853d6e4a3deb3c36e6e661689697c8ebf898f 
>   src/tests/no_executor_framework_test.sh 
> d2d395595b778bc543e1baaa0fd415dc622b647f 
>   src/tests/script.cpp 515e3141e1d517da4cfc1421d5301d0a3bd6ad51 
> 
> Diff: https://reviews.apache.org/r/34970/diff/
> 
> 
> Testing
> ---
> 
> The existing no executor framework test picks this up.
> 
> Since we do not have an non-zero estimator yet, I modified the slave to send 
> revocable resources in order to manually test the revocable case.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 35152: Document and consolidate qdisc handles

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35150, 35152]

All tests passed.

- Mesos ReviewBot


On June 5, 2015, 8:38 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35152/
> ---
> 
> (Updated June 5, 2015, 8:38 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Bugs: MESOS-2821
> https://issues.apache.org/jira/browse/MESOS-2821
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document and consolidate qdisc handles
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 871e9cf1625d96d1feef50edd4081972c097d191 
> 
> Diff: https://reviews.apache.org/r/35152/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Review Request 35119: Introduced metrics for revocable resources.

[Jiang Yan Xu]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

state.json changes are in a subsequent review.


Diffs
-

  src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f 
  src/master/master.cpp be0db42da3c59761aa154439653d715556465256 
  src/master/metrics.hpp 833033c1912daee429b45423d24d365d8699a428 
  src/master/metrics.cpp 264252c5159990fdf7a4569933a305d07bd7dd6e 
  src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 

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


Testing
---

make check.

- Modified a test to test the `total` resources metrics.
- We don't have unit tests that use the revocable resources yet, when we add 
that we should check `used` resources metrics too.


Thanks,

Jiang Yan Xu

Re: Review Request 35152: Document and consolidate qdisc handles

[Cong Wang]

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



src/slave/containerizer/isolators/network/port_mapping.cpp


s/connection/interface/



src/slave/containerizer/isolators/network/port_mapping.cpp


s/data/packet/



src/slave/containerizer/isolators/network/port_mapping.cpp


s/policies/filters/



src/slave/containerizer/isolators/network/port_mapping.cpp


s/interface qdisc/qdisc/



src/slave/containerizer/isolators/network/port_mapping.cpp


Nit: there could be more than one filters since we could have more than one 
port ranges for each container.



src/slave/containerizer/isolators/network/port_mapping.cpp


What is "filter chain"?? There is no such thing in TC. Also we don't have 
any filter on htb.



src/slave/containerizer/isolators/network/port_mapping.cpp


Ditto, there is no filter here, just a class.



src/slave/containerizer/isolators/network/port_mapping.cpp


The fq_codel qdisc here is for reducing buffer-bloat, not traffic shaping.



src/slave/containerizer/isolators/network/port_mapping.cpp


s/FInally/Finally/



src/slave/containerizer/isolators/network/port_mapping.cpp


Ditto, the fq_codel here is for traffic isolation and reducing buffer-bloat.


- Cong Wang


On June 5, 2015, 8:38 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35152/
> ---
> 
> (Updated June 5, 2015, 8:38 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Bugs: MESOS-2821
> https://issues.apache.org/jira/browse/MESOS-2821
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document and consolidate qdisc handles
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 871e9cf1625d96d1feef50edd4081972c097d191 
> 
> Diff: https://reviews.apache.org/r/35152/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Review Request 35118: Made updateSlave() update its 'totalResources'.

[Jiang Yan Xu]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

- This way Master::Slave::totalResources includes revocable resources, which we 
need for metrics for revocable resources.
- Changed updateSlave() argument to use `const Resources& 
oversubscribedResources` instead of `const std::vector& 
oversubscribedResources` because `Resources` provides convenience methods such 
as `revocable()`.


Diffs
-

  src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f 
  src/master/master.cpp be0db42da3c59761aa154439653d715556465256 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 35150: Add output stream operation for handle to use in port_mapping.cpp

[Chi Zhang]

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

Ship it!


Ship It!

- Chi Zhang


On June 5, 2015, 8:30 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35150/
> ---
> 
> (Updated June 5, 2015, 8:30 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Bugs: MESOS-2821
> https://issues.apache.org/jira/browse/MESOS-2821
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add output stream operation for handle to use in port_mapping.cpp
> 
> 
> Diffs
> -
> 
>   src/linux/routing/handle.hpp c107a7ef16f5060210b16b91e3a881af84ba6c10 
> 
> Diff: https://reviews.apache.org/r/35150/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 34970: Cleaned up and generalized NoExecutorScheduler to be more configurable.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34968, 34969, 34970]

All tests passed.

- Mesos ReviewBot


On June 5, 2015, 7:56 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34970/
> ---
> 
> (Updated June 5, 2015, 7:56 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-2655
> https://issues.apache.org/jira/browse/MESOS-2655
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now the NoExecutorScheduler can take a custom command, as well as the 
> resources for the task and how many tasks to run.
> 
> This allows us to continue using it for an end-to-end test as part of make 
> check, but also allows it to be used against a cluster in a long-lived 
> fashion.
> 
> 
> Diffs
> -
> 
>   src/examples/no_executor_framework.cpp 
> 1fb853d6e4a3deb3c36e6e661689697c8ebf898f 
>   src/tests/no_executor_framework_test.sh 
> d2d395595b778bc543e1baaa0fd415dc622b647f 
>   src/tests/script.cpp 515e3141e1d517da4cfc1421d5301d0a3bd6ad51 
> 
> Diff: https://reviews.apache.org/r/34970/diff/
> 
> 
> Testing
> ---
> 
> The existing no executor framework test picks this up.
> 
> Since we do not have an non-zero estimator yet, I modified the slave to send 
> revocable resources in order to manually test the revocable case.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Review Request 35152: Document and consolidate qdisc handles

[Paul Brett]

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

Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.


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


Repository: mesos


Description
---

Document and consolidate qdisc handles


Diffs
-

  src/slave/containerizer/isolators/network/port_mapping.cpp 
871e9cf1625d96d1feef50edd4081972c097d191 

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


Testing
---

make check


Thanks,

Paul Brett

Review Request 35150: Add output stream operation for handle to use in port_mapping.cpp

[Paul Brett]

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

Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.


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


Repository: mesos


Description
---

Add output stream operation for handle to use in port_mapping.cpp


Diffs
-

  src/linux/routing/handle.hpp c107a7ef16f5060210b16b91e3a881af84ba6c10 

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


Testing
---

make check


Thanks,

Paul Brett

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

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

(Updated June 5, 2015, 8:18 p.m.)


Review request for mesos, haosdent huang and Niklas Nielsen.


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


Repository: mesos


Description
---

Jira: MESOS-2340

This is a preliminary step to enabling JSON API
for Master Discovery via Zookeeper.

We currently save the MasterInfo PB to ZK
serializing directly the binary data, so that
for clients to retrieve that information, they
need to either link up with libmesos or
obtain the PB definition (in mesos/mesos.proto).

This change only provides the (de)serialization
utility methods and associated tests.


Diffs
-

  src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
  src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
  src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
  src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
  src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
  src/tests/common/parse_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Marco Massenzio

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

> On June 5, 2015, 6:42 p.m., Vinod Kone wrote:
> > I made some minor comments below but I think a better way to do this is to 
> > *not* write custom masterinfo json <-> protobuf converters. I prefer we 
> > just add a new optional field (say ipAddress of type string). Then you can 
> > just leverage the already existing generic protobuf <-> json converters.
> 
> Niklas Nielsen wrote:
> We have been working on this for a while now and we are using 
> JSON::Protobuf() aldready and can enhance it further with your suggestion. 
> However, the current approach isn't semantically different from the one you 
> suggest and we would like to move forward and make that change later. Is that 
> OK with you?
> 
> Marco Massenzio wrote:
> Hey Vinod - as Niklas pointed out, we have invested a significant amount 
> of time on this one, including the manual testing I've done (as summarized on 
> MESOS-2304) and I'd be really reluctant to start again from scratch.
> 
> As I don't really think there is any semantic difference; my approach 
> does not introduce any performance penalty (in fact, I believe it may even be 
> better than a 'generic' one); and, finally, that this does not impact the 
> readability of the code, we should commit the code 'as is' (with your 
> suggestions below) and move on.
> 
> There are a ton of features and work to do, and I'd like us to focus on 
> important issues.
> 
> Vinod Kone wrote:
> Marco. I appreciate that you invested significant effort to making sure 
> the backwards compatibility story is airtight, but I urge you to spend some 
> extra cycles to simplify the code and avoid tech debt. I honestly don't think 
> it would take too much time to simplify this. I would really like to 
> understand what's the most time consuming part here? The compatibility 
> testing? Can you automate it with niklas's compatibility script?
> 
> As an aside, going through earlier reviews I saw that BenH made similar 
> comments but it got sidetracked with deprecating "ip". Also, my fault for not 
> jumping on this sooner, but in my defense, I wasn't marked as a reviewer. I 
> would be happy to shepherd this change for you going forward.

The upgrade/change of MasterInfo is tracked in 
https://issues.apache.org/jira/browse/MESOS-2736
which I believe is a different topic than what is addressed here (which is 
tracked here: https://issues.apache.org/jira/browse/MESOS-2807 - I'll update 
this patch description in a sec).

In any event, as `ip` is a required int32 field, we MUST support it, no matter 
what - this patch does this, correctly, without introducing "tech debt" (I 
don't see where I'm doing that).

Thanks in advance for your understanding.


- Marco


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


On June 3, 2015, 9 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 9 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Vinod Kone]

> On June 5, 2015, 6:42 p.m., Vinod Kone wrote:
> > I made some minor comments below but I think a better way to do this is to 
> > *not* write custom masterinfo json <-> protobuf converters. I prefer we 
> > just add a new optional field (say ipAddress of type string). Then you can 
> > just leverage the already existing generic protobuf <-> json converters.
> 
> Niklas Nielsen wrote:
> We have been working on this for a while now and we are using 
> JSON::Protobuf() aldready and can enhance it further with your suggestion. 
> However, the current approach isn't semantically different from the one you 
> suggest and we would like to move forward and make that change later. Is that 
> OK with you?
> 
> Marco Massenzio wrote:
> Hey Vinod - as Niklas pointed out, we have invested a significant amount 
> of time on this one, including the manual testing I've done (as summarized on 
> MESOS-2304) and I'd be really reluctant to start again from scratch.
> 
> As I don't really think there is any semantic difference; my approach 
> does not introduce any performance penalty (in fact, I believe it may even be 
> better than a 'generic' one); and, finally, that this does not impact the 
> readability of the code, we should commit the code 'as is' (with your 
> suggestions below) and move on.
> 
> There are a ton of features and work to do, and I'd like us to focus on 
> important issues.

Marco. I appreciate that you invested significant effort to making sure the 
backwards compatibility story is airtight, but I urge you to spend some extra 
cycles to simplify the code and avoid tech debt. I honestly don't think it 
would take too much time to simplify this. I would really like to understand 
what's the most time consuming part here? The compatibility testing? Can you 
automate it with niklas's compatibility script?

As an aside, going through earlier reviews I saw that BenH made similar 
comments but it got sidetracked with deprecating "ip". Also, my fault for not 
jumping on this sooner, but in my defense, I wasn't marked as a reviewer. I 
would be happy to shepherd this change for you going forward.


- Vinod


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


On June 3, 2015, 9 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 9 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34970: Cleaned up and generalized NoExecutorScheduler to be more configurable.

[Ben Mahler]

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

(Updated June 5, 2015, 7:56 p.m.)


Review request for mesos, Jie Yu and Vinod Kone.


Changes
---

Resolved merge conflicts for the os::getenv changes that landed.


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


Repository: mesos


Description
---

Now the NoExecutorScheduler can take a custom command, as well as the resources 
for the task and how many tasks to run.

This allows us to continue using it for an end-to-end test as part of make 
check, but also allows it to be used against a cluster in a long-lived fashion.


Diffs (updated)
-

  src/examples/no_executor_framework.cpp 
1fb853d6e4a3deb3c36e6e661689697c8ebf898f 
  src/tests/no_executor_framework_test.sh 
d2d395595b778bc543e1baaa0fd415dc622b647f 
  src/tests/script.cpp 515e3141e1d517da4cfc1421d5301d0a3bd6ad51 

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


Testing
---

The existing no executor framework test picks this up.

Since we do not have an non-zero estimator yet, I modified the slave to send 
revocable resources in order to manually test the revocable case.


Thanks,

Ben Mahler

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

> On June 5, 2015, 6:42 p.m., Vinod Kone wrote:
> > I made some minor comments below but I think a better way to do this is to 
> > *not* write custom masterinfo json <-> protobuf converters. I prefer we 
> > just add a new optional field (say ipAddress of type string). Then you can 
> > just leverage the already existing generic protobuf <-> json converters.
> 
> Niklas Nielsen wrote:
> We have been working on this for a while now and we are using 
> JSON::Protobuf() aldready and can enhance it further with your suggestion. 
> However, the current approach isn't semantically different from the one you 
> suggest and we would like to move forward and make that change later. Is that 
> OK with you?

Hey Vinod - as Niklas pointed out, we have invested a significant amount of 
time on this one, including the manual testing I've done (as summarized on 
MESOS-2304) and I'd be really reluctant to start again from scratch.

As I don't really think there is any semantic difference; my approach does not 
introduce any performance penalty (in fact, I believe it may even be better 
than a 'generic' one); and, finally, that this does not impact the readability 
of the code, we should commit the code 'as is' (with your suggestions below) 
and move on.

There are a ton of features and work to do, and I'd like us to focus on 
important issues.


- Marco


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


On June 3, 2015, 9 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 9 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34970: Cleaned up and generalized NoExecutorScheduler to be more configurable.

[Ben Mahler]

> On June 5, 2015, 6:24 a.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [34968, 34969, 34970]
> > 
> > Failed command: ./support/apply-review.sh -n -r 34970
> > 
> > Error:
> >  2015-06-05 06:24:06 URL:https://reviews.apache.org/r/34970/diff/raw/ 
> > [16370/16370] -> "34970.patch" [1]
> > error: patch failed: src/examples/no_executor_framework.cpp:16
> > error: src/examples/no_executor_framework.cpp: patch does not apply
> > Failed to apply patch

Interesting, this shouldn't have applied the previous two reviews since 
[MESOS-1881](https://issues.apache.org/jira/browse/MESOS-1881) is resolved.


- Ben


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


On June 5, 2015, 5:52 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34970/
> ---
> 
> (Updated June 5, 2015, 5:52 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-2655
> https://issues.apache.org/jira/browse/MESOS-2655
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now the NoExecutorScheduler can take a custom command, as well as the 
> resources for the task and how many tasks to run.
> 
> This allows us to continue using it for an end-to-end test as part of make 
> check, but also allows it to be used against a cluster in a long-lived 
> fashion.
> 
> 
> Diffs
> -
> 
>   src/examples/no_executor_framework.cpp 
> 37001c389f31f9f1dafe6d7f3eb17adc2e369057 
>   src/tests/no_executor_framework_test.sh 
> d2d395595b778bc543e1baaa0fd415dc622b647f 
>   src/tests/script.cpp 515e3141e1d517da4cfc1421d5301d0a3bd6ad51 
> 
> Diff: https://reviews.apache.org/r/34970/diff/
> 
> 
> Testing
> ---
> 
> The existing no executor framework test picks this up.
> 
> Since we do not have an non-zero estimator yet, I modified the slave to send 
> revocable resources in order to manually test the revocable case.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Niklas Nielsen]

> On June 5, 2015, 11:42 a.m., Vinod Kone wrote:
> > I made some minor comments below but I think a better way to do this is to 
> > *not* write custom masterinfo json <-> protobuf converters. I prefer we 
> > just add a new optional field (say ipAddress of type string). Then you can 
> > just leverage the already existing generic protobuf <-> json converters.

We have been working on this for a while now and we are using JSON::Protobuf() 
aldready and can enhance it further with your suggestion. However, the current 
approach isn't semantically different from the one you suggest and we would 
like to move forward and make that change later. Is that OK with you?


- Niklas


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


On June 3, 2015, 2 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 2 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Niklas Nielsen]

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



src/common/http.hpp


s/info/masterInfo/



src/common/http.cpp


Unfold to:

```
/**
 * Foobar
 */
```



src/common/parse.hpp


Should this block have 2 space indent or 1? You have 1 above :)



src/tests/common/parse_tests.cpp


ASSERT_EQ() on the master infos instead of SerializeAsString()?


- Niklas Nielsen


On June 3, 2015, 2 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 2 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Niklas Nielsen]

> On June 5, 2015, 11:42 a.m., Vinod Kone wrote:
> > src/common/http.cpp, line 212
> > 
> >
> > Why do you need this temporary?

BenH brought this up - look above. I think this is fine if it is made const.


- Niklas


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


On June 3, 2015, 2 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 2 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Vinod Kone]

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


I made some minor comments below but I think a better way to do this is to 
*not* write custom masterinfo json <-> protobuf converters. I prefer we just 
add a new optional field (say ipAddress of type string). Then you can just 
leverage the already existing generic protobuf <-> json converters.


src/common/http.cpp


Why do you need this temporary?



src/common/parse.hpp


s/error out/return an error/



src/common/parse.hpp


s/result/info/



src/common/parse.hpp


"ips" sounds like multiple IPs though i know you meant it as IP as a string.

just call it "ipString"



src/common/parse.hpp


s/this/This/


- Vinod Kone


On June 3, 2015, 9 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 9 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 33159: Pump updateFramework through Allocator from Master.

[Vinod Kone]

> On June 5, 2015, 5:42 p.m., Vinod Kone wrote:
> > src/master/allocator/mesos/hierarchical.hpp, lines 455-456
> > 
> >
> > I don't follow. Why are these CHECKs? Is there currently code in the 
> > master that guarantees that these checks won't fail?

actually nm. looks like this is being called with framework->info in the master 
and not with the frameworkinfo that a framework re-registers with.


- Vinod


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


On June 5, 2015, 1:35 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33159/
> ---
> 
> (Updated June 5, 2015, 1:35 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-2615 and MESOS-703
> https://issues.apache.org/jira/browse/MESOS-2615
> https://issues.apache.org/jira/browse/MESOS-703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up of 32961 where we add the 'updateFramework' function to the 
> allocator.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8347cfaf6abce155a777484970e595e6b141bb87 
>   src/master/allocator/mesos/allocator.hpp 
> b57b03daf992082ec7c73199ab2574bf7993 
>   src/master/allocator/mesos/hierarchical.hpp 
> 44fbccaf72b65251095f69b68627d0efa58707d4 
>   src/master/master.cpp be0db42da3c59761aa154439653d715556465256 
>   src/tests/mesos.hpp 86660ac4d7476bc6cc077fc2d474c4a9ac81c031 
> 
> Diff: https://reviews.apache.org/r/33159/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 33159: Pump updateFramework through Allocator from Master.

[Vinod Kone]

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



src/master/allocator/mesos/hierarchical.hpp


I don't follow. Why are these CHECKs? Is there currently code in the master 
that guarantees that these checks won't fail?


- Vinod Kone


On June 5, 2015, 1:35 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33159/
> ---
> 
> (Updated June 5, 2015, 1:35 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-2615 and MESOS-703
> https://issues.apache.org/jira/browse/MESOS-2615
> https://issues.apache.org/jira/browse/MESOS-703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up of 32961 where we add the 'updateFramework' function to the 
> allocator.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8347cfaf6abce155a777484970e595e6b141bb87 
>   src/master/allocator/mesos/allocator.hpp 
> b57b03daf992082ec7c73199ab2574bf7993 
>   src/master/allocator/mesos/hierarchical.hpp 
> 44fbccaf72b65251095f69b68627d0efa58707d4 
>   src/master/master.cpp be0db42da3c59761aa154439653d715556465256 
>   src/tests/mesos.hpp 86660ac4d7476bc6cc077fc2d474c4a9ac81c031 
> 
> Diff: https://reviews.apache.org/r/33159/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 34258: Removed os::dirname and os::basename.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [34256, 35131, 34259, 34260]

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

Error:
 2015-06-05 16:47:17 URL:https://reviews.apache.org/r/34260/diff/raw/ 
[23611/23611] -> "34260.patch" [1]
error: patch failed: src/cli/mesos.cpp:28
error: src/cli/mesos.cpp: patch does not apply
error: patch failed: src/examples/low_level_scheduler_libprocess.cpp:345
error: src/examples/low_level_scheduler_libprocess.cpp: patch does not apply
error: patch failed: src/examples/low_level_scheduler_pthread.cpp:403
error: src/examples/low_level_scheduler_pthread.cpp: patch does not apply
error: patch failed: src/examples/test_framework.cpp:32
error: src/examples/test_framework.cpp: patch does not apply
error: patch failed: src/launcher/executor.cpp:633
error: src/launcher/executor.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On June 5, 2015, 4:39 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34258/
> ---
> 
> (Updated June 5, 2015, 4:39 p.m.)
> 
> 
> Review request for mesos and Cody Maloney.
> 
> 
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> os::dirname and os::basename were not thread safe on OSX. Replacements are 
> path::dirname and path::basename.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/README.md 588f739 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp d2ca4be 
> 
> Diff: https://reviews.apache.org/r/34258/diff/
> 
> 
> Testing
> ---
> 
> make check on trailing RR.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 34258: Removed os::dirname and os::basename.

[Till Toenshoff]

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

(Updated June 5, 2015, 4:39 p.m.)


Review request for mesos and Cody Maloney.


Changes
---

re-triggering the buildbot


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


Repository: mesos-incubating


Description
---

os::dirname and os::basename were not thread safe on OSX. Replacements are 
path::dirname and path::basename.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/README.md 588f739 
  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp d2ca4be 

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


Testing
---

make check on trailing RR.


Thanks,

Till Toenshoff

Re: Review Request 35129: Refactor Future::Data to use Result. Remove dynamic allocation.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35129]

All tests passed.

- Mesos ReviewBot


On June 5, 2015, 1:16 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35129/
> ---
> 
> (Updated June 5, 2015, 1:16 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Michael Park.
> 
> 
> Bugs: MESOS-2801
> https://issues.apache.org/jira/browse/MESOS-2801
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> 75cbe122f18d2a8ea919331e7fc91e1ec3bfd28b 
> 
> Diff: https://reviews.apache.org/r/35129/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 33295: Added firewall mechanism to control access on libprocess http endpoints.

[Till Toenshoff]

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

Ship it!



3rdparty/libprocess/include/process/firewall.hpp


First of all, sorry for leading you in the wrong direction here - but could 
you please start the description with a capital letter here and everywhere else 
as it has just decided and documented in our styleguides?



3rdparty/libprocess/include/process/firewall.hpp


s/requiered/required/



3rdparty/libprocess/include/process/firewall.hpp


s/considering/considered/?



3rdparty/libprocess/src/process.cpp


I see that you copied this from the above but once you get rid of the 
duplication, could you also make sure that this comment really adds value (or 
gets killed)?



3rdparty/libprocess/src/process.cpp


Let make this a std::move.



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


Could you move this comment three lines down before the actual request?



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


Could you please adjust the style of this comment towards those above?



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


s/a null pointer/an empty vector/


- Till Toenshoff


On June 2, 2015, 9:57 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33295/
> ---
> 
> (Updated June 2, 2015, 9:57 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2620
> https://issues.apache.org/jira/browse/MESOS-2620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces the interface `FirewallRule` which will be matched against 
> incoming connections in order to allow them to be served or being blocked. 
> For details, check the [design 
> doc](https://docs.google.com/document/d/1JSJTJMJ6ZXLkCSmvOIabTLrjtqqr0E-u99Rx2BHR1hs/edit).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/Makefile.am 
> 8aab0593f296c7aae71289f9bd6cf3eb3578a721 
>   3rdparty/libprocess/include/process/firewall.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/process.hpp 
> 79d1719932a3fdc90b6247d3a77adee123e72435 
>   3rdparty/libprocess/src/process.cpp 
> e3de3cd6b536aaaf59784360aed546512dd04dc9 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 67e582cc250a9767a389e2bd0cc68985477f3ffb 
> 
> Diff: https://reviews.apache.org/r/33295/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 34944: Used flags validation to handle --help.

[Benjamin Hindman]

> On June 2, 2015, 3:54 p.m., Marco Massenzio wrote:
> > Sweet!
> > 
> > Only comment is that this does not give the client of FlagsBase an option 
> > as to the behavior of --help (ie, it prints and exit: like it or lump it :).
> > 
> > Which is perfectly fine, IMO, but was wondering whether this was too 
> > restrictive?
> > (I haven't seen any other pattern in the 10+ refactorings I've done - so 
> > this should be just fine)

Yeah, I'm not convinced this is the way we want to do this, more that it's an 
easy MVP with a flags validator (which motivated me to dust off my old flags 
validation patch). I'm going to discard this review for now while someone 
actually puts a little more thought into this.


- Benjamin


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


On June 2, 2015, 2:46 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34944/
> ---
> 
> (Updated June 2, 2015, 2:46 p.m.)
> 
> 
> Review request for mesos and Marco Massenzio.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is one possible way of handling processing --help. Also, this is missing 
> a test.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> 61a405f225d14acbc38a80d35570426cb05a3d0a 
> 
> Diff: https://reviews.apache.org/r/34944/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 34943: Added validation to flags.

[Benjamin Hindman]

> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 514-517
> > 
> >
> > I'll admit I'm still a bith hazy about the new { } initializer, but 
> > could have this been:
> > ```
> > char* argv[] = { (char*) "/path/to/program",
> >  (char*) "--name1=billy joel" };
> > ```
> 
> Benjamin Hindman wrote:
> I just copied this code. Let me give this a test and if so I'll cleanup 
> the others too.

This is definitely cleaner and we should do it this way going forward so I'm 
going drop it here and follow up with a subsequent review that takes care of 
this, thanks for pointing it out!


- Benjamin


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


On June 5, 2015, 2:48 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> ---
> 
> (Updated June 5, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, 
> enabling us to get rid of our "loader" and "stringifier" functors.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 
> 87606d860dea3235ec70d127d3379065d42dc90b 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> ee855da6128c2d671eb2fc7eaa2c0aab2341aebb 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 
> 51d3ab020bd2bae1f12b66cac0da5383c813d5d7 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp 
> fda5ae1328a23a4371475652f921341dce7448d5 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> 80450185f60c5b273face490e0bb9e695b0cb984 
> 
> Diff: https://reviews.apache.org/r/34943/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 34943: Added validation to flags.

[Benjamin Hindman]

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

(Updated June 5, 2015, 2:48 p.m.)


Review request for mesos.


Repository: mesos


Description (updated)
---

Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, 
enabling us to get rid of our "loader" and "stringifier" functors.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
6ac2f04a6a1d8db1725972a3b4b46a0dd734566f 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 
87606d860dea3235ec70d127d3379065d42dc90b 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
ee855da6128c2d671eb2fc7eaa2c0aab2341aebb 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 
51d3ab020bd2bae1f12b66cac0da5383c813d5d7 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp 
fda5ae1328a23a4371475652f921341dce7448d5 
  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
80450185f60c5b273face490e0bb9e695b0cb984 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 34943: Added validation to flags.

[Benjamin Hindman]

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

(Updated June 5, 2015, 2:48 p.m.)


Review request for mesos.


Changes
---

Addressed Marco's comments.

Also C++11 lambda'fied the remaining Flag functions currently implemented via 
functors.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
6ac2f04a6a1d8db1725972a3b4b46a0dd734566f 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 
87606d860dea3235ec70d127d3379065d42dc90b 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
ee855da6128c2d671eb2fc7eaa2c0aab2341aebb 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 
51d3ab020bd2bae1f12b66cac0da5383c813d5d7 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp 
fda5ae1328a23a4371475652f921341dce7448d5 
  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
80450185f60c5b273face490e0bb9e695b0cb984 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 35125: Fixed verb naming of Flag::loader to Flag::load.

[Benjamin Hindman]

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

(Updated June 5, 2015, 2:21 p.m.)


Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 
87606d860dea3235ec70d127d3379065d42dc90b 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
ee855da6128c2d671eb2fc7eaa2c0aab2341aebb 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 33159: Pump updateFramework through Allocator from Master.

[Joris Van Remoortere]

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

(Updated June 5, 2015, 1:35 p.m.)


Review request for mesos and Benjamin Hindman.


Changes
---

switched CHECK to CHECK_EQ


Bugs: MESOS-2615 and MESOS-703
https://issues.apache.org/jira/browse/MESOS-2615
https://issues.apache.org/jira/browse/MESOS-703


Repository: mesos


Description
---

Follow up of 32961 where we add the 'updateFramework' function to the allocator.


Diffs (updated)
-

  include/mesos/master/allocator.hpp 8347cfaf6abce155a777484970e595e6b141bb87 
  src/master/allocator/mesos/allocator.hpp 
b57b03daf992082ec7c73199ab2574bf7993 
  src/master/allocator/mesos/hierarchical.hpp 
44fbccaf72b65251095f69b68627d0efa58707d4 
  src/master/master.cpp be0db42da3c59761aa154439653d715556465256 
  src/tests/mesos.hpp 86660ac4d7476bc6cc077fc2d474c4a9ac81c031 

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


Testing
---

make check


Thanks,

Joris Van Remoortere

Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Till Toenshoff]

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

(Updated June 5, 2015, 1:31 p.m.)


Review request for mesos and Cody Maloney.


Changes
---

Removed some include artefact.


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


Repository: mesos-incubating


Description
---

Introducing Path::dirname() and Path::basename() as a thread safe replacement 
of the respective system functions. Also contains new tests covering corner 
cases.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650 
  3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412 

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


Testing
---

make check (including new tests).

Result comparison to match ::dirname and ::basename on interesting cases.


Thanks,

Till Toenshoff

Re: Review Request 34260: Replaced os::dirname and os::basename with Path::dirname and Path::basename.

[Till Toenshoff]

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

(Updated June 5, 2015, 1:26 p.m.)


Review request for mesos and Cody Maloney.


Changes
---

Rebased.


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


Repository: mesos-incubating


Description
---

see summary.


Diffs (updated)
-

  src/cli/mesos.cpp 1121e19 
  src/cli/resolve.cpp 74545a0 
  src/examples/low_level_scheduler_libprocess.cpp df92e8d 
  src/examples/low_level_scheduler_pthread.cpp 175ee4d 
  src/examples/persistent_volume_framework.cpp ee2311f 
  src/examples/test_framework.cpp 25f5f8c 
  src/files/files.cpp ce02411 
  src/health-check/main.cpp 3607479 
  src/launcher/executor.cpp f79dc60 
  src/linux/cgroups.cpp 831237b 
  src/local/main.cpp ec21ed0 
  src/logging/logging.cpp 6b14575 
  src/slave/containerizer/fetcher.cpp f77652b 
  src/slave/containerizer/isolators/cgroups/cpushare.cpp 5bd3525 
  src/slave/containerizer/isolators/cgroups/mem.cpp 9647e79 
  src/slave/containerizer/isolators/cgroups/perf_event.cpp 3e5153f 
  src/slave/containerizer/isolators/network/port_mapping.cpp 871e9cf 
  src/slave/containerizer/linux_launcher.cpp 8eae258 
  src/slave/main.cpp c4d8940 
  src/slave/state.hpp fed4b7e 
  src/slave/state.cpp 8eda22a 
  src/slave/status_update_manager.cpp 1d7c4d0 
  src/tests/fetcher_tests.cpp 361d918 
  src/tests/mesos.cpp d3a8bb7 
  src/zookeeper/group.cpp 173caa8 

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


Testing
---

make check


Thanks,

Till Toenshoff

Re: Review Request 34943: Added validation to flags.

[Benjamin Hindman]

> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 141
> > 
> >
> > can you please add some documentation to explain what each type (and 
> > the relative @param) is?
> > (using Doxygen would be awesome :)

Most definitely, but I'll do that in another review.


> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 146
> > 
> >
> > How do we enforce that F is a function, and not, say, a std::string?

It gets enforced by using 'F'as a function in the body of 'add'. Bottom line, 
we take any functor here, if we can invoke the apply operator than we're good 
to go.


> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 184
> > 
> >
> > too much choice, IMO - there are (if I counted them right) 8 different 
> > `add()` variants to choose from (none of them documented :)
> > 
> > It's virtually impossible for folks to figure out which to pick and 
> > this leads, I believe, to the current 'code by copying' approach.
> > 
> > I would cull it down to no more than 2-3 different options, with some 
> > sensible default values.

I completely agree this needs documentation, which I'll take on in a different 
review. But there are really only 4 variants here:

(1) The flag _is_ a class member.
(A) The flag has a default value (i.e., is _not_ an Option).
(B) The flag does not have a default value (i.e., _is_ an Option).

(2) The flag is _not_ a class member.
(A) The flag has a default value (i.e., is _not_ an Option).
(B) The flag does not have a default value (i.e., _is_ an Option).

All 4 of these options can optionally have a validation function, but since we 
can't capture the default validation function with the existing 4 functions we 
have to do the delegating function trick, resulting in 8 functions. Bottom 
line, this needs to be documented (and if someone has a better suggestion to 
avoid the delgating function trick I'm all ears).


> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 500-504
> > 
> >
> > so, with your approach, everyone, every time that they add a required 
> > flag, will have to, essentially, copy & paste this code.
> > 
> > what I had envisioned, was a simpler way (eg, by adding a `bool 
> > required = false` arg) that, if true would automatically do this validation 
> > step in `FlagsBase`
> > 
> > (of course, we could add another `add()` method that has that 
> > signature, that just internally implements this and calls one of the other 
> > `add()` ones - making it the ninth option :) )

I wasn't trying to solve the "required" flag problem, I was just trying to show 
an example of a validation function. But I violently agree that this is 
therefore a horribly bad example that sets a bad precedent, so I've replaced 
this with a different validation function.

I also agree that we should introduce a simplier way, such as a bool as you 
suggested (or better an an enumeration). This doesn't completley solve the 
problem, however, since a flag without a default value will still be an Option 
and still require one to do 'CHECK_SOME(flags.flag)'. Got any other ideas?


> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 514-517
> > 
> >
> > I'll admit I'm still a bith hazy about the new { } initializer, but 
> > could have this been:
> > ```
> > char* argv[] = { (char*) "/path/to/program",
> >  (char*) "--name1=billy joel" };
> > ```

I just copied this code. Let me give this a test and if so I'll cleanup the 
others too.


- Benjamin


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


On June 2, 2015, 2:43 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> ---
> 
> (Updated June 2, 2015, 2:43 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp

Re: Review Request 34259: Replaced os::dirname and os::basename with Path::dirname and Path::basename.

[Till Toenshoff]

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

(Updated June 5, 2015, 1:22 p.m.)


Review request for mesos and Cody Maloney.


Changes
---

rebased


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


Repository: mesos-incubating


Description
---

see summary.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp f1d3e5b 

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


Testing
---

make check on trailing RR.


Thanks,

Till Toenshoff

Review Request 35131: Replaced os::dirname and os::basename with Path::dirname and Path::basename.

[Till Toenshoff]

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

Review request for mesos and Cody Maloney.


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


Repository: mesos-incubating


Description
---

see summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da 

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


Testing
---

make check on trailing RR.


Thanks,

Till Toenshoff

Review Request 35129: Refactor Future::Data to use Result. Remove dynamic allocation.

[Joris Van Remoortere]

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

Review request for mesos, Benjamin Hindman and Michael Park.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/include/process/future.hpp 
75cbe122f18d2a8ea919331e7fc91e1ec3bfd28b 

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


Testing
---

make check


Thanks,

Joris Van Remoortere

Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Till Toenshoff]

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

(Updated June 5, 2015, 1:03 p.m.)


Review request for mesos and Cody Maloney.


Changes
---

Some simplification, added more comments and did a rebase.


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


Repository: mesos-incubating


Description
---

Introducing Path::dirname() and Path::basename() as a thread safe replacement 
of the respective system functions. Also contains new tests covering corner 
cases.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650 
  3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412 

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


Testing
---

make check (including new tests).

Result comparison to match ::dirname and ::basename on interesting cases.


Thanks,

Till Toenshoff

Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Till Toenshoff]

> On May 19, 2015, 8:07 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp, line 29
> > 
> >
> > nice tests.

Can I drop this issue :)?


> On May 19, 2015, 8:07 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, lines 32-90
> > 
> >
> > Can you add comments to the code here? It is really hard to follow 
> > what's happening.
> > 
> > Alternatively, can you simplify these using "strings" functions?
> > 
> > For example, looking at http://linux.die.net/man/3/basename 
> > ```
> > The functions dirname() and basename() break a null-terminated pathname 
> > string into directory and filename components. In the usual case, dirname() 
> > returns the string up to, but not including, the final '/', and basename() 
> > returns the component following the final '/'. Trailing '/' characters are 
> > not counted as part of the pathname.
> > 
> > If path does not contain a slash, dirname() returns the string "." 
> > while basename() returns a copy of path. If path is the string "/", then 
> > both dirname() and basename() return the string "/". If path is a NULL 
> > pointer or points to an empty string, then both dirname() and basename() 
> > return the string ".".
> > ```
> > 
> > this is how I would implement basename
> > 
> > ```
> > string basename() 
> > {
> >// Remove trailing "/", if exists.
> >string result = strings::remove(value, "/", strings::SUFFIX);
> >
> >if (result.empty()) {
> >  return string(".");
> >}
> >
> >if (!strings::contains(result, "/")) {
> >  return result;
> >}
> >
> >if (result == "/") {
> >  return result;
> >}
> >
> >vector tokens = strings::tokenize(result, "/");
> >return tokens[tokens.size() - 1];
> > }
> > ```
> > 
> > I haven't checked all the edge cases, but you get the idea.

I thought a while about how we could use strings::remove but unfortunately I 
did not see a good way to make sure that e.g. multiple trailing slashes are 
properly cut off. In the end I decided that using an indexed based parsing 
would be more efficient and by the added comments also readable fine - at least 
that is what I think :)


- Till


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


On May 17, 2015, 7:46 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> ---
> 
> (Updated May 17, 2015, 7:46 p.m.)
> 
> 
> Review request for mesos and Cody Maloney.
> 
> 
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Introducing Path::dirname() and Path::basename() as a thread safe replacement 
> of the respective system functions. Also contains new tests covering corner 
> cases.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412 
> 
> Diff: https://reviews.apache.org/r/34256/diff/
> 
> 
> Testing
> ---
> 
> make check (including new tests).
> 
> Result comparison to match ::dirname and ::basename on interesting cases.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Review Request 35127: Updated libprocess to match stout Flags changes.

[Benjamin Hindman]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/src/subprocess.cpp 
f41f5e2a34788e31749eb996c8ab38ea45989068 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 35125: Fixed verb naming of Flag::loader to Flag::load.

[Benjamin Hindman]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 
87606d860dea3235ec70d127d3379065d42dc90b 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
ee855da6128c2d671eb2fc7eaa2c0aab2341aebb 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 35128: Updated Mesos to match stout Flags changes.

[Benjamin Hindman]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/http.cpp f8ac30934352db859e73819e0656a70047bb0dc5 
  src/slave/http.cpp bc25bdd33277dbfa30410ad081ea09f0fc39c598 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 35126: Used C++11 lambdas instead of functors in FlagsBase.

[Benjamin Hindman]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
6ac2f04a6a1d8db1725972a3b4b46a0dd734566f 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 
87606d860dea3235ec70d127d3379065d42dc90b 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
ee855da6128c2d671eb2fc7eaa2c0aab2341aebb 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 
51d3ab020bd2bae1f12b66cac0da5383c813d5d7 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp 
fda5ae1328a23a4371475652f921341dce7448d5 
  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
80450185f60c5b273face490e0bb9e695b0cb984 
  3rdparty/libprocess/3rdparty/stout/tests/subcommand_tests.cpp 
c40bba4f1e7eef7cb04f79b567e32684648b2004 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 33271: Update style guide to disallow capturing temporaries by reference.

[Joris Van Remoortere]

Here is the review. I used std::string instead. Also cleaned up an unused
struct.
https://reviews.apache.org/r/35120

On Fri, Jun 5, 2015 at 9:58 AM, Joris Van Remoortere <
joris.van.remoort...@gmail.com> wrote:

> Hey BenM,
>
> BenH added spaces before the commit, but thanks for also catching it!
> I'll follow up with a small patch to change the POD to something like
> SlaveID.
>
> Can you shepherd that one? :-)
>
> Joris
>
> On Fri, Jun 5, 2015 at 8:03 AM, Ben Mahler 
> wrote:
>
>>This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/33271/
>>
>> Thanks guys!
>>
>>
>>docs/mesos-c++-style-guide.md
>>  
>> (Diff
>> revision 9)
>>
>> 180
>>
>> foreachpair(const int& key, hashset& values, index) {}
>>
>>   181
>>
>> foreachvalue(const hashset& values, index) {}
>>
>>   182
>>
>> foreachkey(const int& key, index) {}
>>
>>   Do we really want to encourage taking a const& of a POD type? In general 
>> we have not been doing this, so it seems pretty inconsistent to put it in 
>> this example.
>>
>> Also, looks like we need a space before the openening parenthesis.
>>
>>
>> - Ben Mahler
>>
>> On June 2nd, 2015, 9:34 a.m. UTC, Joris Van Remoortere wrote:
>>   Review request for mesos, Bernd Mathiske, Cody Maloney, Joerg Schad,
>> Michael Park, and Till Toenshoff.
>> By Joris Van Remoortere.
>>
>> *Updated June 2, 2015, 9:34 a.m.*
>>  *Bugs: * MESOS-2629 
>>  *Repository: * mesos
>> Description
>>
>> Follow up from r32630.
>>
>>   Diffs
>>
>>- docs/mesos-c++-style-guide.md
>>(13312f6f4fe1788791479bd768f60df0a8e80e69)
>>
>> View Diff 
>>
>
>

Re: Review Request 35000: Doxygen'ized Subprocess.

[Till Toenshoff]

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

Ship it!



3rdparty/libprocess/include/process/subprocess.hpp


How about starting both enumerations with lower case here to make them part 
of the same sentence?



3rdparty/libprocess/include/process/subprocess.hpp


This leading sentence can exist without the colon and hence the following 
enumerations could use capitalized sentence starts.

I would replace the colon with a period to enable full sentences below.


- Till Toenshoff


On June 3, 2015, 1:45 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35000/
> ---
> 
> (Updated June 3, 2015, 1:45 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> 37cab7755d2890619b64e1ca09e0b7ad0e72cf76 
> 
> Diff: https://reviews.apache.org/r/35000/diff/
> 
> 
> Testing
> ---
> 
> make check
> doxygen ../Doxyfile
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 35120: Use non-POD type for alias example in c++ style guide.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35120]

All tests passed.

- Mesos ReviewBot


On June 5, 2015, 9:34 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35120/
> ---
> 
> (Updated June 5, 2015, 9:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also got rid of an unused struct.
> 
> 
> Diffs
> -
> 
>   docs/mesos-c++-style-guide.md 38dd201a22dd775971fad190378d022c50885969 
> 
> Diff: https://reviews.apache.org/r/35120/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Review Request 35120: Use non-POD type for alias example in c++ style guide.

[Joris Van Remoortere]

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

Review request for mesos, Benjamin Hindman and Ben Mahler.


Repository: mesos


Description
---

Also got rid of an unused struct.


Diffs
-

  docs/mesos-c++-style-guide.md 38dd201a22dd775971fad190378d022c50885969 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 33159: Pump updateFramework through Allocator from Master.

[Benjamin Hindman]

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

Ship it!



src/master/allocator/mesos/hierarchical.hpp


CHECK_EQ


- Benjamin Hindman


On June 5, 2015, 8:53 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33159/
> ---
> 
> (Updated June 5, 2015, 8:53 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-2615 and MESOS-703
> https://issues.apache.org/jira/browse/MESOS-2615
> https://issues.apache.org/jira/browse/MESOS-703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up of 32961 where we add the 'updateFramework' function to the 
> allocator.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8347cfaf6abce155a777484970e595e6b141bb87 
>   src/master/allocator/mesos/allocator.hpp 
> b57b03daf992082ec7c73199ab2574bf7993 
>   src/master/allocator/mesos/hierarchical.hpp 
> 44fbccaf72b65251095f69b68627d0efa58707d4 
>   src/master/master.cpp be0db42da3c59761aa154439653d715556465256 
>   src/tests/mesos.hpp 86660ac4d7476bc6cc077fc2d474c4a9ac81c031 
> 
> Diff: https://reviews.apache.org/r/33159/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 33159: Pump updateFramework through Allocator from Master.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [32961, 33159]

All tests passed.

- Mesos ReviewBot


On June 5, 2015, 8:53 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33159/
> ---
> 
> (Updated June 5, 2015, 8:53 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-2615 and MESOS-703
> https://issues.apache.org/jira/browse/MESOS-2615
> https://issues.apache.org/jira/browse/MESOS-703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up of 32961 where we add the 'updateFramework' function to the 
> allocator.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8347cfaf6abce155a777484970e595e6b141bb87 
>   src/master/allocator/mesos/allocator.hpp 
> b57b03daf992082ec7c73199ab2574bf7993 
>   src/master/allocator/mesos/hierarchical.hpp 
> 44fbccaf72b65251095f69b68627d0efa58707d4 
>   src/master/master.cpp be0db42da3c59761aa154439653d715556465256 
>   src/tests/mesos.hpp 86660ac4d7476bc6cc077fc2d474c4a9ac81c031 
> 
> Diff: https://reviews.apache.org/r/33159/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 33159: Pump updateFramework through Allocator from Master.

[Joris Van Remoortere]

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

(Updated June 5, 2015, 8:53 a.m.)


Review request for mesos and Benjamin Hindman.


Changes
---

rebased.


Bugs: MESOS-2615 and MESOS-703
https://issues.apache.org/jira/browse/MESOS-2615
https://issues.apache.org/jira/browse/MESOS-703


Repository: mesos


Description
---

Follow up of 32961 where we add the 'updateFramework' function to the allocator.


Diffs (updated)
-

  include/mesos/master/allocator.hpp 8347cfaf6abce155a777484970e595e6b141bb87 
  src/master/allocator/mesos/allocator.hpp 
b57b03daf992082ec7c73199ab2574bf7993 
  src/master/allocator/mesos/hierarchical.hpp 
44fbccaf72b65251095f69b68627d0efa58707d4 
  src/master/master.cpp be0db42da3c59761aa154439653d715556465256 
  src/tests/mesos.hpp 86660ac4d7476bc6cc077fc2d474c4a9ac81c031 

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


Testing
---

make check


Thanks,

Joris Van Remoortere

Re: Review Request 33271: Update style guide to disallow capturing temporaries by reference.

[Joris Van Remoortere]

Hey BenM,

BenH added spaces before the commit, but thanks for also catching it!
I'll follow up with a small patch to change the POD to something like
SlaveID.

Can you shepherd that one? :-)

Joris

On Fri, Jun 5, 2015 at 8:03 AM, Ben Mahler 
wrote:

>This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33271/
>
> Thanks guys!
>
>
>docs/mesos-c++-style-guide.md
>  
> (Diff
> revision 9)
>
> 180
>
> foreachpair(const int& key, hashset& values, index) {}
>
>   181
>
> foreachvalue(const hashset& values, index) {}
>
>   182
>
> foreachkey(const int& key, index) {}
>
>   Do we really want to encourage taking a const& of a POD type? In general we 
> have not been doing this, so it seems pretty inconsistent to put it in this 
> example.
>
> Also, looks like we need a space before the openening parenthesis.
>
>
> - Ben Mahler
>
> On June 2nd, 2015, 9:34 a.m. UTC, Joris Van Remoortere wrote:
>   Review request for mesos, Bernd Mathiske, Cody Maloney, Joerg Schad,
> Michael Park, and Till Toenshoff.
> By Joris Van Remoortere.
>
> *Updated June 2, 2015, 9:34 a.m.*
>  *Bugs: * MESOS-2629 
>  *Repository: * mesos
> Description
>
> Follow up from r32630.
>
>   Diffs
>
>- docs/mesos-c++-style-guide.md
>(13312f6f4fe1788791479bd768f60df0a8e80e69)
>
> View Diff 
>

Re: Review Request 35102: Remove common/lock. Use synchronized instead.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35088, 35089, 35090, 35091, 35092, 35093, 35094, 35095, 
35096, 35097, 35098, 35099, 35100, 35101, 35102]

All tests passed.

- Mesos ReviewBot


On June 5, 2015, 7:42 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35102/
> ---
> 
> (Updated June 5, 2015, 7:42 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
> 
> 
> Bugs: MESOS-2805
> https://issues.apache.org/jira/browse/MESOS-2805
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 3cf8bd2e86ce5823160f2984738c4e1391081c6a 
>   src/common/lock.hpp 988dff524e3d9f6c0bafa3398f6c3c258829cfe3 
>   src/common/lock.cpp bb8ea3ada6b710357e6872959868d2d8a2035371 
> 
> Diff: https://reviews.apache.org/r/35102/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 35102: Remove common/lock. Use synchronized instead.

[Joris Van Remoortere]

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

(Updated June 5, 2015, 7:42 a.m.)


Review request for mesos, Benjamin Hindman and Bernd Mathiske.


Changes
---

rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/Makefile.am 3cf8bd2e86ce5823160f2984738c4e1391081c6a 
  src/common/lock.hpp 988dff524e3d9f6c0bafa3398f6c3c258829cfe3 
  src/common/lock.cpp bb8ea3ada6b710357e6872959868d2d8a2035371 

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


Testing
---

make check


Thanks,

Joris Van Remoortere

Re: Review Request 35090: Update libprocess process to use synchronized.

[Joris Van Remoortere]

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

(Updated June 5, 2015, 7:42 a.m.)


Review request for mesos, Benjamin Hindman and Bernd Mathiske.


Changes
---

remove extra friendship.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/include/process/process.hpp 
79d1719932a3fdc90b6247d3a77adee123e72435 
  3rdparty/libprocess/src/process.cpp d1b4d469a11abc618c1406bce602300dd9793b58 

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


Testing
---


Thanks,

Joris Van Remoortere