is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28663/#review63753
-----------
On Dec. 3, 2014, 7:53 p.m., Ben Mahler wrote:
>
> ---
>
h the testing cleanup, and for now, drop a TODO
for testing (1) and (2)?
src/master/master.cpp
<https://reviews.apache.org/r/28664/#comment106060>
Per my comment above, can you add a TODO here?
- Ben Mahler
On Dec. 3, 2014, 7:38 p.m., J
626/#comment106068>
Do you need to validate the ID as well? Will it ultimately need to land as
a directory name on the slave (i.e. no slashes, etc)?
- Ben Mahler
On Dec. 3, 2014, 7:41 p.m., Jie Yu wrote:
>
> ---
> This is a
s.apache.org/r/28665/diff/
Testing
---
make check
Thanks,
Ben Mahler
src/tests/resource_offers_tests.cpp 7e09319727992505b0b98af0943f9541c5d182e2
src/tests/slave_recovery_tests.cpp 2b6c76af704b3b02da27fe463b3e52086d2d106a
Diff: https://reviews.apache.org/r/28667/diff/
Testing
---
make check
Thanks,
Ben Mahler
.
Diffs
-
src/master/hierarchical_allocator_process.hpp
fbaa23fad37fc6cbe870932cd4ace6622080001b
Diff: https://reviews.apache.org/r/28669/diff/
Testing
---
make check
Thanks,
Ben Mahler
f7dfd
Diff: https://reviews.apache.org/r/28668/diff/
Testing
---
make check
Thanks,
Ben Mahler
src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223
Diff: https://reviews.apache.org/r/28666/diff/
Testing
---
make check
Thanks,
Ben Mahler
Thanks,
Ben Mahler
tor_process.hpp
fbaa23fad37fc6cbe870932cd4ace6622080001b
Diff: https://reviews.apache.org/r/28663/diff/
Testing
---
make check
Thanks,
Ben Mahler
/r/28684/#comment106095>
Hm.. could we just use std::array? Looks like it is included in 4.4:
https://gcc.gnu.org/ml/libstdc++/2010-03/msg00046.html
Ditto for task validators.
src/master/master.cpp
<https://reviews.apache.org/r/28684/#comment106094>
s/checkers/validators
omment? ;)
src/tests/resource_offers_tests.cpp
<https://reviews.apache.org/r/28627/#comment106031>
How about 'UnreservedDiskInfo'?
- Ben Mahler
On Dec. 3, 2014, 3:05 a.m., Jie Yu wrote:
>
> ---
> This is an
we're doing.
At that point, we don't need the comment :)
src/master/master.cpp
<https://reviews.apache.org/r/28626/#comment106018>
s/RO/Read-only/ ?
src/master/master.cpp
<https://reviews.apache.org/r/28626/#c
, but I'll clean it up :)
- Ben
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28666/#review63759
-------
On Dec
command line.
Thanks! Will do.
- Ben
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28665/#review63755
-------
On De
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28433/#review63626
---
Ship it!
Just curious, how did you find this?
- Ben Mahler
On
tps://reviews.apache.org/r/28616/#comment105904>
Is this TODO still needed? Looks like we'll send back the message now,
which I assume will reference the name of the invalid resource?
- Ben Mahler
On Dec. 3, 2014, 12:15 a.m., J
askMessage when we add the task. In isolation,
that's what I would expect this patch to do. :)
- Ben Mahler
On Nov. 24, 2014, 7:34 p.m., Vinod Kone wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
accidentally forgot
to set the author field when doing that, apologies!
- Ben Mahler
On Nov. 24, 2014, 7:34 p.m., Vinod Kone wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache
these are "Visitors", but they are all ultimately for
validation.
So, how about we call these Validators? TaskIDValidator is a TaskValidator,
UniqueOfferIDValidator is an OfferValidator.
- Ben Mahler
On Dec. 2, 2014, 8:36 p.m.,
tps://reviews.apache.org/r/28444/#comment105681>
Thanks Nik!
Mind adding a newline before the start of the comment? Not sure why the
lack of newlines was missed for ContainerInfo/HealthCheck, but the rest of this
file uses whitespace. :)
- Ben Mahler
On Nov. 25, 2014, 6:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28433/#review63393
---
Where was the double counting occurring?
- Ben Mahler
On Nov. 25
for the using clauses,
think you can get rid of vector and process now.
- Ben Mahler
On Nov. 21, 2014, 1:55 a.m., Niklas Nielsen wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.
;& user.get() == expected) {
return 0;
}
return 1;
```
If not, let's document why whoami is needed!
- Ben Mahler
On Nov. 19, 2014, 4:53 p.m., Niklas Nielsen wrote:
>
> ---
> This is an automatically gene
> On Nov. 20, 2014, 9:11 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/http.hpp, lines 415-416
> > <https://reviews.apache.org/r/28253/diff/1/?file=770208#file770208line415>
> >
> > Here's what we'll have if we added join:
> &
d the regresion and comment on it
with this review?
- Ben Mahler
On Nov. 21, 2014, 12:23 a.m., Jie Yu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.a
189/#comment104510>
Why the change to the logic here? Looks like a regression in some cases:
For example, if there is an executor with port 31000, but no cpus and no
memory, we'll launch it with no memory now!
- Ben Mahler
On Nov. 18, 2014, 10:54 p.m., Ian D
call .disk() even if it's not set."
src/common/resources.cpp
<https://reviews.apache.org/r/28264/#comment104505>
(e.g. across slaves)
src/common/resources.cpp
<https://reviews.apache.org/r/28264/#comment104506>
Could you add a note about why subtractable need
s valid ones? ;)
Should maybe split the test or just call it 'Validation'.
- Ben Mahler
On Nov. 19, 2014, 11:39 p.m., Jie Yu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
>
-
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28258/
> -------
>
> (Updated Nov. 19, 2014, 11:39 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, switched to
>
ent104497>
"a reservation"
- Ben Mahler
On Nov. 20, 2014, 8:40 p.m., Jie Yu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27550/
> ---
> On Nov. 20, 2014, 9:11 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/http.hpp, lines 415-416
> > <https://reviews.apache.org/r/28253/diff/1/?file=770208#file770208line415>
> >
> > Here's what we'll have if we added join:
> &
> On Nov. 20, 2014, 9:11 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/http.hpp, lines 415-416
> > <https://reviews.apache.org/r/28253/diff/1/?file=770208#file770208line415>
> >
> > Here's what we'll have if we added join:
> &
ike to make this more intuitive.
- Ben Mahler
On Nov. 20, 2014, 12:50 a.m., Cody Maloney wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> h
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28294/#review62415
---
Ship it!
Ship It!
- Ben Mahler
On Nov. 20, 2014, 7:37 p.m
g/r/28293/#comment104460>
Hm.. sharing the same key is allowed? What about same key, but with no
values?
e.g.
key = "foo"
key = "foo"
Is that allowed? What would be the point of that?
- Ben Mahler
On Nov. 20, 2014, 7:37
he.org/r/27606/#comment104457>
What is this? Why should it work?
src/tests/files_tests.cpp
<https://reviews.apache.org/r/27606/#comment104458>
This should work per my top level comment ;)
- Ben Mahler
On Nov. 20, 2014, 12:34 a.m., Cody Maloney wrote:
>
; when one hits browse.json without an argument?
e.g.
$ curl .../browse.json
/log
/containers/foo/
/containers/bar/
...
- Ben Mahler
On Nov. 19, 2014, 11:48 p.m., Cody Maloney wrote:
>
> ---
> This is an automatically generated
enforce the format as part
of the tests, to avoid accidentally breaking the http api. Just to confirm,
would the tests break if the Label protobuf is changed? If not, let's make sure
a test will catch it.
- Ben Mahler
On Nov. 20, 2014, 6:20 p.m., Niklas Nielsen
lease write descriptions on your reviews! Helps provide
context for the reviewers :)
- Ben Mahler
On Nov. 19, 2014, 8:30 p.m., Cody Maloney wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https
check w/ many iterations of the reconciliation tests
Thanks,
Ben Mahler
---
Added a test and ran all the reconciliation tests in repetition.
Thanks,
Ben Mahler
de42f8eb7c3c4ed64fb7fea9f4977e276f4a9043
Diff: https://reviews.apache.org/r/28266/diff/
Testing
---
make check w/ many iterations of the reconciliation tests
Thanks,
Ben Mahler
<https://reviews.apache.org/r/28256/#comment104241>
Doesn't look like you need this? Can't you just compoase .contains() and
.put() in the caller?
- Ben Mahler
On Nov. 19, 2014, 9:01 p.m., Cody Maloney wrote:
>
> --
since these do not acquire the locks!
Seems a little unfortunate to make the reasoning around the synchronization
invariants less local.
- Ben Mahler
On Nov. 19, 2014, 12:03 a.m., Joris Van Remoortere wrote:
>
> ---
155/#comment104200>
I'm curious, was JSON::Protobuf not sufficient here?
```
JSON::Array labels;
foreach (const Label& label, task.labels()) {
labels.values.push_back(JSON::Protobuf(label));
}
object.values["labels"] = labels;
```
Ditto b
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28142/#review62054
---
Ship it!
Great, I can throw away my branch now! ;)
- Ben Mahler
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28143/#review62053
---
Ship it!
Ship It!
- Ben Mahler
On Nov. 18, 2014, 1:35 a.m., Jie
ished.. ;)
- Ben Mahler
On Nov. 18, 2014, 8 p.m., Ian Downes wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27923/
> -
stop anyone from
constructing a Resources without validating anyway.
In fact, the code you adjusted still performs no validation because it is
assumed that the offer's resources are valid, so not sure what we're gaining
here. Can we punt on this one?
- Ben Mahler
On Nov. 17, 2014,
removed.
src/slave/containerizer/containerizer.cpp
<https://reviews.apache.org/r/28091/#comment103739>
No comment...? :)
How about a block comment after we parse the resources?
// NOTE: We need to check for the "cpus" string within the flag
// because on
frameworks use an internal abstraction that
provides allocation primitives. I agree that allocation primitives don't belong
here.
- Ben Mahler
On Nov. 15, 2014, 7:01 a.m., Jie Yu wrote:
>
> ---
> This is an automatically genera
se be
simplified?
```
left.mutable_scalar()->CopyFrom(left.scalar() + right.scalar());
```
Ditto for all the others, looks like they could all be done in one line,
ditto for -= implementation.
- Ben Mahler
On Nov. 17, 20
---
>
> (Updated Nov. 15, 2014, 7:18 a.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1974
> https://issues.apache.org/jira/browse/MESOS-1974
>
>
> Repository: mesos-git
>
>
>
03662>
How about a newline here?
- Ben Mahler
On Nov. 17, 2014, 7:06 p.m., Joris Van Remoortere wrote:
>
> ---
> This is an automatically generated e-mail. To re
<https://reviews.apache.org/r/28125/#comment103647>
What does "link" mean in this context?
How about:
"Failed to create socket, os::nonblock: " + ...
Ditto below.
- Ben Mahler
On Nov. 17, 2014, 6:47
src/common/resources.cpp
<https://reviews.apache.org/r/28088/#comment103415>
Could we go with 'value' here?
- Ben Mahler
On Nov. 15, 2014, 6:51 a.m., Jie Yu wrote:
>
> ---
> This is an automatically generate
change here?
src/tests/gc_tests.cpp
<https://reviews.apache.org/r/28091/#comment103404>
Looks like these tests should just be using resources.cpus() and
resources.mem()?
src/tests/resource_offers_tests.cpp
<https://reviews.apache.org/r/28091/#
is not enough for the tests to remain unchanged?
Seems a bit too clunky without these operators IMO.
- Ben Mahler
On Nov. 15, 2014, 7:11 a.m., Jie Yu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
>
03s user 0.02s system 93% cpu 0.054 total
```
This is after several runs to warm the file cache.
- Ben Mahler
On Nov. 15, 2014, 6:43 a.m., Jie Yu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visi
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28093/#review61651
---
Ship it!
- Ben Mahler
On Nov. 15, 2014, 7:24 a.m., Jie Yu wrote
/resources.cpp
<https://reviews.apache.org/r/28094/#comment103390>
How about including `this->` to show the symmetry?
```
return this->contains(that) && that.contains(*this);
```
- Ben Mahler
On Nov. 15, 2014, 7:3
> On Nov. 14, 2014, 7:33 p.m., Ben Mahler wrote:
> > This will need some more thought, as this breaks the resource accounting in
> > the master.
> > The slave re-registers with tasks and regular executors, which excludes the
> > command executors.
> >
> >
tps://reviews.apache.org/r/27924/#comment103141>
- Ben Mahler
On Nov. 14, 2014, 7:02 p.m., Ian Downes wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27923/#review61481
---
Ship it!
Ship It!
- Ben Mahler
On Nov. 12, 2014, 8:57 p.m., Ian
923/#comment103131>
Are you not using the commit hook?
>>> len(' slave::Slave::_runTask(future, frameworkInfo, frameworkId, pid,
task, executorInfo);')
86
- Ben Mahler
On Nov. 12, 2014, 8:57 p.m., Ian Downes wrote:
>
> --
apache.org/r/27945/#comment102833>
Let's match the brace-on-a-newline style here :)
I'll do this for you.
m4/ax_cxx_compile_stdcxx_11.m4
<https://reviews.apache.org/r/27945/#comment102837>
Let's remove the extra whitespace introduced here, I'll do thi
here that implicit string construction is
allowed).
I'll take care of this for you.
- Ben Mahler
On Nov. 13, 2014, 12:39 a.m., Cody Maloney wrote:
>
> ---
> This is an automatically generated e-mail. To reply, vis
d new test to the libprocess Makefile."
Remember these get directly translated into commit messages, so end with a
period.
- Ben Mahler
On Nov. 13, 2014, 12:39 a.m., Cody Maloney wrote:
>
> ---
> This is an automatically g
> On Nov. 10, 2014, 7:50 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp, line 30
> > <https://reviews.apache.org/r/27605/diff/1/?file=750087#file750087line30>
> >
> > Is there a configure check for initializer lists?
>
like this was not the
intended behvior on Linux either.
How about: "Fixed a bad logging message in libprocess initialization." ?
- Ben Mahler
On Nov. 12, 2014, 10:17 p.m., Dominic Hamon wrote:
>
> ---
> This is an auto
;
Just to be sure, does this test fails without the fix?
src/tests/scheduler_tests.cpp
<https://reviews.apache.org/r/27896/#comment102633>
How about just StopThenAbort, or do you think that's not specific enough?
- Ben
> On Nov. 12, 2014, 1:04 a.m., Ben Mahler wrote:
> > Hey Ben, would love to help review these. Any chance you could link a
> > ticket in here with the context? And/or update the description on these
> > changes to say a bit more than "See summary"?
> >
--
>
> (Updated Nov. 11, 2014, 4:52 p.m.)
>
>
> Review request for mesos, Ben Mahler, Jie Yu, Joris Van Remoortere, Niklas
> Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
>
> On Oct. 18, 2014, 9:50 p.m., Ben Mahler wrote:
> > src/tests/slave_tests.cpp, line 528
> > <https://reviews.apache.org/r/26894/diff/1/?file=724949#file724949line528>
> >
> > Hm.. this doesn't look like the right usage of shell=false (which
> On Nov. 5, 2014, 7:50 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, lines 1195-1200
> > <https://reviews.apache.org/r/27567/diff/1/?file=748326#file748326line1195>
> >
> > A comment here as to why we don't need to send TASK_LOST would be much
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27826/#review60717
---
Ship it!
Ship It!
- Ben Mahler
On Nov. 11, 2014, 12:13 a.m
tps://reviews.apache.org/r/27831/#comment102075>
Hm.. not sure why 'message' is on the heap in the first place, but we can
save that for later.
- Ben Mahler
On Nov. 10, 2014, 10:26 p.m., Dominic Hamon wrote:
>
> --
case though.
3rdparty/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/27826/#comment102064>
Can we avoid the need for this with a separate change to use ABORT when
failure() is called on a non-failed future? Much like we do for try, result,
option.
- Ben Mahler
lt;https://reviews.apache.org/r/27605/#comment102029>
Interestingly:
>>> os.path.join("/a/", "/b/", "/c/")
'/c/'
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp
<https://
BORT to take an Error, or a string as well. =/
3rdparty/libprocess/src/tests/benchmarks.cpp
<https://reviews.apache.org/r/27113/#comment101827>
Let's remove all of the `auto` keywords in this file, they go against the
s
do you think?
- Ben Mahler
On Nov. 4, 2014, 10:01 p.m., Jie Yu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.
567/#comment101353>
A comment here as to why we don't need to send TASK_LOST would be much
appreciated! It's not obvious so someone might come along and add a TASK_LOST
to make sure we're not dropping the task on the floor, so context here would be
great!
- Ben Mahler
On No
> On Nov. 4, 2014, 11:09 a.m., Adam B wrote:
> > Ship It!
>
> Adam B wrote:
> Following the logic with my internal C++ compiler/interpreter (aka my
> brain), it's pretty clear that we would have returned earlier in this
> function if framework were in fact NULL, so the null-check branch is
ould we do this cleanup in the slave too?
src/sched/sched.cpp
<https://reviews.apache.org/r/27315/#comment100699>
How about s/duration/timeout/ ?
src/sched/sched.cpp
<https://reviews.apache.org/r/27315/#comment100700>
We should bound this as w
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27243/#review58737
---
Ship it!
Ship It!
- Ben Mahler
On Oct. 28, 2014, 12:53 a.m
out/svn.hpp
<https://reviews.apache.org/r/27243/#comment99839>
svn_txdelta_to_svndiff3 was introduced in 1.7
svn_txdelta_to_svndiff2 was introduced in 1.4
- Ben Mahler
On Oct. 27, 2014, 10:21 p.m., Timothy Chen wrote:
>
> -
ment99624>
"since"
- Ben Mahler
On Oct. 25, 2014, 11:18 p.m., Benjamin Hindman wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
>
--
>
> (Updated Oct. 24, 2014, 10:04 p.m.)
>
>
> Review request for mesos, Ben Mahler and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> ---
>
> Add namespaces/pid to --isolation slave flag. Pl
tps://reviews.apache.org/r/25966/#comment99465>
"registerExecutorMessage"
Ditto below.
- Ben Mahler
On Oct. 24, 2014, 9:53 p.m., Ian Downes wrote:
>
> ---
> This is an automatically generated e-mail.
tps://reviews.apache.org/r/27164/#comment99464>
Can you just take a non-const copy in the argument list?
- Ben Mahler
On Oct. 24, 2014, 9:55 p.m., Ian Downes wrote:
>
> ---
> This is an automatically generated e-mail.
tps://reviews.apache.org/r/27166/#comment99463>
Should we have a small note similar to your review description?
- Ben Mahler
On Oct. 24, 2014, 9:54 p.m., Ian Downes wrote:
>
> ---
> This is an automatically generated e-mail.
che.org/r/27165/#comment99459>
children
src/tests/ns_tests.cpp
<https://reviews.apache.org/r/27165/#comment99461>
Can we use ROOT?
- Ben Mahler
On Oct. 24, 2014, 9:54 p.m., Ian Downes wrote:
>
> ---
> This is an automat
t99432>
We can use the ROOT_ filter for this and the other tests.
src/tests/ns_tests.cpp
<https://reviews.apache.org/r/27127/#comment99441>
Ditto on !empty
- Ben Mahler
On Oct. 24, 2014, 9:54 p.m., Ian Downes wrote:
>
> --
_test_helper.cpp
<https://reviews.apache.org/r/27091/#comment99421>
Maybe a note as to why pid is special here?
- Ben Mahler
On Oct. 24, 2014, 9:53 p.m., Ian Downes wrote:
>
> ---
> This is an automatically generated e-
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27130/#review58394
---
Ship it!
Ship It!
- Ben Mahler
On Oct. 24, 2014, 2:56 a.m
How about s/$debug/$debug_flags/ and s/$optimize/$optimize_flags/ ?
- Ben Mahler
On Oct. 23, 2014, 9:20 p.m., Cody Maloney wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://re
g/r/27150/#comment99281>
Why did you wrap all the ones above but not this one?
- Ben Mahler
On Oct. 24, 2014, 6:22 p.m., Jie Yu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://re
g/r/27102/#comment99275>
Can you update the existing ExecutorInfo checker instead? We already check
presence there, but not equality.
- Ben Mahler
On Oct. 23, 2014, 11:32 p.m., Vinod Kone wrote:
>
> ---
> This is an automatica
to combine
them? :)
- Ben Mahler
On Oct. 24, 2014, 1:27 a.m., Ian Downes wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache
501 - 600 of 2563 matches
Mail list logo
