Re: Review Request 45067: Updated the long-lived-framework example.

[Joseph Wu]

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

(Updated April 6, 2016, 4:19 p.m.)


Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod 
Kone.


Changes
---

Update description to fit under 72 characters per line.


Bugs: MESOS-5062
https://issues.apache.org/jira/browse/MESOS-5062


Repository: mesos


Description (updated)
---

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 
ef498d63bc5f0a8deb46d71edd85a76a1d38fdd0 

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

Re: Review Request 45067: Updated the long-lived-framework example.

[Joseph Wu]

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

(Updated April 6, 2016, 3:34 p.m.)


Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod 
Kone.


Changes
---

Accidentally reset my .reviewboardrc.  Restored description.


Summary (updated)
-

Updated the long-lived-framework example.


Bugs: MESOS-5062
https://issues.apache.org/jira/browse/MESOS-5062


Repository: mesos


Description (updated)
---

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 
ef498d63bc5f0a8deb46d71edd85a76a1d38fdd0 

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

Re: Review Request 45067: Updated the long-lived-framework example.

[Vinod Kone]

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


Fix it, then Ship it!





src/examples/long_lived_framework.cpp (line 155)


no need for quotes around slaveId since it is generated by mesos and we 
know it doesn't have spaces.



src/examples/long_lived_framework.cpp (line 163)


// Helper to decline an offer.



src/examples/long_lived_framework.cpp (line 172)


// Helper to launch a task using the offer.


- Vinod Kone


On April 5, 2016, 10:40 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45067/
> ---
> 
> (Updated April 5, 2016, 10:40 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5062
> https://issues.apache.org/jira/browse/MESOS-5062
> 
> 
> 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 
> ef498d63bc5f0a8deb46d71edd85a76a1d38fdd0 
> 
> 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
> 
>

Re: Review Request 45067: Updated the long-lived-framework example.

[Joseph Wu]

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

(Updated April 5, 2016, 3:40 p.m.)


Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod 
Kone.


Changes
---

Missed a line while refactoring the `resourceOffers` method.


Bugs: MESOS-5062
https://issues.apache.org/jira/browse/MESOS-5062


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 (updated)
-

  src/examples/long_lived_framework.cpp 
ef498d63bc5f0a8deb46d71edd85a76a1d38fdd0 

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

Re: Review Request 45067: Updated the long-lived-framework example.

[Joseph Wu]

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

(Updated April 5, 2016, 3:33 p.m.)


Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod 
Kone.


Changes
---

Addressed Vinod's comments.


Bugs: MESOS-5062
https://issues.apache.org/jira/browse/MESOS-5062


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 (updated)
-

  src/examples/long_lived_framework.cpp 
ef498d63bc5f0a8deb46d71edd85a76a1d38fdd0 

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

Re: Review Request 45067: Updated the long-lived-framework example.

[Vinod Kone]

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




src/examples/long_lived_framework.cpp (line 51)


I think you should add a comment here about what the high level behavior of 
this scheduler is. For example that this schedule tries to pick one slave and 
launch as many tasks on it as possible, using a single multi-task executor. And 
that if the slave or executor fails, it picks another slave and repeats the 
process.



src/examples/long_lived_framework.cpp (line 64)


Any particular reason you are using "cout" instead of LOG? It would be 
great to have timestamps, esp if you are planning to run this in a test cluster.



src/examples/long_lived_framework.cpp (line 86)


Can we use "foreach(const Offer& offer, offers)" here?



src/examples/long_lived_framework.cpp (line 87)


why do you need a vector here? AFAICT, you only ever call launchTasks with 
a single task. If that's the case just pass '{task}' to driver.launchTasks().



src/examples/long_lived_framework.cpp (line 92)


s/offer/offer is/



src/examples/long_lived_framework.cpp (lines 104 - 150)


This is still a bit hard to follow. 

How about:

```
foreach (const Offer& offer : offers) {

  if (slaveID.isNone() {
// No active executor running in the cluster. Launch a new task with 
executor.
  
if (offer.resources < (task + executor).resources()) {
   // Not enough resources. Decline.
   decline();
} else {
   launch();
}
  } else if (slaveID == offer.slaveID() {
// Offer from the same slave that has an active executor. Launch more 
tasks on that executor.

if (offer.resources < task.executors()) {
  // Noe enough resources. Decline.
  decline();
} else {
  launch();
}
  } else {
// Offer from a slave different from the slave that has an active 
executor. Decline.
decline();
  }
}

```



src/examples/long_lived_framework.cpp (line 177)


log a message here.



src/examples/long_lived_framework.cpp (line 186)


log a message here.


- Vinod Kone


On March 29, 2016, 7:08 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45067/
> ---
> 
> (Updated March 29, 2016, 7:08 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5062
> https://issues.apache.org/jira/browse/MESOS-5062
> 
> 
> 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 
> ef498d63bc5f0a8deb46d71edd85a76a1d38fdd0 
> 
> 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
> 
>

Re: Review Request 45067: Updated the long-lived-framework example.

[Joseph Wu]

> On March 28, 2016, 5:31 p.m., Vinod Kone wrote:
> > src/examples/long_lived_framework.cpp, line 77
> > 
> >
> > s/. Reject/. Reject/ <-- extraneous space

Oops, that's a habit (and the Apache license uses 2 spaces :)


> On March 28, 2016, 5:31 p.m., Vinod Kone wrote:
> > src/examples/long_lived_framework.cpp, lines 95-96
> > 
> >
> > 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.

We actually do expect the same executor to be used for each task.  (The 
executor's ID is "default".)

There are really 5 cases here:
1) Executor running & offer from different slave.
2) Executor running & offer not big enough for a task.
3) Executor running & offer from same slave & offer big enough for task.
4) Executor not running & offer not big enough for task + executor.
5) Executor not running & offer big enough for task + executor.


