mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 31265: Provided a factory for allocator in tests.
Date Sun, 19 Apr 2015 17:30:24 GMT


> On April 17, 2015, 1:31 p.m., Michael Park wrote:
> > src/local/local.cpp, lines 141-145
> > <https://reviews.apache.org/r/31265/diff/8/?file=929832#file929832line141>
> >
> >     What's the point of setting `_allocator` here? It looks like it only gets passed
to the `new Master(...)` call.
> >     
> >     Can we just leave `_allocator` alone and do
> >     
> >     ```
> >     if (allocator_.isError()) {
> >       LOG(ERROR) << "Failed to create an instance of HierarchicalDRFAllocator:
"
> >                  << allocator_.error();
> >       allocator = NULL;
> >     }
> >     
> >     // Save the instance for deleting later.
> >     allocator = allocator_.get();
> >     ```
> >     
> >     then pass `allocator` to `new Master` below?

`_allocator` is passed by non-const pointer, so theoretically we can modify caller's data.
I can't find any code where this is the case, but I still don't feel like doing this drive-by
change. Let's create a separate JIRA if you feel this should be fixed.


> On April 17, 2015, 1:31 p.m., Michael Park wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 77
> > <https://reviews.apache.org/r/31265/diff/8/?file=929836#file929836line77>
> >
> >     I see this is the place where you wanted to use `ASSERT_SOME` but couldn't due
to the [gtest limitation](https://code.google.com/p/googletest/wiki/AdvancedGuide#Assertion_Placement).
> >     
> >     One way to solve this I think is to use `SetUp/TearDown` rather than `Ctor/Dtor`.
> >     Another way may be to use `CHECK_SOME` in place of `ASSERT_SOME`. I think this
makes sense as well since success of `HierarchicalDRFAllocator::create` isn't actually a testing
condition.
> >     
> >     Thoughts?
> 
> Michael Park wrote:
>     Actually, is it possible just use a `TestAllocator` constructed from `TestAllocator::create<>()`
as suggested below?

Good idea!


- Alexander


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


On April 15, 2015, 2:19 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31265/
> -----------------------------------------------------------
> 
> (Updated April 15, 2015, 2:19 p.m.)
> 
> 
> Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2160
>     https://issues.apache.org/jira/browse/MESOS-2160
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The factory creates allocator instances in a way identical to how instances from modules
are created. It allows us to use same typed tests for built-in and modularized allocators.
> 
> 
> Diffs
> -----
> 
>   src/examples/test_allocator_module.cpp PRE-CREATION 
>   src/local/local.cpp 289b9bced7bab0d745fe14823efa4e90ec36905e 
>   src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f 
>   src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f 
>   src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec 
>   src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef

>   src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 
>   src/tests/mesos.hpp 42e42ac425a448fcc5e93db1cef1112cbf5e67c4 
> 
> Diff: https://reviews.apache.org/r/31265/diff/
> 
> 
> Testing
> -------
> 
> make check (Mac OS 10.9.5, Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message