You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2016/01/06 00:45:23 UTC

Review Request 41950: Cleaned up hierarchical allocator tests.

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

Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.


Repository: mesos


Description
-------

Changes made:
- empty resource map promoted to a const class field;
- removed variable numeric suffixes where appropriate;
- added const where appropriate.


Diffs
-----

  src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59 

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


Testing
-------

`make check` on Mac OS 10.10.5


Thanks,

Alexander Rukletsov


Re: Review Request 41950: Cleaned up hierarchical allocator tests.

Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41950/#review113989
-----------------------------------------------------------



src/tests/hierarchical_allocator_tests.cpp (line 223)
<https://reviews.apache.org/r/41950/#comment174774>

    Should we maybe make the name more descriptive? EMPTY sounds very general....


- Joerg Schad


On Jan. 5, 2016, 11:45 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41950/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2016, 11:45 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Changes made:
> - empty resource map promoted to a const class field;
> - removed variable numeric suffixes where appropriate;
> - added const where appropriate.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59 
> 
> Diff: https://reviews.apache.org/r/41950/diff/
> 
> 
> Testing
> -------
> 
> `make check` on Mac OS 10.10.5
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


Re: Review Request 41950: Cleaned up hierarchical allocator tests.

Posted by Alex Rukletsov <al...@mesosphere.io>.
Folks,

Sorry for not communicating it properly, I should have discarded the patch
actually. As Ben already mentioned, we had agreed to rework it and maybe
split into multiple patches for clarity. However, I haven't published them
yet.

Best,
Alex
On 23 Feb 2016 18:35, "Bernd Mathiske" <be...@mesosphere.io> wrote:

