----------------------------------------------------------- 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 > >
