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




src/examples/long_lived_framework.cpp (lines 39 - 40)
<https://reviews.apache.org/r/45067/#comment188642>

    Add new constants for executor and use them, instead of resuing these.
    
    CPUS_PER_EXECUTOR
    MEM_PER_EXECUTOR



src/examples/long_lived_framework.cpp (line 74)
<https://reviews.apache.org/r/45067/#comment188651>

    s/. Reject/. Reject/ <-- extraneous space



src/examples/long_lived_framework.cpp (lines 84 - 85)
<https://reviews.apache.org/r/45067/#comment188656>

    So if resources are not enough we do not explicitly decline the offer?
    
    How about:
    
    ```
    // Reject offer if the offer belongs to a different slave than the one 
currently running the executor(s).
    if (slaveID.isSome() && slaveID != offers[i].slave_id()) {
      // decline offer.
    }
    
    // Reject the offer if the resources are not enough.
    if (!Resources(offers[i].resources()).flatten().contains(TASK_RESOURCES + 
EXECUTOR_RESOURCES)) {
      // decline offer.
    }
    
    // Launch the task.
    
    ```
    
    Also, are you expecting the executor to be multi-task? If yes, the 
resources check above should only include task resources? 
    
    Since you do not control whether the executor is single or multi-task, I 
would recommend to launch each task with an unique executor id.



src/examples/long_lived_framework.cpp (line 85)
<https://reviews.apache.org/r/45067/#comment188653>

    TASK_RESOURCES + EXEcUTOR_RESOURCES



src/examples/long_lived_framework.cpp (line 128)
<https://reviews.apache.org/r/45067/#comment188657>

    s/sid/slaveID/



src/examples/long_lived_framework.cpp (line 129)
<https://reviews.apache.org/r/45067/#comment188658>

    I thought we could compare option directly with a value instead of doing a 
.get()?



src/examples/long_lived_framework.cpp (line 138)
<https://reviews.apache.org/r/45067/#comment188663>

    seems weird that you are setting residency/slaveID to None() if one 
executor is lost. isn't it still possible for other executors to be running on 
that slave? do we want to launch new executors on a different slave in that 
case?
    
    I would recommend to keep track of running executors with `set<ExecutorID>`.



src/examples/long_lived_framework.cpp (line 151)
<https://reviews.apache.org/r/45067/#comment188650>

    why is this called "residency" instead of "slaveID" ?



src/examples/long_lived_framework.cpp (line 174)
<https://reviews.apache.org/r/45067/#comment188647>

    why does the framework assume master lives on the same machine?



src/examples/long_lived_framework.cpp (line 184)
<https://reviews.apache.org/r/45067/#comment188648>

    s/mesos// ?



src/examples/long_lived_framework.cpp (line 212)
<https://reviews.apache.org/r/45067/#comment188644>

    no underscores please. you can just name it "resources" here.



src/examples/long_lived_framework.cpp (lines 213 - 214)
<https://reviews.apache.org/r/45067/#comment188643>

    use "*_PER_EXECUTOR" here



src/examples/long_lived_framework.cpp (lines 222 - 242)
<https://reviews.apache.org/r/45067/#comment188646>

    Can you rephrase the comments to use the flag based executor first? I think 
it is easier to follow.
    
    ```
    if (flags.executor_command.iSome()) {
    
    } else if (flags.build_dir.isSome()) {
    
    } else {
    
    }
    
    ```



src/examples/long_lived_framework.cpp (line 228)
<https://reviews.apache.org/r/45067/#comment188645>

    s/executor_command/command/


- Vinod Kone


On March 23, 2016, 10:50 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45067/
> -----------------------------------------------------------
> 
> (Updated March 23, 2016, 10:50 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Artem Harutyunyan, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This gives the example `long-lived-framework` enough options to run outside 
> of the build environment.
> 
> This also updates:
> 
> * The style of the framework code.
> * Gives the `ExecutorInfo` some resources (needed for some cgroups isolators).
> * Restricts the framework to one agent.  Otherwise, it would grab a small 
> chunk of every machine in the cluster.
> * Adds filters for declined offers.
> 
> 
> Diffs
> -----
> 
>   src/examples/long_lived_framework.cpp 
> 0000289a0b9dd3d1ce30f20dd9bb381126bff30c 
> 
> Diff: https://reviews.apache.org/r/45067/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> Ran this on the master node on a Mesos cluster:
> ```
> ./long-lived-framework --master=zk://localhost:2181/mesos 
> --executor_uri="https://s3.amazonaws.com/url/to/long-lived-executor"; 
> --executor_command="LD_LIBRARY_PATH=/path/to/libmesos && 
> ./long-lived-executor"
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Reply via email to