You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Zhitao Li <zh...@gmail.com> on 2017/02/07 07:11:11 UTC
Re: Review Request 53679: Implemented capacity heuristic check
related tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53679/
-----------------------------------------------------------
(Updated Feb. 7, 2017, 7:11 a.m.)
Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Kunal Thakar.
Changes
-------
Rebase and review comments.
Summary (updated)
-----------------
Implemented capacity heuristic check related tests.
Bugs: MESOS-6840
https://issues.apache.org/jira/browse/MESOS-6840
Repository: mesos
Description (updated)
-------
Implemented capacity heuristic check related tests.
Diffs (updated)
-----
src/tests/master_quota_tests.cpp b25f5911579c435549b9bc65994627414357dcb6
Diff: https://reviews.apache.org/r/53679/diff/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 53679: Implemented capacity heuristic check
related tests.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53679/#review165348
-----------------------------------------------------------
src/tests/master_quota_tests.cpp (line 1181)
<https://reviews.apache.org/r/53679/#comment237187>
This line is too long and currently this chain does not apply cleanly. Please run commits through `support/mesos-style.py`.
- Benjamin Bannier
On Feb. 10, 2017, 8:55 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53679/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 8:55 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Xiaojian Huang, Kunal Thakar, and Neil Conway.
>
>
> Bugs: MESOS-6840
> https://issues.apache.org/jira/browse/MESOS-6840
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented capacity heuristic check related tests.
>
>
> Diffs
> -----
>
> src/tests/master_quota_tests.cpp d15c7aacb85596cdee7cf59c0c179247ba624fe5
>
> Diff: https://reviews.apache.org/r/53679/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 53679: Implemented capacity heuristic check
related tests.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53679/
-----------------------------------------------------------
(Updated Feb. 13, 2017, 7:09 p.m.)
Review request for mesos, Alexander Rukletsov, Xiaojian Huang, Kunal Thakar, and Neil Conway.
Bugs: MESOS-6840
https://issues.apache.org/jira/browse/MESOS-6840
Repository: mesos
Description
-------
Implemented capacity heuristic check related tests.
Diffs (updated)
-----
src/tests/master_quota_tests.cpp d15c7aacb85596cdee7cf59c0c179247ba624fe5
Diff: https://reviews.apache.org/r/53679/diff/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 53679: Implemented capacity heuristic check
related tests.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53679/
-----------------------------------------------------------
(Updated Feb. 10, 2017, 7:55 a.m.)
Review request for mesos, Alexander Rukletsov, Xiaojian Huang, Kunal Thakar, and Neil Conway.
Changes
-------
Rebase with recent change.
Bugs: MESOS-6840
https://issues.apache.org/jira/browse/MESOS-6840
Repository: mesos
Description
-------
Implemented capacity heuristic check related tests.
Diffs (updated)
-----
src/tests/master_quota_tests.cpp d15c7aacb85596cdee7cf59c0c179247ba624fe5
Diff: https://reviews.apache.org/r/53679/diff/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 53679: Implemented capacity heuristic check
related tests.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53679/
-----------------------------------------------------------
(Updated Feb. 9, 2017, 9:42 p.m.)
Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Kunal Thakar.
Changes
-------
Review comments.
Bugs: MESOS-6840
https://issues.apache.org/jira/browse/MESOS-6840
Repository: mesos
Description
-------
Implemented capacity heuristic check related tests.
Diffs (updated)
-----
src/tests/master_quota_tests.cpp b25f5911579c435549b9bc65994627414357dcb6
Diff: https://reviews.apache.org/r/53679/diff/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 53679: Implemented capacity heuristic check
related tests.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53679/#review164651
-----------------------------------------------------------
src/tests/master_quota_tests.cpp (line 1134)
<https://reviews.apache.org/r/53679/#comment236415>
`EXPECT_FALSE(offers->empty());`
src/tests/master_quota_tests.cpp (line 1136)
<https://reviews.apache.org/r/53679/#comment236416>
Let's move this just before the invocation of `driver->launchTasks`.
src/tests/master_quota_tests.cpp (line 1150)
<https://reviews.apache.org/r/53679/#comment236417>
`status->state()`
src/tests/master_quota_tests.cpp (lines 1158 - 1159)
<https://reviews.apache.org/r/53679/#comment236419>
Any reason we need to reference `defaultAgentResources` here at all? I see you make sure above that `defaultAgentResources == agent1TotalResources`, but we could work with less symbols in the scope if we'd just use `agent1TotalResources`.
src/tests/master_quota_tests.cpp (line 1165)
<https://reviews.apache.org/r/53679/#comment236420>
No need for the explicit `false` here since it is the default. I also find it not very readible (_what is `false`?_).
src/tests/master_quota_tests.cpp (line 1168)
<https://reviews.apache.org/r/53679/#comment236418>
`response->body`
src/tests/master_quota_tests.cpp (line 1172)
<https://reviews.apache.org/r/53679/#comment236414>
I am surprised this would not be called deterministically. Is that the intended behavior or a bug?
src/tests/master_quota_tests.cpp (line 1201)
<https://reviews.apache.org/r/53679/#comment236421>
Let's use `agent1Id` or similar instead to not confuse this with the id of `agent2`'s possible ID.
src/tests/master_quota_tests.cpp (line 1203)
<https://reviews.apache.org/r/53679/#comment236422>
We don't make use of this anywhere in the test.
src/tests/master_quota_tests.cpp (line 1215)
<https://reviews.apache.org/r/53679/#comment236423>
We don't make use of this anywhere in the test.
src/tests/master_quota_tests.cpp (line 1221)
<https://reviews.apache.org/r/53679/#comment236425>
Can you use explicit capture here?
src/tests/master_quota_tests.cpp (line 1227)
<https://reviews.apache.org/r/53679/#comment236430>
This implicit conversion does not return a reference, but a new value instead; we do not use const ref to extend lifetime of temporaries.
Instead just make the copy explicit by dropping the ref.
src/tests/master_quota_tests.cpp (line 1252)
<https://reviews.apache.org/r/53679/#comment236431>
Unneeded capture.
src/tests/master_quota_tests.cpp (line 1255)
<https://reviews.apache.org/r/53679/#comment236432>
Unneeded capture.
src/tests/master_quota_tests.cpp (line 1298)
<https://reviews.apache.org/r/53679/#comment236433>
We don't rely on this anywhere in the test.
src/tests/master_quota_tests.cpp (line 1318)
<https://reviews.apache.org/r/53679/#comment236437>
Let's not put expressions which can fail in the expected part here.
How about
EXPECT_EQ(agent2Resources, stringify(agent2TotalResources.get());
Note that this requires inserting some spaces into `agent2Resources`, e.g., `cpus(%s):1; mem:512` etc.
src/tests/master_quota_tests.cpp (line 1325)
<https://reviews.apache.org/r/53679/#comment236413>
Extra whitespace at EOL.
src/tests/master_quota_tests.cpp (line 1328)
<https://reviews.apache.org/r/53679/#comment236438>
`// Satisfied`
src/tests/master_quota_tests.cpp (line 1337)
<https://reviews.apache.org/r/53679/#comment236439>
Let's add an expectation for `response->body`.
src/tests/master_quota_tests.cpp (line 1377)
<https://reviews.apache.org/r/53679/#comment236440>
We could use `agentTotalResources` and avoid pulling more variables into this scope.
src/tests/master_quota_tests.cpp (line 1422)
<https://reviews.apache.org/r/53679/#comment236442>
Let's add an expectation for `response->body`.
src/tests/master_quota_tests.cpp (line 1465)
<https://reviews.apache.org/r/53679/#comment236443>
We could just use `agent1TotalResources` in this test, and avoid pulling another variable into scope.
src/tests/master_quota_tests.cpp (line 1478)
<https://reviews.apache.org/r/53679/#comment236444>
`slaveRegisteredMessage->to`
src/tests/master_quota_tests.cpp (line 1492)
<https://reviews.apache.org/r/53679/#comment236445>
Let's add an expectation for `response->body`.
- Benjamin Bannier
On Feb. 7, 2017, 8:11 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53679/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 8:11 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Kunal Thakar.
>
>
> Bugs: MESOS-6840
> https://issues.apache.org/jira/browse/MESOS-6840
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented capacity heuristic check related tests.
>
>
> Diffs
> -----
>
> src/tests/master_quota_tests.cpp b25f5911579c435549b9bc65994627414357dcb6
>
> Diff: https://reviews.apache.org/r/53679/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>