You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2015/06/26 23:11:39 UTC
Review Request 35911: Moved StatusUpdate.uuid from required to
optional.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35911/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Vinod Kone.
Bugs: MESOS-2940
https://issues.apache.org/jira/browse/MESOS-2940
Repository: mesos
Description
-------
The `TaskStatus` contains an optional `UUID`, which should be unset for master-generated updates.
This udpates the `StatusUpdate` 'uuid' to also be optional.
Since the master and slave assume the 'uuid' is set, this adds CHECKs for now. Later, we'll want to properly validate this instead. I left a TODO for this.
Diffs
-----
include/mesos/mesos.proto 81008ed85daea798ed19d1031de8421a7dc3fb19
src/common/type_utils.cpp f88dff76a0dd57f5e16dc4d908655b4bb9d460a5
src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df
src/messages/messages.proto 1c8d79e3fca365520cdd67051f8730593955cab6
src/sched/sched.cpp bc76c71ae9d44bdddd291049223366e38cb0fd0c
src/scheduler/scheduler.cpp 1efc6fb351e49deaa8f626823592bc9155f5137b
src/slave/slave.cpp b3e1ccc28d2698d8bc91829d70787dc2fc17b80d
src/slave/status_update_manager.cpp 0ad24500c3007202263794fd094fa60535d8f58c
Diff: https://reviews.apache.org/r/35911/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request 35911: Moved StatusUpdate.uuid from required to
optional.
Posted by Ben Mahler <be...@gmail.com>.
> On June 26, 2015, 10:30 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, lines 2656-2663
> > <https://reviews.apache.org/r/35911/diff/1/?file=993611#file993611line2656>
> >
> > s/pure clients/HTTP API/
> >
> > How is this even possible with the current driver and slave semsntics? Should this be a CHECK?
> >
> > also, pull this up to #2633.
For the second comment, it's not possible with the driver, but some folks might be using some pure language bindings (e.g. pesos, jesos, etc). More notably, want to set us up for validating when the HTTP API comes, the CHECK seems unnecessarily brittle given we can just increment the metric and stay online..?
> On June 26, 2015, 10:30 p.m., Vinod Kone wrote:
> > src/slave/status_update_manager.cpp, lines 319-321
> > <https://reviews.apache.org/r/35911/diff/1/?file=993612#file993612line319>
> >
> > Why is this being done here? This should go inside stream::update()?
Sure, I've moved it there.
> On June 26, 2015, 10:30 p.m., Vinod Kone wrote:
> > src/sched/sched.cpp, line 702
> > <https://reviews.apache.org/r/35911/diff/1/?file=993609#file993609line702>
> >
> > why add a new condition? is it possible for update to not have an uuid when it's not from master/driver?
After chatting with vinod, decided to go with separate if conditions and update the comment to guide the reader as to what happened here.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35911/#review89575
-----------------------------------------------------------
On June 26, 2015, 9:11 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35911/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 9:11 p.m.)
>
>
> Review request for mesos, Jie Yu and Vinod Kone.
>
>
> Bugs: MESOS-2940
> https://issues.apache.org/jira/browse/MESOS-2940
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `TaskStatus` contains an optional `UUID`, which should be unset for master-generated updates.
>
> This udpates the `StatusUpdate` 'uuid' to also be optional.
>
> Since the master and slave assume the 'uuid' is set, this adds CHECKs for now. Later, we'll want to properly validate this instead. I left a TODO for this.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 81008ed85daea798ed19d1031de8421a7dc3fb19
> src/common/type_utils.cpp f88dff76a0dd57f5e16dc4d908655b4bb9d460a5
> src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df
> src/messages/messages.proto 1c8d79e3fca365520cdd67051f8730593955cab6
> src/sched/sched.cpp bc76c71ae9d44bdddd291049223366e38cb0fd0c
> src/scheduler/scheduler.cpp 1efc6fb351e49deaa8f626823592bc9155f5137b
> src/slave/slave.cpp b3e1ccc28d2698d8bc91829d70787dc2fc17b80d
> src/slave/status_update_manager.cpp 0ad24500c3007202263794fd094fa60535d8f58c
>
> Diff: https://reviews.apache.org/r/35911/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 35911: Moved StatusUpdate.uuid from required to
optional.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35911/#review89575
-----------------------------------------------------------
include/mesos/mesos.proto (line 922)
<https://reviews.apache.org/r/35911/#comment142201>
s/drivers/scheduler and executor driver/
src/messages/messages.proto (line 75)
<https://reviews.apache.org/r/35911/#comment142203>
s/replaced by/deprecated in favor of/
src/sched/sched.cpp (line 702)
<https://reviews.apache.org/r/35911/#comment142209>
why add a new condition? is it possible for update to not have an uuid when it's not from master/driver?
src/sched/sched.cpp (line 728)
<https://reviews.apache.org/r/35911/#comment142210>
ditto.
src/sched/sched.cpp (lines 919 - 926)
<https://reviews.apache.org/r/35911/#comment142205>
Can we use protobuf::createStatusUpdate() here?
src/sched/sched.cpp (lines 1002 - 1006)
<https://reviews.apache.org/r/35911/#comment142206>
ditto.
src/scheduler/scheduler.cpp (line 631)
<https://reviews.apache.org/r/35911/#comment142211>
ditto.
src/slave/slave.cpp (lines 2656 - 2663)
<https://reviews.apache.org/r/35911/#comment142212>
s/pure clients/HTTP API/
How is this even possible with the current driver and slave semsntics? Should this be a CHECK?
also, pull this up to #2633.
src/slave/status_update_manager.cpp (lines 319 - 321)
<https://reviews.apache.org/r/35911/#comment142216>
Why is this being done here? This should go inside stream::update()?
- Vinod Kone
On June 26, 2015, 9:11 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35911/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 9:11 p.m.)
>
>
> Review request for mesos, Jie Yu and Vinod Kone.
>
>
> Bugs: MESOS-2940
> https://issues.apache.org/jira/browse/MESOS-2940
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `TaskStatus` contains an optional `UUID`, which should be unset for master-generated updates.
>
> This udpates the `StatusUpdate` 'uuid' to also be optional.
>
> Since the master and slave assume the 'uuid' is set, this adds CHECKs for now. Later, we'll want to properly validate this instead. I left a TODO for this.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 81008ed85daea798ed19d1031de8421a7dc3fb19
> src/common/type_utils.cpp f88dff76a0dd57f5e16dc4d908655b4bb9d460a5
> src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df
> src/messages/messages.proto 1c8d79e3fca365520cdd67051f8730593955cab6
> src/sched/sched.cpp bc76c71ae9d44bdddd291049223366e38cb0fd0c
> src/scheduler/scheduler.cpp 1efc6fb351e49deaa8f626823592bc9155f5137b
> src/slave/slave.cpp b3e1ccc28d2698d8bc91829d70787dc2fc17b80d
> src/slave/status_update_manager.cpp 0ad24500c3007202263794fd094fa60535d8f58c
>
> Diff: https://reviews.apache.org/r/35911/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 35911: Moved StatusUpdate.uuid from required to
optional.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35911/#review89651
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On June 27, 2015, 12:36 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35911/
> -----------------------------------------------------------
>
> (Updated June 27, 2015, 12:36 a.m.)
>
>
> Review request for mesos, Jie Yu and Vinod Kone.
>
>
> Bugs: MESOS-2940
> https://issues.apache.org/jira/browse/MESOS-2940
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `TaskStatus` contains an optional `UUID`, which should be unset for master-generated updates.
>
> This udpates the `StatusUpdate` 'uuid' to also be optional.
>
> Since the master and slave assume the 'uuid' is set, this adds CHECKs for now. Later, we'll want to properly validate this instead. I left a TODO for this.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 81008ed85daea798ed19d1031de8421a7dc3fb19
> src/common/type_utils.cpp f88dff76a0dd57f5e16dc4d908655b4bb9d460a5
> src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df
> src/messages/messages.proto 1c8d79e3fca365520cdd67051f8730593955cab6
> src/sched/sched.cpp bc76c71ae9d44bdddd291049223366e38cb0fd0c
> src/scheduler/scheduler.cpp 1efc6fb351e49deaa8f626823592bc9155f5137b
> src/slave/slave.cpp b3e1ccc28d2698d8bc91829d70787dc2fc17b80d
> src/slave/status_update_manager.cpp 0ad24500c3007202263794fd094fa60535d8f58c
>
> Diff: https://reviews.apache.org/r/35911/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 35911: Moved StatusUpdate.uuid from required to
optional.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35911/
-----------------------------------------------------------
(Updated June 27, 2015, 12:36 a.m.)
Review request for mesos, Jie Yu and Vinod Kone.
Changes
-------
Vinod's feedback.
Bugs: MESOS-2940
https://issues.apache.org/jira/browse/MESOS-2940
Repository: mesos
Description
-------
The `TaskStatus` contains an optional `UUID`, which should be unset for master-generated updates.
This udpates the `StatusUpdate` 'uuid' to also be optional.
Since the master and slave assume the 'uuid' is set, this adds CHECKs for now. Later, we'll want to properly validate this instead. I left a TODO for this.
Diffs (updated)
-----
include/mesos/mesos.proto 81008ed85daea798ed19d1031de8421a7dc3fb19
src/common/type_utils.cpp f88dff76a0dd57f5e16dc4d908655b4bb9d460a5
src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df
src/messages/messages.proto 1c8d79e3fca365520cdd67051f8730593955cab6
src/sched/sched.cpp bc76c71ae9d44bdddd291049223366e38cb0fd0c
src/scheduler/scheduler.cpp 1efc6fb351e49deaa8f626823592bc9155f5137b
src/slave/slave.cpp b3e1ccc28d2698d8bc91829d70787dc2fc17b80d
src/slave/status_update_manager.cpp 0ad24500c3007202263794fd094fa60535d8f58c
Diff: https://reviews.apache.org/r/35911/diff/
Testing
-------
make check
Thanks,
Ben Mahler