>
>
> > On Feb. 23, 2016, 9:33 a.m., Ben Mahler wrote:
> > > Apologies, did you check with Alex prior to committing? We talked
> about this change recently but we didn't publish the comments, sorry that
> it wasn't clear! I reverted it for now, since we didn't really like the
> empty map constants as class members. We also needed to look into whether
> using empty initializer lists inline worked instead of this change.
>
> Alex is on vacation and I found this lingering. Since it looks a lot like
> an improvement, I went ahead after reading it all and testing it. No
> problem, we can progress further from either version.
>
>
> - Bernd
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41950/#review120338
> -----------------------------------------------------------
>
>
> On Jan. 28, 2016, 5:12 a.m., Alexander Rukletsov wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/41950/
> > -----------------------------------------------------------
> >
> > (Updated Jan. 28, 2016, 5:12 a.m.)
> >
> >
> > Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and
> Joris Van Remoortere.
> >
> >
> > Repository: mesos
> >
> >
> > Description
> > -------
> >
> > Changes made:
> > - empty resource map promoted to a const class field;
> > - removed variable numeric suffixes where appropriate;
> > - added const where appropriate.
> >
> >
> > Diffs
> > -----
> >
> >   src/tests/hierarchical_allocator_tests.cpp
> f18e6eb10572b0f5b8bbff338384d9406f6ad62b
> >
> > Diff: https://reviews.apache.org/r/41950/diff/
> >
> >
> > Testing
> > -------
> >
> > On Mac OS 10.10.4:
> >
> > `make check`
> >
> > `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> >
> >
> > Thanks,
> >
> > Alexander Rukletsov
> >
> >
>
>

Re: Review Request 41950: Cleaned up hierarchical allocator tests.

Posted by Bernd Mathiske <be...@mesosphere.io>.

> On Feb. 23, 2016, 9:33 a.m., Ben Mahler wrote:
> > Apologies, did you check with Alex prior to committing? We talked about this change recently but we didn't publish the comments, sorry that it wasn't clear! I reverted it for now, since we didn't really like the empty map constants as class members. We also needed to look into whether using empty initializer lists inline worked instead of this change.

Alex is on vacation and I found this lingering. Since it looks a lot like an improvement, I went ahead after reading it all and testing it. No problem, we can progress further from either version.


- Bernd


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


On Jan. 28, 2016, 5:12 a.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41950/
> -----------------------------------------------------------
> 
> (Updated Jan. 28, 2016, 5:12 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Changes made:
> - empty resource map promoted to a const class field;
> - removed variable numeric suffixes where appropriate;
> - added const where appropriate.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp f18e6eb10572b0f5b8bbff338384d9406f6ad62b 
> 
> Diff: https://reviews.apache.org/r/41950/diff/
> 
> 
> Testing
> -------
> 
> On Mac OS 10.10.4:
> 
> `make check` 
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


Re: Review Request 41950: Cleaned up hierarchical allocator tests.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41950/#review120338
-----------------------------------------------------------



Apologies, did you check with Alex prior to committing? We talked about this change recently but we didn't publish the comments, sorry that it wasn't clear! I reverted it for now, since we didn't really like the empty map constants as class members. We also needed to look into whether using empty initializer lists inline worked instead of this change.

- Ben Mahler


On Jan. 28, 2016, 1:12 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41950/
> -----------------------------------------------------------
> 
> (Updated Jan. 28, 2016, 1:12 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Changes made:
> - empty resource map promoted to a const class field;
> - removed variable numeric suffixes where appropriate;
> - added const where appropriate.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp f18e6eb10572b0f5b8bbff338384d9406f6ad62b 
> 
> Diff: https://reviews.apache.org/r/41950/diff/
> 
> 
> Testing
> -------
> 
> On Mac OS 10.10.4:
> 
> `make check` 
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


Re: Review Request 41950: Cleaned up hierarchical allocator tests.

Posted by Klaus Ma <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41950/#review119268
-----------------------------------------------------------


Ship it!




Ship It!

- Klaus Ma


On Jan. 28, 2016, 9:12 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41950/
> -----------------------------------------------------------
> 
> (Updated Jan. 28, 2016, 9:12 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Changes made:
> - empty resource map promoted to a const class field;
> - removed variable numeric suffixes where appropriate;
> - added const where appropriate.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp f18e6eb10572b0f5b8bbff338384d9406f6ad62b 
> 
> Diff: https://reviews.apache.org/r/41950/diff/
> 
> 
> Testing
> -------
> 
> On Mac OS 10.10.4:
> 
> `make check` 
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


Re: Review Request 41950: Cleaned up hierarchical allocator tests.

Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41950/
-----------------------------------------------------------

(Updated Jan. 28, 2016, 1:12 p.m.)


Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris Van Remoortere.


Changes
-------

Rebased.


Repository: mesos


Description
-------

Changes made:
- empty resource map promoted to a const class field;
- removed variable numeric suffixes where appropriate;
- added const where appropriate.


Diffs (updated)
-----

  src/tests/hierarchical_allocator_tests.cpp f18e6eb10572b0f5b8bbff338384d9406f6ad62b 

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


Testing
-------

On Mac OS 10.10.4:

`make check` 

`GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`


Thanks,

Alexander Rukletsov


Re: Review Request 41950: Cleaned up hierarchical allocator tests.

Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41950/
-----------------------------------------------------------

(Updated Jan. 22, 2016, 10:51 p.m.)


Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris Van Remoortere.


Changes
-------

Rebased; more cleanups.


Repository: mesos


Description
-------

Changes made:
- empty resource map promoted to a const class field;
- removed variable numeric suffixes where appropriate;
- added const where appropriate.


Diffs (updated)
-----

  src/tests/hierarchical_allocator_tests.cpp b1cb955b7eb1213c7ba4a9c5181545bb49154f06 

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


Testing (updated)
-------

On Mac OS 10.10.4:

`make check` 

`GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`


Thanks,

Alexander Rukletsov


Re: Review Request 41950: Cleaned up hierarchical allocator tests.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41950/#review115781
-----------------------------------------------------------


Bad patch!

Reviews applied: [41950]

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

Error:
 2016-01-22 03:35:47 URL:https://reviews.apache.org/r/41950/diff/raw/ [24032/24032] -> "41950.patch" [1]
error: patch failed: src/tests/hierarchical_allocator_tests.cpp:1803
error: src/tests/hierarchical_allocator_tests.cpp: patch does not apply

- Mesos ReviewBot


On Jan. 22, 2016, 1:20 a.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41950/
> -----------------------------------------------------------
> 
> (Updated Jan. 22, 2016, 1:20 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Changes made:
> - empty resource map promoted to a const class field;
> - removed variable numeric suffixes where appropriate;
> - added const where appropriate.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59 
> 
> Diff: https://reviews.apache.org/r/41950/diff/
> 
> 
> Testing
> -------
> 
> `make check` on Mac OS 10.10.5
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


Re: Review Request 41950: Cleaned up hierarchical allocator tests.

Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41950/
-----------------------------------------------------------

(Updated Jan. 22, 2016, 1:20 a.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.


Repository: mesos


Description
-------

Changes made:
- empty resource map promoted to a const class field;
- removed variable numeric suffixes where appropriate;
- added const where appropriate.


Diffs
-----

  src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59 

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


Testing
-------

`make check` on Mac OS 10.10.5


Thanks,

Alexander Rukletsov


Re: Review Request 41950: Cleaned up hierarchical allocator tests.

Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41950/#review115382
-----------------------------------------------------------

Ship it!


Ship It!

- Bernd Mathiske


On Jan. 12, 2016, 1:34 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41950/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2016, 1:34 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Changes made:
> - empty resource map promoted to a const class field;
> - removed variable numeric suffixes where appropriate;
> - added const where appropriate.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59 
> 
> Diff: https://reviews.apache.org/r/41950/diff/
> 
> 
> Testing
> -------
> 
> `make check` on Mac OS 10.10.5
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


Re: Review Request 41950: Cleaned up hierarchical allocator tests.

Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41950/#review114931
-----------------------------------------------------------

Ship it!


Ship It!

- Guangya Liu


On 一月 12, 2016, 9:34 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41950/
> -----------------------------------------------------------
> 
> (Updated 一月 12, 2016, 9:34 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Changes made:
> - empty resource map promoted to a const class field;
> - removed variable numeric suffixes where appropriate;
> - added const where appropriate.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59 
> 
> Diff: https://reviews.apache.org/r/41950/diff/
> 
> 
> Testing
> -------
> 
> `make check` on Mac OS 10.10.5
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


Re: Review Request 41950: Cleaned up hierarchical allocator tests.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41950/#review114937
-----------------------------------------------------------


Patch looks great!

Reviews applied: [41936, 41937, 41947, 41948, 41938, 41939, 41949, 41648, 41950]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 12, 2016, 9:34 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41950/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2016, 9:34 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Changes made:
> - empty resource map promoted to a const class field;
> - removed variable numeric suffixes where appropriate;
> - added const where appropriate.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59 
> 
> Diff: https://reviews.apache.org/r/41950/diff/
> 
> 
> Testing
> -------
> 
> `make check` on Mac OS 10.10.5
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


Re: Review Request 41950: Cleaned up hierarchical allocator tests.

Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41950/
-----------------------------------------------------------

(Updated Jan. 12, 2016, 9:34 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.


Changes
-------

Rebased; addressed issues.


Repository: mesos


Description
-------

Changes made:
- empty resource map promoted to a const class field;
- removed variable numeric suffixes where appropriate;
- added const where appropriate.


Diffs (updated)
-----

  src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59 

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


Testing
-------

`make check` on Mac OS 10.10.5


Thanks,

Alexander Rukletsov


Re: Review Request 41950: Cleaned up hierarchical allocator tests.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41950/#review114003
-----------------------------------------------------------


Bad patch!

Reviews applied: [41936]

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

Error:
 2016-01-12 16:30:26 URL:https://reviews.apache.org/r/41936/diff/raw/ [23873/23873] -> "41936.patch" [1]
error: patch failed: src/tests/master_quota_tests.cpp:1088
error: src/tests/master_quota_tests.cpp: patch does not apply

- Mesos ReviewBot


On Jan. 5, 2016, 11:45 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41950/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2016, 11:45 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Changes made:
> - empty resource map promoted to a const class field;
> - removed variable numeric suffixes where appropriate;
> - added const where appropriate.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59 
> 
> Diff: https://reviews.apache.org/r/41950/diff/
> 
> 
> Testing
> -------
> 
> `make check` on Mac OS 10.10.5
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>