mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vinod Kone" <vinodk...@gmail.com>
Subject Re: Review Request 14540: Task reconciliation for frameworks
Date Thu, 10 Oct 2013 18:09:06 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14540/#review26873
-----------------------------------------------------------



include/mesos/scheduler.hpp
<https://reviews.apache.org/r/14540/#comment52270>

    s/cause/causes/



include/mesos/scheduler.hpp
<https://reviews.apache.org/r/14540/#comment52269>

    s/reconcileTasks/reconcile/ ? This way we could make it more generic in the future?



include/mesos/scheduler.hpp
<https://reviews.apache.org/r/14540/#comment52271>

    ditto.



src/common/protobuf_utils.hpp
<https://reviews.apache.org/r/14540/#comment52274>

    This function name is a bit weird. AFAICT, this is only used once in master.cpp. It is
probably not worth to pull this out as a helper yet.



src/common/protobuf_utils.hpp
<https://reviews.apache.org/r/14540/#comment52272>

    The convention has been to use camelCase for variables.
    
    How about 
    s/update/_update/
    s/mutable_update/update/
    
    ?



src/master/master.cpp
<https://reviews.apache.org/r/14540/#comment52275>

    This seems a bit harsh. What if this is a failed over master?
    
    Can we just log a warning and ignore it instead?



src/master/master.cpp
<https://reviews.apache.org/r/14540/#comment52279>

    s/down/unknown/



src/master/master.cpp
<https://reviews.apache.org/r/14540/#comment52278>

    s/down/unknown/ ?



src/master/master.cpp
<https://reviews.apache.org/r/14540/#comment52276>

    s/have/has/



src/master/master.cpp
<https://reviews.apache.org/r/14540/#comment52277>

    Don't you want to check that status has slave_id set? These are sent by the scheduler
so there are no guarantees.



src/messages/messages.proto
<https://reviews.apache.org/r/14540/#comment52280>

    new line.



src/python/native/mesos_scheduler_driver_impl.cpp
<https://reviews.apache.org/r/14540/#comment52283>

    How about 
    
    "Master sends status updates if task status is different from last known state"



src/sched/sched.cpp
<https://reviews.apache.org/r/14540/#comment52284>

    Can you add
    
    if (!connected) {
      VLOG(1) << "Ignoring....;
      return;
    }



src/slave/slave.cpp
<https://reviews.apache.org/r/14540/#comment52285>

    Instead of doing it here, how about doing it in executor driver (exec.cpp) ?



src/tests/master_tests.cpp
<https://reviews.apache.org/r/14540/#comment52286>

    kill this. Times(1) is the default.



src/tests/master_tests.cpp
<https://reviews.apache.org/r/14540/#comment52287>

    Consider using LaunchTasks action to eliminate a lot of this boiler plate.



src/tests/master_tests.cpp
<https://reviews.apache.org/r/14540/#comment52289>

    kill.



src/tests/master_tests.cpp
<https://reviews.apache.org/r/14540/#comment52291>

    Can you pull these TODOs to the top of this test?



src/tests/master_tests.cpp
<https://reviews.apache.org/r/14540/#comment52292>

    Why this scoping?


- Vinod Kone


On Oct. 10, 2013, 4:52 p.m., Niklas Nielsen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14540/
> -----------------------------------------------------------
> 
> (Updated Oct. 10, 2013, 4:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> A framework can reconcile tasks in connection with a master fail-over by sending it's
"last known state" of its tasks and confirm or reestablish the current state of the setup.
> If existing tasks have changed state, a status update with the new state will be sent.

> 
> Soon, we may be able to reliably tell if any slave or task is no longer present. A status
update with TASK_LOST should be sent to the framework.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 957576b 
>   include/mesos/scheduler.hpp cf3ecda 
>   src/common/protobuf_utils.hpp 5c5f052 
>   src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 6d2a03b 
>   src/java/src/org/apache/mesos/MesosSchedulerDriver.java 7ef1fe7 
>   src/java/src/org/apache/mesos/SchedulerDriver.java c806a55 
>   src/master/master.hpp bed051c 
>   src/master/master.cpp 2fd48a6 
>   src/messages/messages.proto c599eb2 
>   src/python/native/mesos_scheduler_driver_impl.hpp 83fdc18 
>   src/python/native/mesos_scheduler_driver_impl.cpp f25d41d 
>   src/sched/sched.cpp c399f24 
>   src/slave/slave.cpp debb2f4 
>   src/tests/master_tests.cpp 52f09d4 
> 
> Diff: https://reviews.apache.org/r/14540/diff/
> 
> 
> Testing
> -------
> 
> Tests has been provided in MasterTest.ReconcileTaskTest:
> 
> Test sends different state than current -> An update with the current state of task
should be received.
> Stubs have been left for future test, where test sends expected state of non-existing
task -> An update with TASK_LOST should be received.
> 
> 
> Thanks,
> 
> Niklas Nielsen
> 
>


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