Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-02 Thread Bartek Plotka
On June 2, 2015, 5:05 p.m., Jie Yu wrote: src/tests/oversubscription_tests.cpp, lines 77-80 https://reviews.apache.org/r/34816/diff/7/?file=976044#file976044line77 Any reason change the comments and the test name here? Sorry - mistake during fixing git merge conflicts - Bartek

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-02 Thread Bartek Plotka
On June 2, 2015, 4:24 p.m., Niklas Nielsen wrote: src/tests/mesos.hpp, line 797 https://reviews.apache.org/r/34816/diff/7/?file=976043#file976043line797 How will this work, when we can't access the mocked resource estimator from the test body? You were right - default

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-02 Thread Bartek Plotka
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/ --- (Updated June 2, 2015, 7:37 p.m.) Review request for mesos, Jie Yu, Niklas

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-02 Thread Bartek Plotka
On June 2, 2015, 8:05 p.m., Vinod Kone wrote: src/tests/mesos.hpp, lines 716-723 https://reviews.apache.org/r/34816/diff/8/?file=976766#file976766line716 You want to use both ON_CALL and EXPECT_CALL. See TestAllocator in tests/mesos.hpp for the rationale. +1 However, i can see

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-02 Thread Niklas Nielsen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/#review86283 --- Ship it! src/tests/mesos.hpp

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-02 Thread Bartek Plotka
On June 2, 2015, 9:08 p.m., Niklas Nielsen wrote: src/tests/oversubscription_tests.cpp, line 91 https://reviews.apache.org/r/34816/diff/9/?file=976795#file976795line91 This is implicit and you can remove it :) Here and below oh, sure (: - Bartek

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-02 Thread Vinod Kone
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/#review86271 --- src/tests/mesos.hpp

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-02 Thread Bartek Plotka
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/ --- (Updated June 2, 2015, 8:36 p.m.) Review request for mesos, Jie Yu, Niklas

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-02 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/#review86289 --- Ship it! Ship It! - Jie Yu On June 2, 2015, 9:19 p.m., Bartek

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-02 Thread Bartek Plotka
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/ --- (Updated June 2, 2015, 9:19 p.m.) Review request for mesos, Jie Yu, Niklas

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-02 Thread Niklas Nielsen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/#review86235 --- Looks good! Final questions, tiny nits and let's get this in!

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-02 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/#review86246 --- src/tests/oversubscription_tests.cpp

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-01 Thread Bartek Plotka
On June 1, 2015, 7:34 p.m., Jie Yu wrote: src/tests/oversubscription_tests.cpp, line 96 https://reviews.apache.org/r/34816/diff/4/?file=975492#file975492line96 This looks weired to me since we are actually creating a TestResourceEstimator, but TypeParam == NoopResourceEstimator.

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-01 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/#review86051 --- src/tests/oversubscription_tests.cpp

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-01 Thread Niklas Nielsen
On June 1, 2015, 12:34 p.m., Jie Yu wrote: src/tests/oversubscription_tests.cpp, line 96 https://reviews.apache.org/r/34816/diff/4/?file=975492#file975492line96 This looks weired to me since we are actually creating a TestResourceEstimator, but TypeParam == NoopResourceEstimator.

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-01 Thread Niklas Nielsen
On June 1, 2015, 1:46 p.m., Niklas Nielsen wrote: src/tests/oversubscription_tests.cpp, lines 225-226 https://reviews.apache.org/r/34816/diff/4/?file=975492#file975492line225 We can merge these two with multiple put() on a queue :) Bartek Plotka wrote: That's true, but in

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-01 Thread Niklas Nielsen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/#review86066 --- src/tests/oversubscription_tests.cpp

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-01 Thread Bartek Plotka
On June 1, 2015, 8:46 p.m., Niklas Nielsen wrote: src/tests/oversubscription_tests.cpp, lines 225-226 https://reviews.apache.org/r/34816/diff/4/?file=975492#file975492line225 We can merge these two with multiple put() on a queue :) That's true, but in previous issue you asked to

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-01 Thread Bartek Plotka
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/ --- (Updated June 2, 2015, 12:02 a.m.) Review request for mesos, Jie Yu, Niklas

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-01 Thread Bartek Plotka
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/ --- (Updated June 2, 2015, 12:20 a.m.) Review request for mesos, Jie Yu, Niklas

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-06-01 Thread Bartek Plotka
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/ --- (Updated June 2, 2015, 12:38 a.m.) Review request for mesos, Jie Yu, Niklas

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-05-31 Thread Bartek Plotka
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/ --- (Updated June 1, 2015, 5:37 a.m.) Review request for mesos, Jie Yu, Niklas

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-05-31 Thread Bartek Plotka
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/ --- (Updated June 1, 2015, 5:34 a.m.) Review request for mesos, Jie Yu, Niklas

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-05-29 Thread Bartek Plotka
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/ --- (Updated May 29, 2015, 6:18 p.m.) Review request for mesos, Jie Yu, Niklas

Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-05-29 Thread Bartek Plotka
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/ --- Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-05-29 Thread Niklas Nielsen
On May 29, 2015, 1:28 p.m., Niklas Nielsen wrote: src/tests/mesos.hpp, line 704 https://reviews.apache.org/r/34816/diff/2/?file=974402#file974402line704 Why are we specializing it for the NoopResourceEstimator here? Bartek Plotka wrote: To have default value for that

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-05-29 Thread Vinod Kone
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/#review85806 --- you need to rebase this on the latest master src/tests/mesos.hpp

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-05-29 Thread Niklas Nielsen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34816/#review85795 --- src/tests/mesos.hpp

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-05-29 Thread Bartek Plotka
On May 29, 2015, 8:28 p.m., Niklas Nielsen wrote: src/tests/mesos.hpp, line 704 https://reviews.apache.org/r/34816/diff/2/?file=974402#file974402line704 Why are we specializing it for the NoopResourceEstimator here? To have default value for that typename (: It has no use in fact

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-05-29 Thread Bartek Plotka
On May 29, 2015, 8:50 p.m., Vinod Kone wrote: src/tests/mesos.hpp, line 716 https://reviews.apache.org/r/34816/diff/2/?file=974402#file974402line716 s/Mock/Test/ since this wraps a real estimator. If i'll do that, we will have a conflict with enum ModuleID.TestResourceEstimator in

Re: Review Request 34816: Changed TestResourceEstimator to be mocked.

2015-05-29 Thread Bartek Plotka
On May 29, 2015, 8:28 p.m., Niklas Nielsen wrote: src/tests/mesos.hpp, line 710 https://reviews.apache.org/r/34816/diff/2/?file=974402#file974402line710 Couldn't we drop 'Optionstd::string()' or replace with None()? ResourceEstimator was defined to have create(Optionstring