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