> On March 28, 2016, 5:31 p.m., Vinod Kone wrote:
> > src/examples/long_lived_framework.cpp, line 173
> > 
> >
> > why is this called "residency" instead of "slaveID" ?

I wanted a variable name that somehow expressed where the `long-lived-executor` 
was "living".  But `slaveId` would work too.


> On March 28, 2016, 5:31 p.m., Vinod Kone wrote:
> > src/examples/long_lived_framework.cpp, line 196
> > 
> >
> > why does the framework assume master lives on the same machine?

Hm, I guess the location of the master doesn't really play a role here.  
Removed from comment.


> On March 28, 2016, 5:31 p.m., Vinod Kone wrote:
> > src/examples/long_lived_framework.cpp, line 151
> > 
> >
> > I thought we could compare option directly with a value instead of 
> > doing a .get()?

Yup, my mistake.


> On March 28, 2016, 5:31 p.m., Vinod Kone wrote:
> > src/examples/long_lived_framework.cpp, line 160
> > 
> >
> > 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`.

As mentioned above, this framework only launches on executor with ID "default".


- Joseph


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


On March 29, 2016, 12:08 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45067/
> ---
> 
> (Updated March 29, 2016, 12:08 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5062
> https://issues.apache.org/jira/browse/MESOS-5062
> 
> 
> 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 
> ef498d63bc5f0a8deb46d71edd85a76a1d38fdd0 
> 
> Diff: https://reviews.apache.org/r/45067/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Ran this on the master node on a Mesos cluster:
> ```
> ./long-li

Re: Review Request 45067: Updated the long-lived-framework example.

[Joseph Wu]

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

(Updated March 29, 2016, 12:08 p.m.)


Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod 
Kone.


Changes
---

Address comments from Vinod.  Reworked how/when offers are accepted.


Bugs: MESOS-5062
https://issues.apache.org/jira/browse/MESOS-5062


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 (updated)
-

  src/examples/long_lived_framework.cpp 
ef498d63bc5f0a8deb46d71edd85a76a1d38fdd0 

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

Re: Review Request 45067: Updated the long-lived-framework example.

[Vinod Kone]

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


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)


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



src/examples/long_lived_framework.cpp (lines 84 - 85)


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)


TASK_RESOURCES + EXEcUTOR_RESOURCES



src/examples/long_lived_framework.cpp (line 128)


s/sid/slaveID/



src/examples/long_lived_framework.cpp (line 129)


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



src/examples/long_lived_framework.cpp (line 138)


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



src/examples/long_lived_framework.cpp (line 151)


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



src/examples/long_lived_framework.cpp (line 174)


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



src/examples/long_lived_framework.cpp (line 184)


s/mesos// ?



src/examples/long_lived_framework.cpp (line 212)


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



src/examples/long_lived_framework.cpp (lines 213 - 214)


use "*_PER_EXECUTOR" here



src/examples/long_lived_framework.cpp (lines 222 - 242)


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)


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 
> 289a0b9dd3d1ce30f20dd9bb381126bff30c 
> 
> 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_

Re: Review Request 45067: Updated the long-lived-framework example.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45067]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


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

Re: Review Request 45067: Updated the long-lived-framework example.

[Joseph Wu]

> On March 18, 2016, 6:35 p.m., Neil Conway wrote:
> > src/examples/long_lived_framework.cpp, line 135
> > 
> >
> > Don't we need `TaskState_Name` here?

Yeah, for some reason, I thought that used an internal header.  Added it back.


- Joseph


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


On March 23, 2016, 3: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, 3: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 
> 289a0b9dd3d1ce30f20dd9bb381126bff30c 
> 
> 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
> 
>

Re: Review Request 45067: Updated the long-lived-framework example.

[Joseph Wu]

