You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2018/10/20 01:28:31 UTC
Review Request 69096: Moved a few allocator test helpers to
`tests/allocator.hpp`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69096/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler and Gastón Kleiman.
Repository: mesos
Description
-------
This helps to share the helpers between
`hierarchical_allocator_tests.cpp` and
`hierarchical_allocator_benchmarks.cpp`.
Diffs
-----
src/Makefile.am 5795c7097a9ed1f659e169ad81a9f2c09481aa81
src/tests/CMakeLists.txt 00dbdee1c06e7571fa799850cdaf7f2ccadc8bea
src/tests/allocator.hpp 15a5396986ea6939f25b8d23b87a91c27338fc7b
src/tests/allocator.cpp PRE-CREATION
src/tests/hierarchical_allocator_tests.cpp 27fbd9cf0c4442e7675362a806d35bad141ffb6d
Diff: https://reviews.apache.org/r/69096/diff/1/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 69096: Moved a few allocator test helpers to
`tests/allocator.hpp`.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Oct. 24, 2018, 5:22 p.m., Benjamin Mahler wrote:
> > src/tests/allocator.hpp
> > Lines 40-45 (patched)
> > <https://reviews.apache.org/r/69096/diff/1/?file=2101109#file2101109line40>
> >
> > I think we some other create helpers lying around, e.g. createTask. Is this where these belong?
> >
> > Do you think we need them?
>
> Meng Zhu wrote:
> I do not have use cases of other helpers in mind atm. Thus I prefer to pull in only these two.
>
> Benjamin Mahler wrote:
> Do we even need these helpers? They seem pretty trivial, and I wonder how much code it's actually simplifying. I'm fine with the patch, but it feels heavy to have a new header and compliation unit just for these, especially since they're only used in two files and are trivial.
Yeah, we may do these two inline or just copy them around. But I expect that the allocator tests and benchmarks would share more utilities in the future (after all, they are all allocator tests). So might as well set up the room for it. If it makes the code structure cleaner, I think it is worth it.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69096/#review210010
-----------------------------------------------------------
On Oct. 19, 2018, 6:28 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69096/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2018, 6:28 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This helps to share the helpers between
> `hierarchical_allocator_tests.cpp` and
> `hierarchical_allocator_benchmarks.cpp`.
>
>
> Diffs
> -----
>
> src/Makefile.am 5795c7097a9ed1f659e169ad81a9f2c09481aa81
> src/tests/CMakeLists.txt 00dbdee1c06e7571fa799850cdaf7f2ccadc8bea
> src/tests/allocator.hpp 15a5396986ea6939f25b8d23b87a91c27338fc7b
> src/tests/allocator.cpp PRE-CREATION
> src/tests/hierarchical_allocator_tests.cpp 27fbd9cf0c4442e7675362a806d35bad141ffb6d
>
>
> Diff: https://reviews.apache.org/r/69096/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69096: Moved a few allocator test helpers to
`tests/allocator.hpp`.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Oct. 25, 2018, 12:22 a.m., Benjamin Mahler wrote:
> > src/tests/allocator.hpp
> > Lines 40-45 (patched)
> > <https://reviews.apache.org/r/69096/diff/1/?file=2101109#file2101109line40>
> >
> > I think we some other create helpers lying around, e.g. createTask. Is this where these belong?
> >
> > Do you think we need them?
>
> Meng Zhu wrote:
> I do not have use cases of other helpers in mind atm. Thus I prefer to pull in only these two.
Do we even need these helpers? They seem pretty trivial, and I wonder how much code it's actually simplifying. I'm fine with the patch, but it feels heavy to have a new header and compliation unit just for these, especially since they're only used in two files and are trivial.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69096/#review210010
-----------------------------------------------------------
On Oct. 20, 2018, 1:28 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69096/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2018, 1:28 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This helps to share the helpers between
> `hierarchical_allocator_tests.cpp` and
> `hierarchical_allocator_benchmarks.cpp`.
>
>
> Diffs
> -----
>
> src/Makefile.am 5795c7097a9ed1f659e169ad81a9f2c09481aa81
> src/tests/CMakeLists.txt 00dbdee1c06e7571fa799850cdaf7f2ccadc8bea
> src/tests/allocator.hpp 15a5396986ea6939f25b8d23b87a91c27338fc7b
> src/tests/allocator.cpp PRE-CREATION
> src/tests/hierarchical_allocator_tests.cpp 27fbd9cf0c4442e7675362a806d35bad141ffb6d
>
>
> Diff: https://reviews.apache.org/r/69096/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69096: Moved a few allocator test helpers to
`tests/allocator.hpp`.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Oct. 24, 2018, 5:22 p.m., Benjamin Mahler wrote:
> > src/tests/allocator.hpp
> > Lines 40-45 (patched)
> > <https://reviews.apache.org/r/69096/diff/1/?file=2101109#file2101109line40>
> >
> > I think we some other create helpers lying around, e.g. createTask. Is this where these belong?
> >
> > Do you think we need them?
I do not have use cases of other helpers in mind atm. Thus I prefer to pull in only these two.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69096/#review210010
-----------------------------------------------------------
On Oct. 19, 2018, 6:28 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69096/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2018, 6:28 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This helps to share the helpers between
> `hierarchical_allocator_tests.cpp` and
> `hierarchical_allocator_benchmarks.cpp`.
>
>
> Diffs
> -----
>
> src/Makefile.am 5795c7097a9ed1f659e169ad81a9f2c09481aa81
> src/tests/CMakeLists.txt 00dbdee1c06e7571fa799850cdaf7f2ccadc8bea
> src/tests/allocator.hpp 15a5396986ea6939f25b8d23b87a91c27338fc7b
> src/tests/allocator.cpp PRE-CREATION
> src/tests/hierarchical_allocator_tests.cpp 27fbd9cf0c4442e7675362a806d35bad141ffb6d
>
>
> Diff: https://reviews.apache.org/r/69096/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69096: Moved a few allocator test helpers to
`tests/allocator.hpp`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69096/#review210010
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/allocator.hpp
Lines 40-45 (patched)
<https://reviews.apache.org/r/69096/#comment294645>
I think we some other create helpers lying around, e.g. createTask. Is this where these belong?
Do you think we need them?
- Benjamin Mahler
On Oct. 20, 2018, 1:28 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69096/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2018, 1:28 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This helps to share the helpers between
> `hierarchical_allocator_tests.cpp` and
> `hierarchical_allocator_benchmarks.cpp`.
>
>
> Diffs
> -----
>
> src/Makefile.am 5795c7097a9ed1f659e169ad81a9f2c09481aa81
> src/tests/CMakeLists.txt 00dbdee1c06e7571fa799850cdaf7f2ccadc8bea
> src/tests/allocator.hpp 15a5396986ea6939f25b8d23b87a91c27338fc7b
> src/tests/allocator.cpp PRE-CREATION
> src/tests/hierarchical_allocator_tests.cpp 27fbd9cf0c4442e7675362a806d35bad141ffb6d
>
>
> Diff: https://reviews.apache.org/r/69096/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>