You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Sekretenko <as...@mesosphere.io> on 2019/05/29 18:31:42 UTC
Review Request 70752: Supported updating framework in
MesosSchedulerDriver.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70752/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler and Meng Zhu.
Bugs: MESOS-9793
https://issues.apache.org/jira/browse/MESOS-9793
Repository: mesos
Description
-------
This patch adds a method to the MesosSchedulerDriver which updates the
FrameworkInfo (by making an UPDATE_FRAMEWORK call to the master and also
updating the FrameworkInfo stored in the driver for the purposes of
(re-)subscribing).
Diffs
-----
include/mesos/scheduler.hpp 2ea7cdf1f9473c7b94d4282dd0df129947c97e69
src/sched/sched.cpp e77a02951831a7e0c5d9a9068f8d014cb1478382
Diff: https://reviews.apache.org/r/70752/diff/1/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70752: Supported updating framework in
MesosSchedulerDriver.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70752/#review215779
-----------------------------------------------------------
Fix it, then Ship it!
Looks good, the main observation I have is that the current approach will unnecessarily send the UPDATE_FRAMEWORK message in the case that there is not a SUBSCRIBE in progress. In that case, the next SUBSCRIBE would pick up the updated FrameworkInfo anyway. It seems pretty easy to resolve that, but for now I've left it as is (with the extra UPDATE_FRAMEWORK call), since that seems like a significant adjustment for me to make prior to committing.
include/mesos/scheduler.hpp
Lines 327-330 (patched)
<https://reviews.apache.org/r/70752/#comment302642>
Hm.. perhaps just say:
```
// NOTE: If the supplied info is invalid or fails authorization,
// the `error()` callback will be invoked asynchronously (after
// the master replies with a FrameworkErrorMessage).
```
src/sched/sched.cpp
Lines 1603 (patched)
<https://reviews.apache.org/r/70752/#comment302645>
camelCase in mesos code
src/sched/sched.cpp
Lines 1605 (patched)
<https://reviews.apache.org/r/70752/#comment302644>
assignment syntax?
```
*framework.mutable_id() = std::move(framework_id);
```
src/sched/sched.cpp
Lines 1610-1611 (patched)
<https://reviews.apache.org/r/70752/#comment302646>
How about:
"Postponing UPDATE_FRAMEWORK call: not registered with master"
src/sched/sched.cpp
Lines 1666-1667 (patched)
<https://reviews.apache.org/r/70752/#comment302641>
let's use the assignment syntax:
```
*call.mutable_update_framework()->mutable_framework_info() =
framework;
```
src/sched/sched.cpp
Lines 1689 (patched)
<https://reviews.apache.org/r/70752/#comment302643>
camelCase up in mesos code (whereas libprocess and stout tries to use snake_case but is inconsistent about it =/)
src/sched/sched.cpp
Lines 2336-2337 (patched)
<https://reviews.apache.org/r/70752/#comment302640>
We don't use periods in the log messages, so:
```
LOG(ERROR) << "MesosSchedulerDriver::updateFramework should not be called"
<< " with 'FrameworkInfo.id' set, aborting driver";
```
src/sched/sched.cpp
Lines 2344 (patched)
<https://reviews.apache.org/r/70752/#comment302680>
assignment syntax and std::move?
include/mesos/scheduler.hpp
Lines 324 (patched)
<https://reviews.apache.org/r/70752/#comment302681>
"will be"
src/sched/sched.cpp
Lines 1607-1613 (patched)
<https://reviews.apache.org/r/70752/#comment302685>
Hm.. this approach will send an UPDATE_FRAMEWORK unnecessarily when no subscribe is in progress. Perhaps the following comment:
```
// If not `connected`, we defer the sending of the UPDATE_FRAMEWORK
// message. Note that this approach may unnecessarily send an
// UPDATE_FRAMEWORK message in the case that a (re)registration is
// not currently in progress (because the next (re)registration
// will have already used the updated `framework`).
//
// TODO(asekretenko): Consider tracking additional state to avoid
// sending an unnecessary UPDATE_FRAMEWORK message.
```
However, we can probably easily fix this by setting send_update_framework_on_connect to false when sending a SUBSCRIBE call?
src/sched/sched.cpp
Lines 1610-1611 (patched)
<https://reviews.apache.org/r/70752/#comment302684>
Ditto here about VLOG(1)
src/sched/sched.cpp
Lines 1670 (patched)
<https://reviews.apache.org/r/70752/#comment302683>
VLOG(1)
This is an embedded library so the approach we took was to use VLOG(1) for what might normally be INFO messages. Over time, some INFO level logging has been added, so it's becoming less clear, but let's stick with VLOG(1) here for now
src/sched/sched.cpp
Lines 2352 (patched)
<https://reviews.apache.org/r/70752/#comment302682>
Let's keep the whitespace consistent across these functions, so newline here
- Benjamin Mahler
On June 7, 2019, 4:26 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70752/
> -----------------------------------------------------------
>
> (Updated June 7, 2019, 4:26 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9793
> https://issues.apache.org/jira/browse/MESOS-9793
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a method to the MesosSchedulerDriver which updates the
> FrameworkInfo (by making an UPDATE_FRAMEWORK call to the master and also
> updating the FrameworkInfo stored in the driver for the purposes of
> (re-)subscribing).
>
>
> Diffs
> -----
>
> include/mesos/scheduler.hpp 2ea7cdf1f9473c7b94d4282dd0df129947c97e69
> src/sched/sched.cpp e77a02951831a7e0c5d9a9068f8d014cb1478382
>
>
> Diff: https://reviews.apache.org/r/70752/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70752: Supported updating framework in
MesosSchedulerDriver.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70752/
-----------------------------------------------------------
(Updated June 7, 2019, 4:26 p.m.)
Review request for mesos, Benjamin Mahler and Meng Zhu.
Bugs: MESOS-9793
https://issues.apache.org/jira/browse/MESOS-9793
Repository: mesos
Description
-------
This patch adds a method to the MesosSchedulerDriver which updates the
FrameworkInfo (by making an UPDATE_FRAMEWORK call to the master and also
updating the FrameworkInfo stored in the driver for the purposes of
(re-)subscribing).
Diffs
-----
include/mesos/scheduler.hpp 2ea7cdf1f9473c7b94d4282dd0df129947c97e69
src/sched/sched.cpp e77a02951831a7e0c5d9a9068f8d014cb1478382
Diff: https://reviews.apache.org/r/70752/diff/2/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70752: Supported updating framework in
MesosSchedulerDriver.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70752/
-----------------------------------------------------------
(Updated June 7, 2019, 4:26 p.m.)
Review request for mesos, Benjamin Mahler and Meng Zhu.
Changes
-------
Made handling of empty `user` and `hostname` by updateFramework() consistent with that of driver's constructor.
Bugs: MESOS-9793
https://issues.apache.org/jira/browse/MESOS-9793
Repository: mesos
Description
-------
This patch adds a method to the MesosSchedulerDriver which updates the
FrameworkInfo (by making an UPDATE_FRAMEWORK call to the master and also
updating the FrameworkInfo stored in the driver for the purposes of
(re-)subscribing).
Diffs (updated)
-----
include/mesos/scheduler.hpp 2ea7cdf1f9473c7b94d4282dd0df129947c97e69
src/sched/sched.cpp e77a02951831a7e0c5d9a9068f8d014cb1478382
Diff: https://reviews.apache.org/r/70752/diff/2/
Changes: https://reviews.apache.org/r/70752/diff/1-2/
Testing
-------
Thanks,
Andrei Sekretenko