> On March 18, 2016, 11:42 p.m., haosdent huang wrote:
> > src/examples/long_lived_framework.cpp, line 168
> > 
> >
> > I think use
> > ```
> >   if (flags.master.isNone()) {
> > EXIT(EXIT_FAILURE)
> >   << flags.usage("Missing required option --master");
> >   }
> > ```
> > to keep consistent with current codebase would be better

The flag validation lambdas are somewhat newer stylistically, but they are the 
recommended way of adding flag validation now.

As the for the `EXIT`, we exit below:
```
  if (load.isError()) {
EXIT(1) << flags.usage(load.error());
  }
```
(I'll change the `1` to `EXIT_FAILURE`.)


> On March 18, 2016, 11:42 p.m., haosdent huang wrote:
> > src/examples/long_lived_framework.cpp, line 177
> > 
> >
> > Should use `build_dir`?

Ah, yes.  Missed that :)


- Joseph


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


On March 23, 2016, 3: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, 3: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 
> 289a0b9dd3d1ce30f20dd9bb381126bff30c 
> 
> 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
> 
>

Re: Review Request 45067: Updated the long-lived-framework example.

[Joseph Wu]

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

(Updated March 23, 2016, 3:50 p.m.)


Review request for mesos, Anand Mazumdar, Artem Harutyunyan, and Kevin Klues.


Changes
---

A couple more updates to make this framework play nicer on an actual cluster.  
Notably filters and a self-imposed agent restriction.


Repository: mesos


Description (updated)
---

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 (updated)
-

  src/examples/long_lived_framework.cpp 
289a0b9dd3d1ce30f20dd9bb381126bff30c 

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

Re: Review Request 45067: Updated the long-lived-framework example.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45067]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 19, 2016, 1:06 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45067/
> ---
> 
> (Updated March 19, 2016, 1:06 a.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 and allows the 
> `ExecutorInfo` to be run with some cgroups isolators.
> 
> 
> Diffs
> -
> 
>   src/examples/long_lived_framework.cpp 
> 289a0b9dd3d1ce30f20dd9bb381126bff30c 
> 
> 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
> 
>

Re: Review Request 45067: Updated the long-lived-framework example.

[haosdent huang]

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


Fix it, then Ship it!




Ship It!


src/examples/long_lived_framework.cpp (line 135)


I think use
```
  if (flags.master.isNone()) {
EXIT(EXIT_FAILURE)
  << flags.usage("Missing required option --master");
  }
```
to keep consistent with current codebase would be better



src/examples/long_lived_framework.cpp (line 144)


Should use `build_dir`?



src/examples/long_lived_framework.cpp (line 145)


There is a duplicated space between `Mesos.  If`. I think should be `Mesos. 
If`.



src/examples/long_lived_framework.cpp (line 186)


Seems we never use `flags::mesos_build_dir`. By the way, if we use 
`flags.load(PREFIX)`, it would load the flag by environment variable as well so 
that we no need to check it here again.



src/examples/long_lived_framework.cpp (line 210)


I suggest pull down
```
  // Find this executable's directory to locate executor.
  string executor_command;
  Option value = os::getenv("MESOS_BUILD_DIR");
  if (value.isSome()) {
executor_command = path::join(value.get(), "src", 
"long-lived-executor");
  } else {
executor_command = path::join(
os::realpath(Path(argv[0]).dirname()).get(),
"long-lived-executor");
  }
```
and
```
executor.mutable_command()->set_value(executor_command);
```
here, because they are a logic block to set `executor.mutable_command()`


- haosdent huang


On March 19, 2016, 1:06 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45067/
> ---
> 
> (Updated March 19, 2016, 1:06 a.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 and allows the 
> `ExecutorInfo` to be run with some cgroups isolators.
> 
> 
> Diffs
> -
> 
>   src/examples/long_lived_framework.cpp 
> 289a0b9dd3d1ce30f20dd9bb381126bff30c 
> 
> 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
> 
>

Re: Review Request 45067: Updated the long-lived-framework example.

[Neil Conway]

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




src/examples/long_lived_framework.cpp (line 102)


Don't we need `TaskState_Name` here?


- Neil Conway


On March 19, 2016, 1:06 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45067/
> ---
> 
> (Updated March 19, 2016, 1:06 a.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 and allows the 
> `ExecutorInfo` to be run with some cgroups isolators.
> 
> 
> Diffs
> -
> 
>   src/examples/long_lived_framework.cpp 
> 289a0b9dd3d1ce30f20dd9bb381126bff30c 
> 
> 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
> 
>

Review Request 45067: Updated the long-lived-framework example.

[Joseph Wu]

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

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 and allows the `ExecutorInfo` 
to be run with some cgroups isolators.


Diffs
-

  src/examples/long_lived_framework.cpp 
289a0b9dd3d1ce30f20dd9bb381126bff30c 

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