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/06/13 19:04:47 UTC
Review Request 70854: Made scheduler driver's updateFramework() accept
FrameworkInfo with ID.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70854/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Repository: mesos
Description
-------
This patch allows the caller to fill `FrameworkInfo.id` passed into
MesosSchedulerDriver::updateFramework() with the FrameworkID provided
by the scheduler driver. Previously the caller was required to keep
this field empty.
Diffs
-----
include/mesos/scheduler.hpp c5e61d4fd7aae5067d8c7d8dec878dc82c86e4ec
src/sched/sched.cpp 281236bb53e1c6ed77b69bf954e27705595ffb2a
Diff: https://reviews.apache.org/r/70854/diff/1/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70854: Made scheduler driver's updateFramework()
require FrameworkInfo with ID.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On June 15, 2019, 5:34 p.m., Benjamin Mahler wrote:
> > Thanks! One thing I'm wondering is whether it makes sense to just populate `FrameworkInfo.user` and `FrameworkInfo.hostname` in `SchedulerProcess::updateFramework` to the previous values (if not set), rather than calling the system calls again.
I'm not sure either.
This approach has two upsides:
- a bit simpler
- calling updateFramework() is consistent with re-creating the SchedulerDriver
and two downsides:
- user/hostname can change during the SchedulerDriver's lifetime (weird)
- these syscalls might block for an enormous length of time (at least the ones behind `net::hostname()`, which calls `getaddrinfo()`, which might perform DNS lookups)
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70854/#review215916
-----------------------------------------------------------
On June 14, 2019, 12:32 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70854/
> -----------------------------------------------------------
>
> (Updated June 14, 2019, 12:32 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes it mandatory for the caller to fill the 'id' field of
> the FrameworkInfo passed into MesosSchedulerDriver::updateFramework().
>
>
> Diffs
> -----
>
> include/mesos/scheduler.hpp c5e61d4fd7aae5067d8c7d8dec878dc82c86e4ec
> src/sched/sched.cpp 281236bb53e1c6ed77b69bf954e27705595ffb2a
>
>
> Diff: https://reviews.apache.org/r/70854/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70854: Made scheduler driver's updateFramework()
require FrameworkInfo with ID.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70854/#review215916
-----------------------------------------------------------
Fix it, then Ship it!
Thanks! One thing I'm wondering is whether it makes sense to just populate `FrameworkInfo.user` and `FrameworkInfo.hostname` in `SchedulerProcess::updateFramework` to the previous values (if not set), rather than calling the system calls again.
include/mesos/scheduler.hpp
Lines 321-329 (original), 321-332 (patched)
<https://reviews.apache.org/r/70854/#comment302861>
How about:
```
// Inform Mesos master about changes to the `FrameworkInfo`. The
// driver will store the new `FrameworkInfo` and all subsequent
// re-registrations will use it.
//
// NOTE: If the supplied info is invalid or fails authorization,
// the `error()` callback will be invoked asynchronously (after
// the master replies with a `FrameworkErrorMessage`).
//
// NOTE: This must be called after initial registration with the
// master completes and the `FrameworkID` is assigned. The assigned
// `FrameworkID` must be set in `frameworkInfo`.
//
// NOTE: The `FrameworkInfo.user` and `FrameworkInfo.hostname`
// fields will be auto-populated using the same approach used
// during driver initialization.
```
This makes me realize, it would be nice if rather than auto-populate `user` and `hostname`, the SchedulerProcess sets them to the previous values, if missing. That way, we don't have to keep making system calls?
include/mesos/scheduler.hpp
Line 322 (original), 322 (patched)
<https://reviews.apache.org/r/70854/#comment302860>
will send
src/sched/sched.cpp
Line 1600 (original), 1600-1612 (patched)
<https://reviews.apache.org/r/70854/#comment302858>
You'll notice that the other logic checks both set and non empty, e.g.
https://github.com/apache/mesos/blob/1.8.0/src/sched/sched.cpp#L219
https://github.com/apache/mesos/blob/1.8.0/src/sched/sched.cpp#L559
https://github.com/apache/mesos/blob/1.8.0/src/sched/sched.cpp#L825
etc
So we should check similarly here.
src/sched/sched.cpp
Lines 1601 (patched)
<https://reviews.apache.org/r/70854/#comment302859>
The error callback is for error messages from the master, AFAICT we handle scheduler caller validation errors using `ABORT()` currently. However, I see from your next patches that using `error()` makes this testable, so that seems worth it.
src/sched/sched.cpp
Lines 1610 (patched)
<https://reviews.apache.org/r/70854/#comment302864>
no periods in logging / error messages
- Benjamin Mahler
On June 14, 2019, 12:32 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70854/
> -----------------------------------------------------------
>
> (Updated June 14, 2019, 12:32 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes it mandatory for the caller to fill the 'id' field of
> the FrameworkInfo passed into MesosSchedulerDriver::updateFramework().
>
>
> Diffs
> -----
>
> include/mesos/scheduler.hpp c5e61d4fd7aae5067d8c7d8dec878dc82c86e4ec
> src/sched/sched.cpp 281236bb53e1c6ed77b69bf954e27705595ffb2a
>
>
> Diff: https://reviews.apache.org/r/70854/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70854: Made scheduler driver's updateFramework()
require FrameworkInfo with ID.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70854/
-----------------------------------------------------------
(Updated June 14, 2019, 12:32 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Made the FrameworkID required, moved checking it into the SchedulerProcess.
Summary (updated)
-----------------
Made scheduler driver's updateFramework() require FrameworkInfo with ID.
Repository: mesos
Description (updated)
-------
This patch makes it mandatory for the caller to fill the 'id' field of
the FrameworkInfo passed into MesosSchedulerDriver::updateFramework().
Diffs (updated)
-----
include/mesos/scheduler.hpp c5e61d4fd7aae5067d8c7d8dec878dc82c86e4ec
src/sched/sched.cpp 281236bb53e1c6ed77b69bf954e27705595ffb2a
Diff: https://reviews.apache.org/r/70854/diff/2/
Changes: https://reviews.apache.org/r/70854/diff/1-2/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70854: Made scheduler driver's updateFramework()
accept FrameworkInfo with ID.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On June 13, 2019, 8:35 p.m., Benjamin Mahler wrote:
> > src/sched/sched.cpp
> > Lines 2342-2348 (original), 2347-2369 (patched)
> > <https://reviews.apache.org/r/70854/diff/1/?file=2149406#file2149406line2347>
> >
> > I think we want to put most of this type of logic into the SchedulerProcess rather than the MesosSchedulerDriver. It's already where we do the ID assignment.
> >
> > The filling in of hostname/user seems ok to leave in the scheduler driver since that's where it's done during initialization.
Well, my initial thinking was that it is generally better to report a local error at the call site, and not via a callback.
However, after having a look into some existing framework implementations (say, Spark), I've realized that frameworks are obligated to provide an error() callback in their Scheduler implementation - and they do provide it, but they are not obligated to check the status returned by the SchedulerDriver (and they usually don't).
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70854/#review215887
-----------------------------------------------------------
On June 13, 2019, 7:04 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70854/
> -----------------------------------------------------------
>
> (Updated June 13, 2019, 7:04 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch allows the caller to fill `FrameworkInfo.id` passed into
> MesosSchedulerDriver::updateFramework() with the FrameworkID provided
> by the scheduler driver. Previously the caller was required to keep
> this field empty.
>
>
> Diffs
> -----
>
> include/mesos/scheduler.hpp c5e61d4fd7aae5067d8c7d8dec878dc82c86e4ec
> src/sched/sched.cpp 281236bb53e1c6ed77b69bf954e27705595ffb2a
>
>
> Diff: https://reviews.apache.org/r/70854/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70854: Made scheduler driver's updateFramework()
accept FrameworkInfo with ID.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70854/#review215887
-----------------------------------------------------------
How about we simplify and just require the `FrameworkInfo.id` to be set?
We already require it to be set during driver construction if the framework had one assigned, so it would be nice to require it on `updateFramework()` too. I doubt that will be a problem for users, and it lets us simplify the code and tests.
src/sched/sched.cpp
Lines 2342-2348 (original), 2347-2369 (patched)
<https://reviews.apache.org/r/70854/#comment302827>
I think we want to put most of this type of logic into the SchedulerProcess rather than the MesosSchedulerDriver. It's already where we do the ID assignment.
The filling in of hostname/user seems ok to leave in the scheduler driver since that's where it's done during initialization.
src/sched/sched.cpp
Line 2351 (original), 2376 (patched)
<https://reviews.apache.org/r/70854/#comment302826>
// We fill in missing hostname and user fields if absent since
// they're filled in by the driver during initialization and
// we need to keep them consistent. The caller may not know
// which values were filled in (as we don't pass back the
// filled in FrameworkInfo at any point).
- Benjamin Mahler
On June 13, 2019, 7:04 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70854/
> -----------------------------------------------------------
>
> (Updated June 13, 2019, 7:04 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch allows the caller to fill `FrameworkInfo.id` passed into
> MesosSchedulerDriver::updateFramework() with the FrameworkID provided
> by the scheduler driver. Previously the caller was required to keep
> this field empty.
>
>
> Diffs
> -----
>
> include/mesos/scheduler.hpp c5e61d4fd7aae5067d8c7d8dec878dc82c86e4ec
> src/sched/sched.cpp 281236bb53e1c6ed77b69bf954e27705595ffb2a
>
>
> Diff: https://reviews.apache.org/r/70854/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>