You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2015/03/17 00:06:27 UTC
Re: Review Request 32130: Ensured TaskStatus::source field is set for
executor status updates.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32130/
-----------------------------------------------------------
(Updated March 16, 2015, 11:06 p.m.)
Review request for mesos, Niklas Nielsen and Vinod Kone.
Changes
-------
Addressed comments, updated description.
Bugs: MESOS-2499
https://issues.apache.org/jira/browse/MESOS-2499
Repository: mesos
Description (updated)
-------
A status update originating from executor should have the TaskStatus::source field set to TaskStatus::SOURCE_EXECUTOR. Set this field in the slave to be future proof (a future where there will be no executor driver). Previous code has a bug and updated a copy of the update that was not forwarded. Add some checks for source and reason for status updates in existing tests.
Diffs (updated)
-----
src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06
src/tests/slave_tests.cpp a975305430097a8295b4b155e8448572c12bde22
Diff: https://reviews.apache.org/r/32130/diff/
Testing
-------
make check (Mac OS 10.9.5, CentOS 7.0.1406)
Thanks,
Alexander Rukletsov
Re: Review Request 32130: Ensured TaskStatus::source field is set for
executor status updates.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32130/#review76687
-----------------------------------------------------------
Ship it!
Ship It!
- Adam B
On March 16, 2015, 4:06 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32130/
> -----------------------------------------------------------
>
> (Updated March 16, 2015, 4:06 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-2499
> https://issues.apache.org/jira/browse/MESOS-2499
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A status update originating from executor should have the TaskStatus::source field set to TaskStatus::SOURCE_EXECUTOR. Set this field in the slave to be future proof (a future where there will be no executor driver). Previous code has a bug and updated a copy of the update that was not forwarded. Add some checks for source and reason for status updates in existing tests.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06
> src/tests/slave_tests.cpp a975305430097a8295b4b155e8448572c12bde22
>
> Diff: https://reviews.apache.org/r/32130/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, CentOS 7.0.1406)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 32130: Ensured TaskStatus::source field is set for
executor status updates.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32130/#review76694
-----------------------------------------------------------
Ship it!
Ship It!
- Niklas Nielsen
On March 16, 2015, 4:06 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32130/
> -----------------------------------------------------------
>
> (Updated March 16, 2015, 4:06 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-2499
> https://issues.apache.org/jira/browse/MESOS-2499
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A status update originating from executor should have the TaskStatus::source field set to TaskStatus::SOURCE_EXECUTOR. Set this field in the slave to be future proof (a future where there will be no executor driver). Previous code has a bug and updated a copy of the update that was not forwarded. Add some checks for source and reason for status updates in existing tests.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06
> src/tests/slave_tests.cpp a975305430097a8295b4b155e8448572c12bde22
>
> Diff: https://reviews.apache.org/r/32130/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, CentOS 7.0.1406)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 32130: Ensured TaskStatus::source field is set for
executor status updates.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32130/#review76677
-----------------------------------------------------------
Patch looks great!
Reviews applied: [32130]
All tests passed.
- Mesos ReviewBot
On March 16, 2015, 11:06 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32130/
> -----------------------------------------------------------
>
> (Updated March 16, 2015, 11:06 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-2499
> https://issues.apache.org/jira/browse/MESOS-2499
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A status update originating from executor should have the TaskStatus::source field set to TaskStatus::SOURCE_EXECUTOR. Set this field in the slave to be future proof (a future where there will be no executor driver). Previous code has a bug and updated a copy of the update that was not forwarded. Add some checks for source and reason for status updates in existing tests.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06
> src/tests/slave_tests.cpp a975305430097a8295b4b155e8448572c12bde22
>
> Diff: https://reviews.apache.org/r/32130/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, CentOS 7.0.1406)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 32130: Ensured TaskStatus::source field is set for
executor status updates.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 17, 2015, 5:59 p.m., Vinod Kone wrote:
> >
Vinod, we can avoid all these dances if we pass update by value. I would prefer this approach, but it's with our codebase, therefore I went for a copy. What do you think?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32130/#review76750
-----------------------------------------------------------
On March 16, 2015, 11:06 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32130/
> -----------------------------------------------------------
>
> (Updated March 16, 2015, 11:06 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-2499
> https://issues.apache.org/jira/browse/MESOS-2499
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A status update originating from executor should have the TaskStatus::source field set to TaskStatus::SOURCE_EXECUTOR. Set this field in the slave to be future proof (a future where there will be no executor driver). Previous code has a bug and updated a copy of the update that was not forwarded. Add some checks for source and reason for status updates in existing tests.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06
> src/tests/slave_tests.cpp a975305430097a8295b4b155e8448572c12bde22
>
> Diff: https://reviews.apache.org/r/32130/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, CentOS 7.0.1406)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 32130: Ensured TaskStatus::source field is set for
executor status updates.
Posted by Vinod Kone <vi...@gmail.com>.
> On March 17, 2015, 5:59 p.m., Vinod Kone wrote:
> >
>
> Alexander Rukletsov wrote:
> Vinod, we can avoid all these dances if we pass update by value. I would prefer this approach, but it's with our codebase, therefore I went for a copy. What do you think?
SGTM
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32130/#review76750
-----------------------------------------------------------
On March 16, 2015, 11:06 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32130/
> -----------------------------------------------------------
>
> (Updated March 16, 2015, 11:06 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-2499
> https://issues.apache.org/jira/browse/MESOS-2499
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A status update originating from executor should have the TaskStatus::source field set to TaskStatus::SOURCE_EXECUTOR. Set this field in the slave to be future proof (a future where there will be no executor driver). Previous code has a bug and updated a copy of the update that was not forwarded. Add some checks for source and reason for status updates in existing tests.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06
> src/tests/slave_tests.cpp a975305430097a8295b4b155e8448572c12bde22
>
> Diff: https://reviews.apache.org/r/32130/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, CentOS 7.0.1406)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 32130: Ensured TaskStatus::source field is set for
executor status updates.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32130/#review76750
-----------------------------------------------------------
src/slave/slave.cpp
<https://reviews.apache.org/r/32130/#comment124405>
just name the argument 'update_' to avoid making these many name changes in the method.
src/slave/slave.cpp
<https://reviews.apache.org/r/32130/#comment124406>
s/update/update_/
src/slave/slave.cpp
<https://reviews.apache.org/r/32130/#comment124407>
Pull this to the top.
// Make a copy to set source.
StatusUpdate update = update_;
src/slave/slave.cpp
<https://reviews.apache.org/r/32130/#comment124408>
pull this down to where it's first used, #2517.
- Vinod Kone
On March 16, 2015, 11:06 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32130/
> -----------------------------------------------------------
>
> (Updated March 16, 2015, 11:06 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-2499
> https://issues.apache.org/jira/browse/MESOS-2499
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A status update originating from executor should have the TaskStatus::source field set to TaskStatus::SOURCE_EXECUTOR. Set this field in the slave to be future proof (a future where there will be no executor driver). Previous code has a bug and updated a copy of the update that was not forwarded. Add some checks for source and reason for status updates in existing tests.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06
> src/tests/slave_tests.cpp a975305430097a8295b4b155e8448572c12bde22
>
> Diff: https://reviews.apache.org/r/32130/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, CentOS 7.0.1406)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 32130: Ensured TaskStatus::source field is set for
executor status updates.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32130/#review76833
-----------------------------------------------------------
Patch looks great!
Reviews applied: [32130]
All tests passed.
- Mesos ReviewBot
On March 17, 2015, 10:36 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32130/
> -----------------------------------------------------------
>
> (Updated March 17, 2015, 10:36 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-2499
> https://issues.apache.org/jira/browse/MESOS-2499
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A status update originating from executor should have the TaskStatus::source field set to TaskStatus::SOURCE_EXECUTOR. Set this field in the slave to be future proof (a future where there will be no executor driver). Previous code has a bug and updated a copy of the update that was not forwarded. Add some checks for source and reason for status updates in existing tests.
>
> NOTE: Passing argument by value instead of `const&` is not fully consistent with our codebase, but not entirely alien; it also documents our intention. This approach should be fine, since `install()` deduces argument types from the return values of getter functions of protobuf messaged.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 989832f8783d07d0702b30f0a68b8c383b57c621
> src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06
> src/tests/slave_tests.cpp a975305430097a8295b4b155e8448572c12bde22
>
> Diff: https://reviews.apache.org/r/32130/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, CentOS 7.0.1406, g++-4.4 @ Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 32130: Ensured TaskStatus::source field is set for
executor status updates.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32130/
-----------------------------------------------------------
(Updated March 17, 2015, 10:36 p.m.)
Review request for mesos, Niklas Nielsen and Vinod Kone.
Changes
-------
Addressed Niklas' comments.
Bugs: MESOS-2499
https://issues.apache.org/jira/browse/MESOS-2499
Repository: mesos
Description
-------
A status update originating from executor should have the TaskStatus::source field set to TaskStatus::SOURCE_EXECUTOR. Set this field in the slave to be future proof (a future where there will be no executor driver). Previous code has a bug and updated a copy of the update that was not forwarded. Add some checks for source and reason for status updates in existing tests.
NOTE: Passing argument by value instead of `const&` is not fully consistent with our codebase, but not entirely alien; it also documents our intention. This approach should be fine, since `install()` deduces argument types from the return values of getter functions of protobuf messaged.
Diffs (updated)
-----
src/slave/slave.hpp 989832f8783d07d0702b30f0a68b8c383b57c621
src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06
src/tests/slave_tests.cpp a975305430097a8295b4b155e8448572c12bde22
Diff: https://reviews.apache.org/r/32130/diff/
Testing
-------
make check (Mac OS 10.9.5, CentOS 7.0.1406, g++-4.4 @ Ubuntu 14.04)
Thanks,
Alexander Rukletsov
Re: Review Request 32130: Ensured TaskStatus::source field is set for
executor status updates.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32130/#review76830
-----------------------------------------------------------
Ship it!
modulo nik's comments.
- Vinod Kone
On March 17, 2015, 9:54 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32130/
> -----------------------------------------------------------
>
> (Updated March 17, 2015, 9:54 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-2499
> https://issues.apache.org/jira/browse/MESOS-2499
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A status update originating from executor should have the TaskStatus::source field set to TaskStatus::SOURCE_EXECUTOR. Set this field in the slave to be future proof (a future where there will be no executor driver). Previous code has a bug and updated a copy of the update that was not forwarded. Add some checks for source and reason for status updates in existing tests.
>
> NOTE: Passing argument by value instead of `const&` is not fully consistent with our codebase, but not entirely alien; it also documents our intention. This approach should be fine, since `install()` deduces argument types from the return values of getter functions of protobuf messaged.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 989832f8783d07d0702b30f0a68b8c383b57c621
> src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06
> src/tests/slave_tests.cpp a975305430097a8295b4b155e8448572c12bde22
>
> Diff: https://reviews.apache.org/r/32130/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, CentOS 7.0.1406, g++-4.4 @ Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 32130: Ensured TaskStatus::source field is set for
executor status updates.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32130/#review76829
-----------------------------------------------------------
Ship it!
src/slave/slave.hpp
<https://reviews.apache.org/r/32130/#comment124494>
s/adjusted/modified to contain source (from slave or executor)/
src/slave/slave.cpp
<https://reviews.apache.org/r/32130/#comment124493>
Insert newline
- Niklas Nielsen
On March 17, 2015, 2:54 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32130/
> -----------------------------------------------------------
>
> (Updated March 17, 2015, 2:54 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-2499
> https://issues.apache.org/jira/browse/MESOS-2499
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A status update originating from executor should have the TaskStatus::source field set to TaskStatus::SOURCE_EXECUTOR. Set this field in the slave to be future proof (a future where there will be no executor driver). Previous code has a bug and updated a copy of the update that was not forwarded. Add some checks for source and reason for status updates in existing tests.
>
> NOTE: Passing argument by value instead of `const&` is not fully consistent with our codebase, but not entirely alien; it also documents our intention. This approach should be fine, since `install()` deduces argument types from the return values of getter functions of protobuf messaged.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 989832f8783d07d0702b30f0a68b8c383b57c621
> src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06
> src/tests/slave_tests.cpp a975305430097a8295b4b155e8448572c12bde22
>
> Diff: https://reviews.apache.org/r/32130/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, CentOS 7.0.1406, g++-4.4 @ Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 32130: Ensured TaskStatus::source field is set for
executor status updates.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32130/
-----------------------------------------------------------
(Updated March 17, 2015, 9:54 p.m.)
Review request for mesos, Niklas Nielsen and Vinod Kone.
Changes
-------
Tested on gcc4.4
Bugs: MESOS-2499
https://issues.apache.org/jira/browse/MESOS-2499
Repository: mesos
Description
-------
A status update originating from executor should have the TaskStatus::source field set to TaskStatus::SOURCE_EXECUTOR. Set this field in the slave to be future proof (a future where there will be no executor driver). Previous code has a bug and updated a copy of the update that was not forwarded. Add some checks for source and reason for status updates in existing tests.
NOTE: Passing argument by value instead of `const&` is not fully consistent with our codebase, but not entirely alien; it also documents our intention. This approach should be fine, since `install()` deduces argument types from the return values of getter functions of protobuf messaged.
Diffs
-----
src/slave/slave.hpp 989832f8783d07d0702b30f0a68b8c383b57c621
src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06
src/tests/slave_tests.cpp a975305430097a8295b4b155e8448572c12bde22
Diff: https://reviews.apache.org/r/32130/diff/
Testing (updated)
-------
make check (Mac OS 10.9.5, CentOS 7.0.1406, g++-4.4 @ Ubuntu 14.04)
Thanks,
Alexander Rukletsov
Re: Review Request 32130: Ensured TaskStatus::source field is set for
executor status updates.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32130/
-----------------------------------------------------------
(Updated March 17, 2015, 9:50 p.m.)
Review request for mesos, Niklas Nielsen and Vinod Kone.
Changes
-------
Pass argument by value instead of creating a copy.
Bugs: MESOS-2499
https://issues.apache.org/jira/browse/MESOS-2499
Repository: mesos
Description (updated)
-------
A status update originating from executor should have the TaskStatus::source field set to TaskStatus::SOURCE_EXECUTOR. Set this field in the slave to be future proof (a future where there will be no executor driver). Previous code has a bug and updated a copy of the update that was not forwarded. Add some checks for source and reason for status updates in existing tests.
NOTE: Passing argument by value instead of `const&` is not fully consistent with our codebase, but not entirely alien; it also documents our intention. This approach should be fine, since `install()` deduces argument types from the return values of getter functions of protobuf messaged.
Diffs (updated)
-----
src/slave/slave.hpp 989832f8783d07d0702b30f0a68b8c383b57c621
src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06
src/tests/slave_tests.cpp a975305430097a8295b4b155e8448572c12bde22
Diff: https://reviews.apache.org/r/32130/diff/
Testing
-------
make check (Mac OS 10.9.5, CentOS 7.0.1406)
Thanks,
Alexander Rukletsov