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