You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2017/02/25 02:02:10 UTC
Review Request 57059: Updated default executor tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57059/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar and Gilbert Song.
Repository: mesos
Description
-------
Reorganized so that objects are defined closer to their usage.
Diffs
-----
src/tests/default_executor_tests.cpp eaf639467aaaa35b28b21bfb7e16aca5924a5a82
Diff: https://reviews.apache.org/r/57059/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 57059: Updated default executor tests.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57059/#review166802
-----------------------------------------------------------
Patch looks great!
Reviews applied: [57058, 57059]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 25, 2017, 2:02 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57059/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2017, 2:02 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Gilbert Song.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Reorganized so that objects are defined closer to their usage.
>
>
> Diffs
> -----
>
> src/tests/default_executor_tests.cpp eaf639467aaaa35b28b21bfb7e16aca5924a5a82
>
> Diff: https://reviews.apache.org/r/57059/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 57059: Updated default executor tests.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57059/#review166800
-----------------------------------------------------------
LGTM, holding off the ship it due to a minor comment/query around reordering expectations.
src/tests/default_executor_tests.cpp (line 142)
<https://reviews.apache.org/r/57059/#comment238875>
Not yours:
Can we directly do this later instead of creating `frameworkInfo` at all here?
```cpp
subscribe->mutable_framework_info()->CopyFrom(v1::DEFAULT_FRAMEWORK_INFO);
```
src/tests/default_executor_tests.cpp (lines 155 - 166)
<https://reviews.apache.org/r/57059/#comment238877>
Might want to group all the `executorInfo` statements together for readability?
```cpp
v1::FrameworkID frameworkId(subscribed->framework_id());
v1::Resources resources =
v1::Resources::parse("cpus:0.1;mem:32;disk:32").get();
v1::ExecutorInfo executorInfo;
executorInfo.set_type(v1::ExecutorInfo::DEFAULT);
executorInfo.mutable_executor_id()->CopyFrom(v1::DEFAULT_EXECUTOR_ID);
executorInfo.mutable_framework_id()->CopyFrom(frameworkId);
executorInfo.mutable_resources()->CopyFrom(resources);
```
src/tests/default_executor_tests.cpp (lines 308 - 312)
<https://reviews.apache.org/r/57059/#comment238879>
hmm, do we want to move all the expectations just before sending the call itself i.e. before L337?
Ditto for the other occurences too.
- Anand Mazumdar
On Feb. 25, 2017, 2:02 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57059/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2017, 2:02 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Gilbert Song.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Reorganized so that objects are defined closer to their usage.
>
>
> Diffs
> -----
>
> src/tests/default_executor_tests.cpp eaf639467aaaa35b28b21bfb7e16aca5924a5a82
>
> Diff: https://reviews.apache.org/r/57059/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 57059: Updated default executor tests.
Posted by Vinod Kone <vi...@gmail.com>.
> On March 7, 2017, 11:58 p.m., Gilbert Song wrote:
> >
As discussed offline, I will do these changes in a subsequent patch since this patch is mainly about code movement.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57059/#review168219
-----------------------------------------------------------
On March 7, 2017, 1:17 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57059/
> -----------------------------------------------------------
>
> (Updated March 7, 2017, 1:17 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Gilbert Song.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Reorganized so that objects are defined closer to their usage.
>
>
> Diffs
> -----
>
> src/tests/default_executor_tests.cpp e4d43c8ad447577a9c5c7951207596bda1070856
>
>
> Diff: https://reviews.apache.org/r/57059/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 57059: Updated default executor tests.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57059/#review168219
-----------------------------------------------------------
src/tests/default_executor_tests.cpp
Lines 153-160 (original), 142-149 (patched)
<https://reviews.apache.org/r/57059/#comment240414>
Could we use `v1::scheduler::SendSubscribe()` for these part?
We can do it once receive `connected()` at line #119:
```
Future<Nothing> connected;
EXPECT_CALL(*scheduler, connected(_))
.WillOnce(DoAll(v1::scheduler::SendSubscribe(frameworkInfo),
FutureSatisfy(&connected)));
```
ditto to the others.
src/tests/default_executor_tests.cpp
Lines 166-167 (original), 154-161 (patched)
<https://reviews.apache.org/r/57059/#comment240416>
We could use the `v1::createExecutorInfo()` helper here:
```
v1::FrameworkID frameworkId(subscribed->framework_id());
v1::ExecutorInfo executorInfo = v1::createExecutorInfo(
"test_default_executor",
None(),
"cpus:0.1;mem:32;disk:32",
v1::ExecutorInfo::DEFAULT);
// Update `executorInfo` with the subscribed `frameworkId`.
executorInfo.mutable_framework_id()->CopyFrom(frameworkId);
```
Ideally, we should make `FrameworkId` configurable in the helper `createExecutorInfo()`.
src/tests/default_executor_tests.cpp
Lines 185-203 (original), 179-197 (patched)
<https://reviews.apache.org/r/57059/#comment240417>
use `v1::createCallAccept()`?
src/tests/default_executor_tests.cpp
Line 207 (original), 201 (patched)
<https://reviews.apache.org/r/57059/#comment240418>
Not yours, but should we add check for `TASK_FINISHED`?
- Gilbert Song
On March 6, 2017, 5:17 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57059/
> -----------------------------------------------------------
>
> (Updated March 6, 2017, 5:17 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Gilbert Song.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Reorganized so that objects are defined closer to their usage.
>
>
> Diffs
> -----
>
> src/tests/default_executor_tests.cpp e4d43c8ad447577a9c5c7951207596bda1070856
>
>
> Diff: https://reviews.apache.org/r/57059/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 57059: Updated default executor tests.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57059/#review168217
-----------------------------------------------------------
Ship it!
LGTM
- Anand Mazumdar
On March 7, 2017, 1:17 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57059/
> -----------------------------------------------------------
>
> (Updated March 7, 2017, 1:17 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Gilbert Song.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Reorganized so that objects are defined closer to their usage.
>
>
> Diffs
> -----
>
> src/tests/default_executor_tests.cpp e4d43c8ad447577a9c5c7951207596bda1070856
>
>
> Diff: https://reviews.apache.org/r/57059/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 57059: Updated default executor tests.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57059/#review168095
-----------------------------------------------------------
Patch looks great!
Reviews applied: [57058, 57059]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 7, 2017, 1:17 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57059/
> -----------------------------------------------------------
>
> (Updated March 7, 2017, 1:17 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Gilbert Song.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Reorganized so that objects are defined closer to their usage.
>
>
> Diffs
> -----
>
> src/tests/default_executor_tests.cpp e4d43c8ad447577a9c5c7951207596bda1070856
>
>
> Diff: https://reviews.apache.org/r/57059/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 57059: Updated default executor tests.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57059/
-----------------------------------------------------------
(Updated March 7, 2017, 1:17 a.m.)
Review request for mesos, Anand Mazumdar and Gilbert Song.
Changes
-------
addressed anand's comments.
Repository: mesos
Description
-------
Reorganized so that objects are defined closer to their usage.
Diffs (updated)
-----
src/tests/default_executor_tests.cpp e4d43c8ad447577a9c5c7951207596bda1070856
Diff: https://reviews.apache.org/r/57059/diff/2/
Changes: https://reviews.apache.org/r/57059/diff/1-2/
Testing
-------
make check
Thanks,
Vinod Kone