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

> On Sept. 23, 2015, 6:29 p.m., Ben Mahler wrote: > > Where is the benchmark for this change? :( > > Joerg Schad wrote: > We benchmarked the code with a proprietary benchmark. I added > https://issues.apache.org/jira/browse/MESOS-3502 aiming at adding an > open-source benchmark for this.

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review100191 --- Patch looks great! Reviews applied: [38158] All tests passed. -

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review100185 --- src/common/values.cpp (lines 93 - 94)

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review100189 --- src/common/values.cpp (line 253)

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

> On Sept. 23, 2015, 12:58 a.m., Michael Park wrote: > > src/common/values.cpp, line 301 > > > > > > Can we just take a `Value::Ranges` here rather than > > `initializer_list`? > > > > It looks like the

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

> On Sept. 23, 2015, 12:58 a.m., Michael Park wrote: > > src/common/values.cpp, line 204 > > > > > > Why not pass by value? > > Joerg Schad wrote: > Not sure why you would want to pass either by value. Result

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/ --- (Updated Sept. 23, 2015, 12:37 p.m.) Review request for mesos, Bernd Mathiske,

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review100239 --- Ship it! I've submitted this review request with the following

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

> On Sept. 9, 2015, 3:46 p.m., Alexander Rukletsov wrote: > > src/common/values.cpp, lines 250-271 > > > > > > I think we can conflate this two cases, how about this: > > ``` > > if (range->end() + 1 >=

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

> On Sept. 23, 2015, 6:29 p.m., Ben Mahler wrote: > > Where is the benchmark for this change? :( We benchmarked the code with a proprietary benchmark. I added https://issues.apache.org/jira/browse/MESOS-3502 aiming at adding an open-source benchmark for this. - Joerg

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review100261 --- Where is the benchmark for this change? :( - Ben Mahler On

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

> On Sept. 23, 2015, 12:58 a.m., Michael Park wrote: > > src/common/values.cpp, line 204 > > > > > > Why not pass by value? Not sure why you would want to pass either by value. Result we want to use for the

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

> On Sept. 23, 2015, 12:58 a.m., Michael Park wrote: > > src/common/values.cpp, line 301 > > > > > > Can we just take a `Value::Ranges` here rather than > > `initializer_list`? > > > > It looks like the

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review100114 --- src/common/values.cpp (lines 141 - 154)

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review100091 --- I've got some clean-up suggestions, I think `lhs` and `rhs`

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review100119 --- Ship it! After addressing the outstanding issues. - Joris Van

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/ --- (Updated Sept. 22, 2015, 8:55 p.m.) Review request for mesos, Bernd Mathiske,

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review99491 --- Patch looks great! Reviews applied: [38158] All tests passed. -

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/ --- (Updated Sept. 18, 2015, 6:39 a.m.) Review request for mesos, Bernd Mathiske,

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review99500 --- Patch looks great! Reviews applied: [38158] All tests passed. -

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/ --- (Updated Sept. 17, 2015, 11:34 p.m.) Review request for mesos, Bernd Mathiske

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/ --- (Updated Sept. 18, 2015, 5:17 a.m.) Review request for mesos, Bernd Mathiske

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review99124 --- Patch looks great! Reviews applied: [38158] All tests passed. -

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/ --- (Updated Sept. 15, 2015, 8:46 p.m.) Review request for mesos, Bernd Mathiske

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

> On Sept. 7, 2015, 9:24 p.m., Alexander Rukletsov wrote: > > src/common/values.cpp, lines 442-444 > > > > > > Let's let compiler do it's job: how about passing `left` by value and > > not creating a copy manually?

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

> On Sept. 9, 2015, 3:46 p.m., Alexander Rukletsov wrote: > > src/common/values.cpp, lines 250-271 > > > > > > I think we can conflate this two cases, how about this: > > ``` > > if (range->end() + 1 >=

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review98562 --- Patch looks great! Reviews applied: [38158] All tests passed. -

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/ --- (Updated Sept. 10, 2015, 6:15 p.m.) Review request for mesos, Bernd Mathiske

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review98200 --- include/mesos/values.hpp (line 21)

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

> On Sept. 10, 2015, 2:48 a.m., Till Toenshoff wrote: > > Looks really good to me, thanks Joerg. Just noticed the tests fail at a point that seems relevant to this code - please make sure you can recreate the buildbot failure and then fix it please :) - Till

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review97983 --- Ship it! Looks really good to me, thanks Joerg.

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/ --- (Updated Sept. 9, 2015, 8:48 p.m.) Review request for mesos, Bernd Mathiske

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/ --- (Updated Sept. 8, 2015, 7:25 p.m.) Review request for mesos, Bernd Mathiske

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review98055 --- Patch looks great! Reviews applied: [38158] All tests passed. -

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/ --- (Updated Sept. 8, 2015, 7:22 p.m.) Review request for mesos, Bernd Mathiske

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

> On Sept. 7, 2015, 9:24 p.m., Alexander Rukletsov wrote: > > src/common/values.cpp, line 231 > > > > > > An idea to re-use code here. This function boils down to inserting an > > element into a sorted range and

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

> On Sept. 7, 2015, 9:24 p.m., Alexander Rukletsov wrote: > > src/common/values.cpp, lines 277-278 > > > > > > Since you're editing here, mind explaining what an "urange" is? As the comment above states it is an

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review98009 --- Patch looks great! Reviews applied: [38158] All tests passed. -

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/ --- (Updated Sept. 8, 2015, 2:33 p.m.) Review request for mesos, Bernd Mathiske

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/ --- (Updated Sept. 7, 2015, 4:57 p.m.) Review request for mesos, Bernd Mathiske

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/ --- (Updated Sept. 7, 2015, 6:04 p.m.) Review request for mesos, Bernd Mathiske

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review97937 --- I would suggest to write a summarizing comment about how coalescing

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38158/#review97936 --- Patch looks great! Reviews applied: [38158] All tests passed. -