int_fd fd = ::mkstemp(&_path[0]);
...
return _path;
```
- Benjamin Mahler
On Feb. 25, 2019, 9:20 p.m., Benjamin Bannier wrote:
>
> ---
> This is an automatically generated e-mail. To
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70046/#review213186
---
Ship it!
Ship It!
- Benjamin Mahler
On Feb. 25, 2019, 9:19
ome just `std::string` without the `Try`.
- Benjamin Mahler
On Feb. 23, 2019, 8:41 p.m., Benjamin Bannier wrote:
>
> ---
> This is an automatically generated e-mail. To re
Diff: https://reviews.apache.org/r/69990/diff/1/
Testing
---
Ran in repetition
Thanks,
Benjamin Mahler
/mesos/hierarchical.cpp
862dbb90bdfa39ead4b185104a308eabe249d734
Diff: https://reviews.apache.org/r/69989/diff/1/
Testing
---
make check, regression test added in subsequent patch
Thanks,
Benjamin Mahler
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212842
---
Ship it!
Ship It!
- Benjamin Mahler
On Feb. 14, 2019, 8:48
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69818/#review212841
---
Ship it!
Ship It!
- Benjamin Mahler
On Feb. 14, 2019, 8:45
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69862/#review212840
---
Ship it!
Ship It!
- Benjamin Mahler
On Feb. 14, 2019, 8:46
Maybe also a comment:
```
We disallow an empty quantity since that is equivalent to an empty set
(i.e. override of "no minimum")
```
- Benjamin Mahler
On Feb. 13, 2019, 10:31 p.m., Benjamin Bannier wrote:
>
>
ow so it's clear
- Benjamin Mahler
On Feb. 14, 2019, 12:27 a.m., Meng Zhu 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/69821/#review212804
---
Ship it!
Ship It!
- Benjamin Mahler
On Feb. 13, 2019, 3:44
> On Feb. 8, 2019, 8:47 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2357-2374 (patched)
> > <https://reviews.apache.org/r/69890/diff/2/?file=2124579#file2124579line2357>
> >
> > Both "empty set&q
this seems to be moving the break from above down to here. Can we move
it back up so that this diff is very minimal (i.e. like the changes in the
quota loop above)
- Benjamin Mahler
On Feb. 11, 2019, 3:46 p.m., Benjamin Bannier wrote:
>
> ---
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69938/#review212756
---
Ship it!
Ship It!
- Benjamin Mahler
On Feb. 11, 2019, 10:27
> On Feb. 11, 2019, 6:58 p.m., Benjamin Mahler wrote:
> > Nice patch, thanks Clément. Just a few minor comments below and we should
> > be good to go.
> >
> > Just as an aside, the current hook interfaces are rather inefficient
> > (copies the entire ta
```
Looks good, thanks
- Benjamin
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69942/#review212751
---
On Feb. 11, 2019, 6:
sources
or
Resources
would all be ok here
src/tests/hook_tests.cpp
Lines 307-309 (patched)
<https://reviews.apache.org/r/69938/#comment298593>
Doesn't look like this is needed, the option being none will be caught below
- Benjamin Mahler
On Feb. 10, 2019, 11:
repetition.
Thanks,
Benjamin Mahler
hanks,
Benjamin Mahler
saves having to execute a few lines at the top
of the loop? Doesn't seem worth it?
- Benjamin Mahler
On Feb. 8, 2019, 11:32 a.m., Benjamin Bannier wrote:
>
> ---
> This is an automatically generated e-ma
ld we
validate that all entries are non-empty? There doesn't seem to be any point in
allowing an empty entry, since any single empty entry renders it equivalent to
the empty set case?
- Benjamin Mahler
On Feb. 8, 2019, 11:32 a.m., Benjamin Bannier wrote:
>
> ---
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69889/#review212677
---
Ship it!
Ship It!
- Benjamin Mahler
On Feb. 8, 2019, 11:32
below?
> > ditto blew in the second stage
>
> Benjamin Mahler wrote:
> This condition is for **break**ing the role loop, whereas
> allocatableTo(role) would be a continue.
>
> I initially had an **additional** continue-based allocatableTo(role)
> check a
below?
> > ditto blew in the second stage
>
> Benjamin Mahler wrote:
> This condition is for **break**ing the role loop, whereas
> allocatableTo(role) would be a continue.
>
> I initially had an **additional** continue-based allocatableTo(role)
> check a
offeredSharedResources[slaveId] = agentOfferedShared
> > }
> >
> > ditto below in the second stage
Looks complicated, would definitely want to make sure that there's a
significant performance benefit to that to justify it?
- Benjamin
-
Description
---
Reduced unnecessary agent lookups in the allocation loops.
Diffs
-
src/master/allocator/mesos/hierarchical.cpp
bb9a9c95979f36c0564af5b3babb1c43077a363b
Diff: https://reviews.apache.org/r/69900/diff/1/
Testing
---
make check
Thanks,
Benjamin Mahler
d then rebase this against master.
- Benjamin Mahler
On Feb. 5, 2019, 4:38 p.m., Benjamin Bannier wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
&
1 offers
```
Thanks,
Benjamin Mahler
mework` struct) that this field needs to be updated when
`HierarchicalAllocatorProcess::updateFramework(...)` is called.
Can we also have a test ensuring updateFramework correctly updates it?
- Benjamin Mahler
On Feb. 4, 2019, 10:05 p.m., Benjamin Bannier
a
mixture of "minimal" and "minimum" used?
src/tests/hierarchical_allocator_tests.cpp
Lines 2378-2381 (patched)
<https://reviews.apache.org/r/69890/#comment298390>
This seems like the "EmptyMinAllocatable" case and th
d both ways by accident (which becomes a problem if we ever do
mutation).
- Benjamin Mahler
On Feb. 4, 2019, 10:05 p.m., Benjamin Bannier wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://re
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69889/#review212561
---
Ship it!
Ship It!
- Benjamin Mahler
On Feb. 4, 2019, 10:05
4155 (patched)
<https://reviews.apache.org/r/69885/#comment298335>
"Invalid"?
src/tests/resources_tests.cpp
Line 4148 (original), 4176 (patched)
<https://reviews.apache.org/r/69885/#comment298336>
CHECK_NOTERROR rather than naked get?
- Benjamin Mahler
On Feb
ework.cpp
Lines 504 (patched)
<https://reviews.apache.org/r/69821/#comment298325>
Ah, here it is, should be in the earlier review?
- Benjamin Mahler
On Feb. 4, 2019, 8:37 p.m., Benjamin Bannier wrote:
>
> ---
> This is an
(patched)
<https://reviews.apache.org/r/69818/#comment298324>
The comment you mentioned seems to be missing?
- Benjamin Mahler
On Feb. 4, 2019, 11:14 a.m., Benjamin Bannier wrote:
>
> ---
> This is an automatically gener
d then
indexing into that?
https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#sharing-resources-between-tests-in-the-same-test-suite
- Benjamin Mahler
On Feb. 4, 2019, 12:55 p.m., Benjamin Bannier
(patched)
<https://reviews.apache.org/r/69862/#comment298250>
Should we also disallow NaN and +inf? (along with tests for these cases?)
- Benjamin Mahler
On Jan. 31, 2019, 3:15 p.m., Benjamin Bannier wrote:
>
> ---
> On Jan. 29, 2019, 8:37 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2336 (patched)
> > <https://reviews.apache.org/r/69821/diff/2/?file=2123197#file2123197line2336>
> >
> > FrameworkEmptyMin
tps://reviews.apache.org/r/69818/#comment298248>
Can you guard this with a has check?
As it stands it will produce a different message if newInfo has it unset
(info will have it set vs newInfo has it not set)
- Benjamin Mahler
On Jan. 31, 2019, 3:29 p.m., Benjamin Bannier
> On Jan. 30, 2019, 4:09 p.m., Benjamin Mahler wrote:
> > Thanks! Probably don't need the duplicate description in the commit message?
>
> Benjamin Bannier wrote:
> The way we have our reviewboard interaction set up, not manually
> adjusting a redundant descript
ption in the commit message?
- Benjamin Mahler
On Jan. 30, 2019, 3:03 p.m., Benjamin Bannier wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.a
apache.org/r/69821/#comment298192>
Hm.. this doesn't quite look right to me, it seems if a framework is
setting the `min_allocatable_resources` field, it should override, but that's
not happening here?
- Benjamin Mahler
On Jan. 29, 2019, 4:20 p.m., Benjamin Bannier wrote:
tched)
<https://reviews.apache.org/r/69818/#comment298185>
Can we add some validation somewhere to ensure that negative value scalars
are not provided?
- Benjamin Mahler
On Jan. 29, 2019, 4:24 p.m., Benjamin Bannier wrote:
>
>
> On Jan. 29, 2019, 3:09 a.m., Benjamin Mahler wrote:
> > include/mesos/mesos.proto
> > Lines 1519 (patched)
> > <https://reviews.apache.org/r/69818/diff/1/?file=2121448#file2121448line1519>
> >
> > I'm not sure if this gives much information t
put this on the variable instead of the message? Seems worth
reconciling this comment with the flag help string?
- Benjamin Mahler
On Jan. 23, 2019, 12:17 p.m., Benjamin Bannier wrote:
>
> ---
> This is an automatically generated e-ma
order to determine (1) whether
to store zeros, (2) how to evaluate equality and containment between two
quantities.
Thoughts?
- Benjamin Mahler
On Jan. 23, 2019, 12:12 p.m., Benjamin Bannier wrote:
>
> ---
> This is an auto
make it fit on one line?
3rdparty/stout/include/stout/protobuf.hpp
Line 197 (original), 197-203 (patched)
<https://reviews.apache.org/r/69082/#comment297786>
ditto here
- Benjamin Mahler
On Jan. 16, 2019, 1:06 p.m., Benjamin Bannier wrote:
>
> ---
> On Jan. 15, 2019, 6:22 p.m., Benjamin Mahler wrote:
> > Should we be surfacing a close EINTR as an error or let that be silent?
> > I think these errors need some message pre-fixing? E.g.
> >
> > ```
> > Failed to close '3': Bad file number
>
AILURE) << ...; ?
}
```
Some cases don't even handle the failures? :O
E.g.
https://github.com/apache/mesos/blob/f01853aea4eaa3df6dec3f7342e5583f5addd07d/src/master/master.cpp#L1745-L1746
Ideally, the return type of the registry apply operation would allow us to
distinguish between timeo
s 3940-3943 (original), 3959-3962 (patched)
<https://reviews.apache.org/r/69588/#comment297622>
Ditto `Resources::reservationRole` and `*` being invalid (but I guess we
don't care here?)
- Benjamin Mahler
On Dec. 19, 2018, 11:20 p.m., Chun-Hung Hsiao wrote:
>
> ---
ty/stout/include/stout/os/write.hpp
Line 140 (original), 140 (patched)
<https://reviews.apache.org/r/69082/#comment297615>
newline above this to be consistent with the rest of the formatting in this
function? ditto for the others
- Benjamin Mahler
On
description that these are now
duplicated via the options struct and used through that? Did I understand right?
- Benjamin Mahler
On Jan. 12, 2019, 12:12 a.m., Meng Zhu wrote:
>
> ---
> This is an automatically generated e-mail. To rep
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69724/#review211907
---
Ship it!
Ship It!
- Benjamin Mahler
On Jan. 11, 2019, 9:48
he.org/r/69662/#comment297386>
This seems not very readable (or at least I struggled a bit with it), can
we use underscore js clone?
https://underscorejs.org/#clone
```
provider.total_resources_full = _.clone(provider.total_resources);
```
- Benjamin Mahler
On Ja
------
On Jan. 9, 2019, 2:53 a.m., Benjamin Mahler wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69689/
>
/69689/diff/2/
Changes: https://reviews.apache.org/r/69689/diff/1-2/
Testing
---
Ran in repetition before (could trigger the failure) and after (no failures
yet).
Thanks,
Benjamin Mahler
in the
screenshot and it appears to be the case). is it missing some attributes that
the reservation table has?
- Benjamin Mahler
On Jan. 7, 2019, 9:02 p.m., Benjamin Bannier wrote:
>
> ---
> This is an automatically generated e
after (no failures
yet).
Thanks,
Benjamin Mahler
ing provided?
Maybe the column should just be 'Resources' and a stringified version is shown?
- Benjamin Mahler
On Jan. 4, 2019, 10:14 a.m., Benjamin Bannier wrote:
>
> ---
> This is an automatically generated e
> On Jan. 7, 2019, 5:14 a.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Line 671 (original), 671 (patched)
> > <https://reviews.apache.org/r/69599/diff/4/?file=2117856#file2117856line671>
> >
> > Why do you say to consi
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69599/#review211727
---
Ship it!
Ship It!
- Benjamin Mahler
On Jan. 7, 2019, 6:23
But in cases like this, where we're actually inside the guarded block, just
use ->
```
if (allocation.isSome()) {
share = std::max(share, allocation->value() / scalar.value());
```
- Benjamin Mahler
On J
(patched)
<https://reviews.apache.org/r/69600/#comment297240>
Do you want to add a comment above this section of invalid cases, as done
on the others?
- Benjamin Mahler
On Jan. 6, 2019, 7:33 p.m., Meng Zhu wrote:
>
> ---
unfortunate.. if the user typos in "3.1a" it
will fall into TEXT :(
src/tests/values_tests.cpp
Lines 93-103 (patched)
<https://reviews.apache.org/r/69673/#comment297238>
You could group each pair without a newline? Do you also want to add a
subnormal case?
src/v1/values.cpp
L
r.cpp
Lines 750 (patched)
<https://reviews.apache.org/r/69603/#comment297203>
Probably just want to use `*resourceQuantities` since we handle the error
case above.
- Benjamin Mahler
On Jan. 4, 2019, 8:54 p.m., Meng Zhu wrote:
>
>
foreach (
src/v1/resources.cpp
Lines 1538-1540 (patched)
<https://reviews.apache.org/r/69601/#comment297195>
Maybe:
```
foreach (const Resource& r, get(quantity.first)) {
```
- Benjamin M
o reply, visit:
> https://reviews.apache.org/r/69600/
> -------
>
> (Updated Jan. 4, 2019, 8:14 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
.get(resourceName)
.getOrElse(Value::Scalar()); // Default to zero.
share = std::max(share, allocation.value() / total.value());
```
src/master/allocator/sorter/random/sorter.hpp
Line 176 (original)
rg/r/69603/#comment297045>
Maybe you can line up these contains comments to make it a bit more
readable?
- Benjamin Mahler
On Dec. 20, 2018, 7 a.m., Meng Zhu wrote:
>
> ---
> This is an automatically generated e-mail. To reply,
> ---
>
> (Updated Dec. 31, 2018, 5:52 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Added par
onEmptyResources.contains(emptyQuantities);
auto resources = [](const string& s) { ... };
auto quantities = [](const string& s) { ... };
// Can easily read what's being checked:
EXPECT_TRUE(resources("cpus:1;mem:2")
.contains(quantities("
st == name) {
break;
}
if (it->first > name) {
it = quantities.insert(
it, std::make_pair(name, Value::Scalar()));
break;
}
}
it->second += scalar;
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69398/#review211450
---
Ship it!
Ship It!
- Benjamin Mahler
On Dec. 17, 2018, 8:43
<https://reviews.apache.org/r/69557/#comment296586>
Should we clarify that it's subscribe?
FrameworkSubscribeEmptyId
- Benjamin Mahler
On Dec. 17, 2018, 8:43 a.m., Benjamin Bannier wrote:
>
> ---
> This is an automati
88/#comment296564>
Ditto here.
src/master/master.cpp
Lines 3809-3814 (original), 3799-3804 (patched)
<https://reviews.apache.org/r/69588/#comment296565>
Ditto here.
src/master/master.cpp
Line 3817 (original), 3807 (
t; to "now" occurred? Is it
due to your other pending patches, or was this something we changed in a
previous release?
If it's part of your pending patches, we might have to keep the empty == unset
behavior, since I'm not sure which frameworks were relying on that.
- Benjamin Ma
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69397/#review211211
---
Ship it!
Ship It!
- Benjamin Mahler
On Dec. 11, 2018, 1:48
os/blob/1.7.0/src/master/master.cpp#L337-L338
So today it's ` + "-N"`?
- Benjamin Mahler
On Dec. 11, 2018, 1:48 p.m., Benjamin Bannier wrote:
>
> ---
> This is an automatically generated e-mail. To re
(patched)
<https://reviews.apache.org/r/69098/#comment295930>
Do we need these two start outputs? It seems like they clutter the output
to me.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 804-807 (patched)
<https://reviews.apache.org/r/69098/#comment295931>
Maybe
allocator (and
more) to push gauges. Can you update this to use a push gauge?
- Benjamin Mahler
On Dec. 5, 2018, 10:03 p.m., Joseph Wu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://review
> On Nov. 2, 2018, 10:23 p.m., Benjamin Mahler wrote:
> > Hm.. why is this one not just extending the one you added in the previous
> > review?
>
> Meng Zhu wrote:
> This benchmark compares quota vs. non-quota. For non-quota setting, there
> will be no chopp
ps://reviews.apache.org/r/68138/#comment295875>
Maybe: QuotaAccountingUnreserveAllocatedResources?
src/tests/hierarchical_allocator_tests.cpp
Lines 1775 (patched)
<https://reviews.apache.org/r/68138/#comment295876>
Ditto here, the quota setting is actually above this comment?
for windows recently with
executor IDs.
- Benjamin Mahler
On Nov. 19, 2018, 3:52 p.m., Benjamin Bannier wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69451/#review210931
---
Ship it!
Ship It!
- Benjamin Mahler
On Nov. 28, 2018, 12:40
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69464/#review210915
---
Ship it!
Ship It!
- Benjamin Mahler
On Nov. 27, 2018, 11:05
connected -> one of `http` or `pid`
// is set.
}
if (http.isSome()) {
if (!http->send(message)) {
LOG(WARNING) << "Unable to send event to framework " << *this << ":"
<< " connect
> On Nov. 27, 2018, 9:16 p.m., Benjamin Mahler wrote:
> > src/tests/master_tests.cpp
> > Lines 3850 (patched)
> > <https://reviews.apache.org/r/69452/diff/1/?file=2110508#file2110508line3850>
> >
> > Maybe a TODO to simplify this test by having a test
here? seems like this should be a different
patch?
- Benjamin Mahler
On Nov. 27, 2018, 4:58 a.m., Chun-Hung Hsiao wrote:
>
> ---
> This is an automatically generated e-
s? My read of the http/pid state
comment is that it will be set based on the last connection, therefore the only
time one of them won't be set is the recovered case (the change that broke the
original one-being-set invariant).
- Benjamin Mahler
On Nov. 27, 2018, 4:58 a.m., Chun-Hung Hsi
tests/partition_tests.cpp 788d3b988cf7de206f295bffdc0376e7e1242349
Diff: https://reviews.apache.org/r/69280/diff/1/
Testing
---
Ran in repetition.
Thanks,
Benjamin Mahler
/filesystem_tests.cpp
29f06f3dd126007d6b3a81f31795270f0654b847
Diff: https://reviews.apache.org/r/69276/diff/1/
Testing
---
make check
Thanks,
Benjamin Mahler
Diff: https://reviews.apache.org/r/69277/diff/1/
Testing
---
Configured and ensured the parallel test runner was the
default when using make check.
Thanks,
Benjamin Mahler
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69275/#review210374
---
Ship it!
Ship It!
- Benjamin Mahler
On Nov. 7, 2018, 8:30
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69274/#review210373
---
Ship it!
Ship It!
- Benjamin Mahler
On Nov. 7, 2018, 8:30
> On Nov. 6, 2018, 5:58 a.m., Benjamin Mahler wrote:
> > Ah, unfortunately, the use of `os::setenv` is going to break our parallel
> > test runner :(
> >
> > We could have reinitialize take a flags object?
>
> Benjamin Bannier wrote:
> Not sure why this
dequeued/messages: 1
```
Also exposing a queue size directly (via a pull guauge that subtracts the
counters) seems like a nice addition to this.
Thoughts?
- Benjamin Mahler
On July 18, 2018, 1:28 a.m., Benjamin Hindman wrote:
>
> ---
nts running tests in parallel :(
Is there any way to avoid this?
- Benjamin Mahler
On July 18, 2018, 1:19 a.m., Benjamin Hindman wrote:
>
> ---
> This is an automatically generate
iews.apache.org/r/67957/#comment295000>
s/do//
s/on//
- Benjamin Mahler
On July 18, 2018, 1:19 a.m., Benjamin Hindman wrote:
>
> ---
> This is an automatically
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67956/#review210340
---
Ship it!
Ship It!
- Benjamin Mahler
On July 18, 2018, 1:19
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67955/#review210339
---
Ship it!
Ship It!
- Benjamin Mahler
On July 18, 2018, 1:19
901 - 1000 of 2691 matches
Mail list logo
