You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Bartek Plotka <bw...@gmail.com> on 2015/06/06 01:53:22 UTC
Re: Review Request 35157: Added unit tests for fetching ResoruceUsage
in both QoS Controller and Resource Estimator .
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/
-----------------------------------------------------------
(Updated June 5, 2015, 11:53 p.m.)
Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
Added QoS Controller test.
Summary (updated)
-----------------
Added unit tests for fetching ResoruceUsage in both QoS Controller and Resource Estimator .
Repository: mesos
Description (updated)
-------
Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
Diffs (updated)
-----
src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7
Diff: https://reviews.apache.org/r/35157/diff/
Testing
-------
make check
Thanks,
Bartek Plotka
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Bartek Plotka <bw...@gmail.com>.
> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 582
> > <https://reviews.apache.org/r/35157/diff/3/?file=980318#file980318line582>
> >
> > Why copy the offer?
Because we use it several time, however i didn't notice that it wasn't consistent - sometimes i used *offer*, sometimes *offer.get()[0]*. Fixed now.
- Bartek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/#review87074
-----------------------------------------------------------
On June 9, 2015, 12:17 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 12:17 a.m.)
>
>
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
>
> This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp 52380c2461841026ee492797b4d8081f944f7b7b
> src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7
>
> Diff: https://reviews.apache.org/r/35157/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Bartek Plotka <bw...@gmail.com>.
> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 256
> > <https://reviews.apache.org/r/35157/diff/3/?file=980318#file980318line256>
> >
> > Why not await ready? Here and below :)
hmm because when we receive status 'running' from task, then we are pretty sure that Resource Estimator/QoS Controller is initialized (: or rather.. we want to make sure (:
> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/mesos.hpp, lines 188-201
> > <https://reviews.apache.org/r/35157/diff/3/?file=980316#file980316line188>
> >
> > The StartSlave overload is growing a bit out of hand. Let's create a JIRA so we get those consolidated somehow. Can you add the ticket as a comment? :)
MESOS-2833
> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 209
> > <https://reviews.apache.org/r/35157/diff/3/?file=980318#file980318line209>
> >
> > Can you inline this below? (Is it used other places?) If not, should we const it? Here and below
Added inline if possible, otherwise const. Added const also for the other variables which are not being changed during test.
> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, lines 234-238
> > <https://reviews.apache.org/r/35157/diff/3/?file=980318#file980318line234>
> >
> > Have you looked into, whether you can use createTask() instead?
> >
> > Here and below
Good idea - mimized from e.g master_test.
> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 550
> > <https://reviews.apache.org/r/35157/diff/3/?file=980318#file980318line550>
> >
> > We tend to use the shorter form, if there is no ambguity (controller instead of 'qoSController' :) I'll leave that up to you, if you want to change it
+1
Done.
> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 552
> > <https://reviews.apache.org/r/35157/diff/3/?file=980318#file980318line552>
> >
> > Let's introduce a 'using std::list;' above :)
Actually - it was (: But i did not realize that, thx.
> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 618
> > <https://reviews.apache.org/r/35157/diff/3/?file=980318#file980318line618>
> >
> > Let's get the operator overload wired up and do ASSERT_EQ() :)
Done - in type_utils
- Bartek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/#review87074
-----------------------------------------------------------
On June 9, 2015, 12:17 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 12:17 a.m.)
>
>
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
>
> This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp 52380c2461841026ee492797b4d8081f944f7b7b
> src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7
>
> Diff: https://reviews.apache.org/r/35157/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/#review87074
-----------------------------------------------------------
src/tests/mesos.hpp
<https://reviews.apache.org/r/35157/#comment139349>
The StartSlave overload is growing a bit out of hand. Let's create a JIRA so we get those consolidated somehow. Can you add the ticket as a comment? :)
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139350>
Can't you compare the message objects instead? If there isn't a == operator overload - let's add one :)
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139381>
Can you inline this below? (Is it used other places?) If not, should we const it? Here and below
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139382>
Have you looked into, whether you can use createTask() instead?
Here and below
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139380>
Why not await ready? Here and below :)
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139370>
const?
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139372>
We tend to use the shorter form, if there is no ambguity (controller instead of 'qoSController' :) I'll leave that up to you, if you want to change it
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139369>
Let's introduce a 'using std::list;' above :)
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139375>
Why copy the offer?
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139378>
Strip 'std::' :)
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139377>
Let's get the operator overload wired up and do ASSERT_EQ() :)
- Niklas Nielsen
On June 8, 2015, 12:41 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 12:41 p.m.)
>
>
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
>
> This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7
>
> Diff: https://reviews.apache.org/r/35157/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/#review87613
-----------------------------------------------------------
Bad patch!
Reviews applied: [35164, 35157]
Failed command: ./support/apply-review.sh -n -r 35157
Error:
2015-06-11 19:41:24 URL:https://reviews.apache.org/r/35157/diff/raw/ [18101/18101] -> "35157.patch" [1]
error: patch failed: src/tests/oversubscription_tests.cpp:63
error: src/tests/oversubscription_tests.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On June 11, 2015, 6:55 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> -----------------------------------------------------------
>
> (Updated June 11, 2015, 6:55 p.m.)
>
>
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
>
> This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp 52380c2461841026ee492797b4d8081f944f7b7b
> src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7
>
> Diff: https://reviews.apache.org/r/35157/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/#review87609
-----------------------------------------------------------
src/common/type_utils.cpp
<https://reviews.apache.org/r/35157/#comment140005>
Wow. This will become very hard to maintian (i.e., keep up-to-date) as we introduce more and more statistics (e.g., https://reviews.apache.org/r/34894).
So could you please serialize the protobuf to string and test string equality since you want the strict equality here.
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment140006>
Can you align FutureSatisfy and Return? Here and everywhere else.
- Jie Yu
On June 11, 2015, 6:55 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> -----------------------------------------------------------
>
> (Updated June 11, 2015, 6:55 p.m.)
>
>
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
>
> This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp 52380c2461841026ee492797b4d8081f944f7b7b
> src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7
>
> Diff: https://reviews.apache.org/r/35157/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/#review87725
-----------------------------------------------------------
src/tests/mesos.hpp
<https://reviews.apache.org/r/35157/#comment140137>
4 spaces indentation for function arguments.
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment140141>
Insert a new line above.
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment140139>
No need to have this temporary variable. Just use
```
driver.launchTasks(offers.get()[0].id(), {task});
```
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment140142>
We usually put `.WillOnce` in a new line so that it's more readable.
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment140144>
s/resourceUsages/usage/
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment140140>
Kill this line.
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment140143>
We typically put constant in front of the actual variable:
```
ASSERT_EQ(1, usage.get().executors_size());
```
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment140145>
You should be able to use
```
usage.executors(0).executor_info().executor_id()
```
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment140146>
Ditto here.
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment140147>
Please follow the similar comments above for the resource estimator test.
- Jie Yu
On June 12, 2015, 12:21 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> -----------------------------------------------------------
>
> (Updated June 12, 2015, 12:21 a.m.)
>
>
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
>
> This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp 65668a6ba8524a3bfd241f72302f64f408163540
> src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
> src/tests/mesos.hpp e19ef984f9e4696bd405027d6f19756cf23d0df2
> src/tests/mesos.cpp 5e574c5aa79d548e5730db021b0e8b77d27f220b
> src/tests/oversubscription_tests.cpp e8ae053dd9cd712e49bd2830e414b7a3d99c20ca
>
> Diff: https://reviews.apache.org/r/35157/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/#review87755
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On June 12, 2015, 8:40 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> -----------------------------------------------------------
>
> (Updated June 12, 2015, 8:40 p.m.)
>
>
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
>
> This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp 65668a6ba8524a3bfd241f72302f64f408163540
> src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
> src/tests/mesos.hpp e19ef984f9e4696bd405027d6f19756cf23d0df2
> src/tests/mesos.cpp 5e574c5aa79d548e5730db021b0e8b77d27f220b
> src/tests/oversubscription_tests.cpp e8ae053dd9cd712e49bd2830e414b7a3d99c20ca
>
> Diff: https://reviews.apache.org/r/35157/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/
-----------------------------------------------------------
(Updated June 12, 2015, 8:40 p.m.)
Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
Issues addressed.
Repository: mesos
Description
-------
Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
Diffs (updated)
-----
include/mesos/type_utils.hpp 65668a6ba8524a3bfd241f72302f64f408163540
src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
src/tests/mesos.hpp e19ef984f9e4696bd405027d6f19756cf23d0df2
src/tests/mesos.cpp 5e574c5aa79d548e5730db021b0e8b77d27f220b
src/tests/oversubscription_tests.cpp e8ae053dd9cd712e49bd2830e414b7a3d99c20ca
Diff: https://reviews.apache.org/r/35157/diff/
Testing
-------
make check
Thanks,
Bartek Plotka
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/#review87668
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35164, 35157]
All tests passed.
- Mesos ReviewBot
On June 12, 2015, 12:21 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> -----------------------------------------------------------
>
> (Updated June 12, 2015, 12:21 a.m.)
>
>
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
>
> This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp 65668a6ba8524a3bfd241f72302f64f408163540
> src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
> src/tests/mesos.hpp e19ef984f9e4696bd405027d6f19756cf23d0df2
> src/tests/mesos.cpp 5e574c5aa79d548e5730db021b0e8b77d27f220b
> src/tests/oversubscription_tests.cpp e8ae053dd9cd712e49bd2830e414b7a3d99c20ca
>
> Diff: https://reviews.apache.org/r/35157/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/
-----------------------------------------------------------
(Updated June 12, 2015, 12:21 a.m.)
Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
Issues addressed. Rebased master (rabased JieYou's ResourceUsage changes)
Repository: mesos
Description
-------
Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
Diffs (updated)
-----
include/mesos/type_utils.hpp 65668a6ba8524a3bfd241f72302f64f408163540
src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
src/tests/mesos.hpp e19ef984f9e4696bd405027d6f19756cf23d0df2
src/tests/mesos.cpp 5e574c5aa79d548e5730db021b0e8b77d27f220b
src/tests/oversubscription_tests.cpp e8ae053dd9cd712e49bd2830e414b7a3d99c20ca
Diff: https://reviews.apache.org/r/35157/diff/
Testing
-------
make check
Thanks,
Bartek Plotka
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/
-----------------------------------------------------------
(Updated June 11, 2015, 6:55 p.m.)
Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
Issue addressed.
Repository: mesos
Description
-------
Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
Diffs (updated)
-----
include/mesos/type_utils.hpp 52380c2461841026ee492797b4d8081f944f7b7b
src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7
Diff: https://reviews.apache.org/r/35157/diff/
Testing
-------
make check
Thanks,
Bartek Plotka
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/#review87587
-----------------------------------------------------------
src/common/type_utils.cpp
<https://reviews.apache.org/r/35157/#comment139978>
This doesn't match all fields in include/mesos/mesos.proto. Mind adding those?
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139981>
s/that /that the/
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139982>
s/from /from the/
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139983>
Can you do this with an initializer list?
```
vector<TaskInfo> tasks = {task};
```
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139984>
Don't you want to ensure that the status update is actually TASK_RUNNING? :)
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139980>
Let's not do these fly by changes :)
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139985>
s/that /that the/
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139986>
s/from /from the /
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/35157/#comment139987>
We need to update this to match the test above
- Niklas Nielsen
On June 8, 2015, 5:17 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 5:17 p.m.)
>
>
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
>
> This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp 52380c2461841026ee492797b4d8081f944f7b7b
> src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7
>
> Diff: https://reviews.apache.org/r/35157/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/#review87115
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35164, 35157]
All tests passed.
- Mesos ReviewBot
On June 9, 2015, 12:17 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 12:17 a.m.)
>
>
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
>
> This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp 52380c2461841026ee492797b4d8081f944f7b7b
> src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7
>
> Diff: https://reviews.apache.org/r/35157/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Bartek Plotka <bw...@gmail.com>.
> On June 9, 2015, 5:26 p.m., Niklas Nielsen wrote:
> > Hey Bartek,
> >
> > Let's await further work on this ticket before we have resolved the overall approach for this small patch set (taken that the resource statistics object is planned to change).
You mean, these 3 fields wich change their names?
Ok, however this the argument for using protobuf serialization then *operator ==* to compare messages (: We wouldn't have to change the implementation of comparision..
IF, of course, google serialization is fast enough.. ((:
- Bartek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/#review87229
-----------------------------------------------------------
On June 9, 2015, 12:17 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 12:17 a.m.)
>
>
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
>
> This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp 52380c2461841026ee492797b4d8081f944f7b7b
> src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7
>
> Diff: https://reviews.apache.org/r/35157/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/#review87229
-----------------------------------------------------------
Hey Bartek,
Let's await further work on this ticket before we have resolved the overall approach for this small patch set (taken that the resource statistics object is planned to change).
- Niklas Nielsen
On June 8, 2015, 5:17 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 5:17 p.m.)
>
>
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
>
> This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp 52380c2461841026ee492797b4d8081f944f7b7b
> src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7
>
> Diff: https://reviews.apache.org/r/35157/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/
-----------------------------------------------------------
(Updated June 9, 2015, 12:17 a.m.)
Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
Issues addressed.
Repository: mesos
Description
-------
Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
Diffs (updated)
-----
include/mesos/type_utils.hpp 52380c2461841026ee492797b4d8081f944f7b7b
src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b
src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7
Diff: https://reviews.apache.org/r/35157/diff/
Testing
-------
make check
Thanks,
Bartek Plotka
Re: Review Request 35157: Added unit tests for fetching ResourceUsage
in both QoS Controller and Resource Estimator .
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/
-----------------------------------------------------------
(Updated June 8, 2015, 12:41 p.m.)
Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Summary (updated)
-----------------
Added unit tests for fetching ResourceUsage in both QoS Controller and Resource Estimator .
Repository: mesos
Description
-------
Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
Diffs
-----
src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7
Diff: https://reviews.apache.org/r/35157/diff/
Testing
-------
make check
Thanks,
Bartek Plotka
Re: Review Request 35157: Added unit tests for fetching ResoruceUsage
in both QoS Controller and Resource Estimator .
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35157/#review86882
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35164, 35157]
All tests passed.
- Mesos ReviewBot
On June 5, 2015, 11:53 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> -----------------------------------------------------------
>
> (Updated June 5, 2015, 11:53 p.m.)
>
>
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
>
> This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7
>
> Diff: https://reviews.apache.org/r/35157/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>