mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 70752: Supported updating framework in MesosSchedulerDriver.
Date Tue, 11 Jun 2019 18:37:35 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message