You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2016/04/08 00:59:17 UTC
Review Request 45604: Updated the balloon framework and executor.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45604/
-----------------------------------------------------------
Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
Repository: mesos
Description
-------
This gives the example `balloon-framework` enough options to run
outside of the build environment.
This also updates:
* The style of the framework code.
* Adds an option for restricting the number of resources per task
(otherwise, it will eat up an entire node).
* Adds an option for persisting the framework and launching one task
after another.
* Adds filters for declined offers.
* Adds shutdown logic to the executor.
Diffs
-----
src/examples/balloon_executor.cpp 399218478a596387814d123290eeeb33102fad7a
src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3
src/tests/balloon_framework_test.sh ae32753e8942f77f94752543c384d218d6e4d48d
Diff: https://reviews.apache.org/r/45604/diff/
Testing
-------
```
make check
sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
```
Thanks,
Joseph Wu
Re: Review Request 45604: Updated the balloon framework and executor.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On April 15, 2016, 2:13 p.m., Vinod Kone wrote:
> > src/examples/balloon_framework.cpp, lines 117-118
> > <https://reviews.apache.org/r/45604/diff/2-3/?file=1337948#file1337948line117>
> >
> > why would they be incomplete?
There will be several metrics that count the number of tasks run (successfully or not) over time. If the framework exits after one task, these counters will be meaningless (always 0). I guess this is sort of implied, so I'll remove this line.
> On April 15, 2016, 2:13 p.m., Vinod Kone wrote:
> > src/examples/balloon_framework.cpp, lines 67-69
> > <https://reviews.apache.org/r/45604/diff/2-3/?file=1337948#file1337948line67>
> >
> > Just change the type of task_memory_usage_limit from Option<Bytes> to Bytes; no need for this validation then.
Counter-intuitively, specifying `Option<T>` with a lambda is how you specify a required argument. A flag without an `Option<>` must have a default value, and is therefore an optional flag. :)
> On April 15, 2016, 2:13 p.m., Vinod Kone wrote:
> > src/examples/balloon_framework.cpp, lines 85-91
> > <https://reviews.apache.org/r/45604/diff/2-3/?file=1337948#file1337948line85>
> >
> > Ditto. Change `task_memory` to Bytes instead of Option<Bytes>.
Dropping for the same reason as above
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45604/#review129187
-----------------------------------------------------------
On April 19, 2016, 2:51 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45604/
> -----------------------------------------------------------
>
> (Updated April 19, 2016, 2:51 p.m.)
>
>
> Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-5174
> https://issues.apache.org/jira/browse/MESOS-5174
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives the example `balloon-framework` enough options to run
> outside of the build environment.
>
> This also updates:
>
> * The style of the framework code.
> * Adds an option for restricting the number of resources per task
> (otherwise, it will eat up an entire node).
> * Adds an option for persisting the framework and launching one task
> after another.
> * Adds filters for declined offers.
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3
> src/tests/balloon_framework_test.sh a242f6cb9ca1850e5fef90e0938f41044bdaddbf
>
> Diff: https://reviews.apache.org/r/45604/diff/
>
>
> Testing
> -------
>
> ```
> make check
>
> sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 45604: Updated the balloon framework and executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45604/#review129187
-----------------------------------------------------------
src/examples/balloon_framework.cpp (lines 63 - 64)
<https://reviews.apache.org/r/45604/#comment192641>
Kill these two lines.
src/examples/balloon_framework.cpp (lines 67 - 69)
<https://reviews.apache.org/r/45604/#comment192642>
Just change the type of task_memory_usage_limit from Option<Bytes> to Bytes; no need for this validation then.
src/examples/balloon_framework.cpp (lines 85 - 91)
<https://reviews.apache.org/r/45604/#comment192640>
Ditto. Change `task_memory` to Bytes instead of Option<Bytes>.
src/examples/balloon_framework.cpp (line 115)
<https://reviews.apache.org/r/45604/#comment192643>
s/should persist and//
src/examples/balloon_framework.cpp (lines 117 - 118)
<https://reviews.apache.org/r/45604/#comment192644>
why would they be incomplete?
- Vinod Kone
On April 14, 2016, 9:43 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45604/
> -----------------------------------------------------------
>
> (Updated April 14, 2016, 9:43 p.m.)
>
>
> Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-5174
> https://issues.apache.org/jira/browse/MESOS-5174
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives the example `balloon-framework` enough options to run
> outside of the build environment.
>
> This also updates:
>
> * The style of the framework code.
> * Adds an option for restricting the number of resources per task
> (otherwise, it will eat up an entire node).
> * Adds an option for persisting the framework and launching one task
> after another.
> * Adds filters for declined offers.
> * Refines the shutdown logic for the executor. In particular, ironed
> out bugs when the balloon executor does not exceed the memory limit.
>
>
> Diffs
> -----
>
> src/examples/balloon_executor.cpp 399218478a596387814d123290eeeb33102fad7a
> src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3
> src/tests/balloon_framework_test.sh ae32753e8942f77f94752543c384d218d6e4d48d
>
> Diff: https://reviews.apache.org/r/45604/diff/
>
>
> Testing
> -------
>
> ```
> make check
>
> sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 45604: Updated the balloon framework and executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45604/#review129962
-----------------------------------------------------------
This review could've used further splits. As a rule of thumb, if your description says there are 3 different things in this review, it is a strong indication that those 3 could have been done in their own reviews.
src/examples/balloon_framework.cpp (lines 83 - 84)
<https://reviews.apache.org/r/45604/#comment193525>
Kill this line because it is required?
src/examples/balloon_framework.cpp (line 136)
<https://reviews.apache.org/r/45604/#comment193527>
s/size of/resource limits set/
- Vinod Kone
On April 19, 2016, 9:51 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45604/
> -----------------------------------------------------------
>
> (Updated April 19, 2016, 9:51 p.m.)
>
>
> Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-5174
> https://issues.apache.org/jira/browse/MESOS-5174
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives the example `balloon-framework` enough options to run
> outside of the build environment.
>
> This also updates:
>
> * The style of the framework code.
> * Adds an option for restricting the number of resources per task
> (otherwise, it will eat up an entire node).
> * Adds an option for persisting the framework and launching one task
> after another.
> * Adds filters for declined offers.
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3
> src/tests/balloon_framework_test.sh a242f6cb9ca1850e5fef90e0938f41044bdaddbf
>
> Diff: https://reviews.apache.org/r/45604/diff/
>
>
> Testing
> -------
>
> ```
> make check
>
> sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 45604: Updated the balloon framework and executor.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45604/
-----------------------------------------------------------
(Updated April 19, 2016, 2:51 p.m.)
Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
Changes
-------
Split up a few parts (as requested in comments) into separate reviews (before and after).
Bugs: MESOS-5174
https://issues.apache.org/jira/browse/MESOS-5174
Repository: mesos
Description (updated)
-------
This gives the example `balloon-framework` enough options to run
outside of the build environment.
This also updates:
* The style of the framework code.
* Adds an option for restricting the number of resources per task
(otherwise, it will eat up an entire node).
* Adds an option for persisting the framework and launching one task
after another.
* Adds filters for declined offers.
Diffs (updated)
-----
src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3
src/tests/balloon_framework_test.sh a242f6cb9ca1850e5fef90e0938f41044bdaddbf
Diff: https://reviews.apache.org/r/45604/diff/
Testing
-------
```
make check
sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
```
Thanks,
Joseph Wu
Re: Review Request 45604: Updated the balloon framework and executor.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45604/
-----------------------------------------------------------
(Updated April 14, 2016, 2:43 p.m.)
Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
Changes
-------
Reworked some flags and variable names + comments.
Bugs: MESOS-5174
https://issues.apache.org/jira/browse/MESOS-5174
Repository: mesos
Description
-------
This gives the example `balloon-framework` enough options to run
outside of the build environment.
This also updates:
* The style of the framework code.
* Adds an option for restricting the number of resources per task
(otherwise, it will eat up an entire node).
* Adds an option for persisting the framework and launching one task
after another.
* Adds filters for declined offers.
* Refines the shutdown logic for the executor. In particular, ironed
out bugs when the balloon executor does not exceed the memory limit.
Diffs (updated)
-----
src/examples/balloon_executor.cpp 399218478a596387814d123290eeeb33102fad7a
src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3
src/tests/balloon_framework_test.sh ae32753e8942f77f94752543c384d218d6e4d48d
Diff: https://reviews.apache.org/r/45604/diff/
Testing
-------
```
make check
sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
```
Thanks,
Joseph Wu
Re: Review Request 45604: Updated the balloon framework and executor.
Posted by Vinod Kone <vi...@gmail.com>.
> On April 12, 2016, 10:46 p.m., Vinod Kone wrote:
> > src/examples/balloon_executor.cpp, lines 143-153
> > <https://reviews.apache.org/r/45604/diff/2/?file=1337947#file1337947line143>
> >
> > Why the change here?
>
> Joseph Wu wrote:
> I couple reasons:
>
> * I needed to put the "ballooning" logic into a separate thread, so that it doesn't block the scheduler driver. If the scheduler driver is blocked, then we have no way of sending a `TASK_FINISHED` status update before `driver->stop()`.
> * Seemed reasonable to move all the task logic into the task thread, after the above change.
> * The comment was then encompassed by the task thread's comment.
I see. Wish you had done this change in a separate review though.
> On April 12, 2016, 10:46 p.m., Vinod Kone wrote:
> > src/examples/balloon_framework.cpp, lines 250-271
> > <https://reviews.apache.org/r/45604/diff/2/?file=1337948#file1337948line250>
> >
> > why the explicit listing of terminal states rather than what we had before? that is future proof if we add new terminal states.
>
> Joseph Wu wrote:
> The `protobuf::isTerminalState` helper is held in an internal header, so it doesn't exactly belong in example code (which, I believe, should be compilable outside of the mesos build chain.)
>
> Do you think we should move that helper (and possibly others) into public headers?
Again, should've been done in a separate review.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45604/#review128539
-----------------------------------------------------------
On April 14, 2016, 9:43 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45604/
> -----------------------------------------------------------
>
> (Updated April 14, 2016, 9:43 p.m.)
>
>
> Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-5174
> https://issues.apache.org/jira/browse/MESOS-5174
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives the example `balloon-framework` enough options to run
> outside of the build environment.
>
> This also updates:
>
> * The style of the framework code.
> * Adds an option for restricting the number of resources per task
> (otherwise, it will eat up an entire node).
> * Adds an option for persisting the framework and launching one task
> after another.
> * Adds filters for declined offers.
> * Refines the shutdown logic for the executor. In particular, ironed
> out bugs when the balloon executor does not exceed the memory limit.
>
>
> Diffs
> -----
>
> src/examples/balloon_executor.cpp 399218478a596387814d123290eeeb33102fad7a
> src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3
> src/tests/balloon_framework_test.sh ae32753e8942f77f94752543c384d218d6e4d48d
>
> Diff: https://reviews.apache.org/r/45604/diff/
>
>
> Testing
> -------
>
> ```
> make check
>
> sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 45604: Updated the balloon framework and executor.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On April 12, 2016, 3:46 p.m., Vinod Kone wrote:
> > src/examples/balloon_framework.cpp, lines 69-90
> > <https://reviews.apache.org/r/45604/diff/2/?file=1337948#file1337948line69>
> >
> > The interaction between these flags is a bit unclear.
> >
> > If some doesn't set `task_memory` but sets `balloon_limit`, the container might or might not OOM. It depends on how much is offerred to the scheduler. I would rather make this a required field. Infact, I would also add a required flag for executor memory (--executor_memory).
> >
> > Also s/balloon_limit/executor_memory_usage/ for clarity.
>
> Joseph Wu wrote:
> I did it this way to maintain the existing behavior (which I agree doesn't make very much sense).
Note: I renamed to `--task_memory_usage_limit`.
> On April 12, 2016, 3:46 p.m., Vinod Kone wrote:
> > src/examples/balloon_framework.cpp, line 330
> > <https://reviews.apache.org/r/45604/diff/2/?file=1337948#file1337948line330>
> >
> > This comment doesn't make sense?
Oops, removed. That was a copy-paste :)
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45604/#review128539
-----------------------------------------------------------
On April 14, 2016, 2:43 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45604/
> -----------------------------------------------------------
>
> (Updated April 14, 2016, 2:43 p.m.)
>
>
> Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-5174
> https://issues.apache.org/jira/browse/MESOS-5174
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives the example `balloon-framework` enough options to run
> outside of the build environment.
>
> This also updates:
>
> * The style of the framework code.
> * Adds an option for restricting the number of resources per task
> (otherwise, it will eat up an entire node).
> * Adds an option for persisting the framework and launching one task
> after another.
> * Adds filters for declined offers.
> * Refines the shutdown logic for the executor. In particular, ironed
> out bugs when the balloon executor does not exceed the memory limit.
>
>
> Diffs
> -----
>
> src/examples/balloon_executor.cpp 399218478a596387814d123290eeeb33102fad7a
> src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3
> src/tests/balloon_framework_test.sh ae32753e8942f77f94752543c384d218d6e4d48d
>
> Diff: https://reviews.apache.org/r/45604/diff/
>
>
> Testing
> -------
>
> ```
> make check
>
> sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 45604: Updated the balloon framework and executor.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On April 12, 2016, 3:46 p.m., Vinod Kone wrote:
> > src/examples/balloon_executor.cpp, lines 143-153
> > <https://reviews.apache.org/r/45604/diff/2/?file=1337947#file1337947line143>
> >
> > Why the change here?
>
> Joseph Wu wrote:
> I couple reasons:
>
> * I needed to put the "ballooning" logic into a separate thread, so that it doesn't block the scheduler driver. If the scheduler driver is blocked, then we have no way of sending a `TASK_FINISHED` status update before `driver->stop()`.
> * Seemed reasonable to move all the task logic into the task thread, after the above change.
> * The comment was then encompassed by the task thread's comment.
>
> Vinod Kone wrote:
> I see. Wish you had done this change in a separate review though.
Pulled into: https://reviews.apache.org/r/46407/
> On April 12, 2016, 3:46 p.m., Vinod Kone wrote:
> > src/examples/balloon_framework.cpp, lines 250-271
> > <https://reviews.apache.org/r/45604/diff/2/?file=1337948#file1337948line250>
> >
> > why the explicit listing of terminal states rather than what we had before? that is future proof if we add new terminal states.
>
> Joseph Wu wrote:
> The `protobuf::isTerminalState` helper is held in an internal header, so it doesn't exactly belong in example code (which, I believe, should be compilable outside of the mesos build chain.)
>
> Do you think we should move that helper (and possibly others) into public headers?
>
> Vinod Kone wrote:
> Again, should've been done in a separate review.
Pulled into: https://reviews.apache.org/r/46411/
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45604/#review128539
-----------------------------------------------------------
On April 19, 2016, 2:51 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45604/
> -----------------------------------------------------------
>
> (Updated April 19, 2016, 2:51 p.m.)
>
>
> Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-5174
> https://issues.apache.org/jira/browse/MESOS-5174
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives the example `balloon-framework` enough options to run
> outside of the build environment.
>
> This also updates:
>
> * The style of the framework code.
> * Adds an option for restricting the number of resources per task
> (otherwise, it will eat up an entire node).
> * Adds an option for persisting the framework and launching one task
> after another.
> * Adds filters for declined offers.
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3
> src/tests/balloon_framework_test.sh a242f6cb9ca1850e5fef90e0938f41044bdaddbf
>
> Diff: https://reviews.apache.org/r/45604/diff/
>
>
> Testing
> -------
>
> ```
> make check
>
> sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 45604: Updated the balloon framework and executor.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On April 12, 2016, 3:46 p.m., Vinod Kone wrote:
> > src/examples/balloon_executor.cpp, lines 143-153
> > <https://reviews.apache.org/r/45604/diff/2/?file=1337947#file1337947line143>
> >
> > Why the change here?
I couple reasons:
* I needed to put the "ballooning" logic into a separate thread, so that it doesn't block the scheduler driver. If the scheduler driver is blocked, then we have no way of sending a `TASK_FINISHED` status update before `driver->stop()`.
* Seemed reasonable to move all the task logic into the task thread, after the above change.
* The comment was then encompassed by the task thread's comment.
> On April 12, 2016, 3:46 p.m., Vinod Kone wrote:
> > src/examples/balloon_framework.cpp, lines 69-90
> > <https://reviews.apache.org/r/45604/diff/2/?file=1337948#file1337948line69>
> >
> > The interaction between these flags is a bit unclear.
> >
> > If some doesn't set `task_memory` but sets `balloon_limit`, the container might or might not OOM. It depends on how much is offerred to the scheduler. I would rather make this a required field. Infact, I would also add a required flag for executor memory (--executor_memory).
> >
> > Also s/balloon_limit/executor_memory_usage/ for clarity.
I did it this way to maintain the existing behavior (which I agree doesn't make very much sense).
> On April 12, 2016, 3:46 p.m., Vinod Kone wrote:
> > src/examples/balloon_framework.cpp, lines 250-271
> > <https://reviews.apache.org/r/45604/diff/2/?file=1337948#file1337948line250>
> >
> > why the explicit listing of terminal states rather than what we had before? that is future proof if we add new terminal states.
The `protobuf::isTerminalState` helper is held in an internal header, so it doesn't exactly belong in example code (which, I believe, should be compilable outside of the mesos build chain.)
Do you think we should move that helper (and possibly others) into public headers?
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45604/#review128539
-----------------------------------------------------------
On April 11, 2016, 11:24 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45604/
> -----------------------------------------------------------
>
> (Updated April 11, 2016, 11:24 a.m.)
>
>
> Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-5174
> https://issues.apache.org/jira/browse/MESOS-5174
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives the example `balloon-framework` enough options to run
> outside of the build environment.
>
> This also updates:
>
> * The style of the framework code.
> * Adds an option for restricting the number of resources per task
> (otherwise, it will eat up an entire node).
> * Adds an option for persisting the framework and launching one task
> after another.
> * Adds filters for declined offers.
> * Refines the shutdown logic for the executor. In particular, ironed
> out bugs when the balloon executor does not exceed the memory limit.
>
>
> Diffs
> -----
>
> src/examples/balloon_executor.cpp 399218478a596387814d123290eeeb33102fad7a
> src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3
> src/tests/balloon_framework_test.sh ae32753e8942f77f94752543c384d218d6e4d48d
>
> Diff: https://reviews.apache.org/r/45604/diff/
>
>
> Testing
> -------
>
> ```
> make check
>
> sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 45604: Updated the balloon framework and executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45604/#review128539
-----------------------------------------------------------
src/examples/balloon_executor.cpp (line 45)
<https://reviews.apache.org/r/45604/#comment191972>
What is the "balloonLimit"?
s/balloonLimit/upper limit/ ?
src/examples/balloon_executor.cpp (line 133)
<https://reviews.apache.org/r/45604/#comment191973>
s/run/run multiple/
src/examples/balloon_executor.cpp
<https://reviews.apache.org/r/45604/#comment192002>
Why the change here?
src/examples/balloon_framework.cpp (lines 60 - 81)
<https://reviews.apache.org/r/45604/#comment192001>
The interaction between these flags is a bit unclear.
If some doesn't set `task_memory` but sets `balloon_limit`, the container might or might not OOM. It depends on how much is offerred to the scheduler. I would rather make this a required field. Infact, I would also add a required flag for executor memory (--executor_memory).
Also s/balloon_limit/executor_memory_usage/ for clarity.
src/examples/balloon_framework.cpp (lines 98 - 102)
<https://reviews.apache.org/r/45604/#comment191985>
Whether a framework wants checkpointing vs whether it should launch more than one task seems like orthogonal issues. Why club them inside one flag?
src/examples/balloon_framework.cpp (lines 235 - 253)
<https://reviews.apache.org/r/45604/#comment191986>
why the explicit listing of terminal states rather than what we had before? that is future proof if we add new terminal states.
src/examples/balloon_framework.cpp (line 243)
<https://reviews.apache.org/r/45604/#comment191989>
`taskLaunched` sounds like something that is not reset for every task launch.
How about calling it `taskActive`?
src/examples/balloon_framework.cpp (line 246)
<https://reviews.apache.org/r/45604/#comment191994>
NOTE: We expect TASK_FAILED when this scheduler is launched by balloon_framework_test.sh shell script. The abort here ensures the script considers the test resutl as 'PASS'.
src/examples/balloon_framework.cpp (line 301)
<https://reviews.apache.org/r/45604/#comment191982>
This comment doesn't make sense?
- Vinod Kone
On April 11, 2016, 6:24 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45604/
> -----------------------------------------------------------
>
> (Updated April 11, 2016, 6:24 p.m.)
>
>
> Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-5174
> https://issues.apache.org/jira/browse/MESOS-5174
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives the example `balloon-framework` enough options to run
> outside of the build environment.
>
> This also updates:
>
> * The style of the framework code.
> * Adds an option for restricting the number of resources per task
> (otherwise, it will eat up an entire node).
> * Adds an option for persisting the framework and launching one task
> after another.
> * Adds filters for declined offers.
> * Refines the shutdown logic for the executor. In particular, ironed
> out bugs when the balloon executor does not exceed the memory limit.
>
>
> Diffs
> -----
>
> src/examples/balloon_executor.cpp 399218478a596387814d123290eeeb33102fad7a
> src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3
> src/tests/balloon_framework_test.sh ae32753e8942f77f94752543c384d218d6e4d48d
>
> Diff: https://reviews.apache.org/r/45604/diff/
>
>
> Testing
> -------
>
> ```
> make check
>
> sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 45604: Updated the balloon framework and executor.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45604/
-----------------------------------------------------------
(Updated April 11, 2016, 11:24 a.m.)
Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
Changes
-------
Added JIRA with summary of problems found while deploying this framework.
Bugs: MESOS-5174
https://issues.apache.org/jira/browse/MESOS-5174
Repository: mesos
Description
-------
This gives the example `balloon-framework` enough options to run
outside of the build environment.
This also updates:
* The style of the framework code.
* Adds an option for restricting the number of resources per task
(otherwise, it will eat up an entire node).
* Adds an option for persisting the framework and launching one task
after another.
* Adds filters for declined offers.
* Refines the shutdown logic for the executor. In particular, ironed
out bugs when the balloon executor does not exceed the memory limit.
Diffs
-----
src/examples/balloon_executor.cpp 399218478a596387814d123290eeeb33102fad7a
src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3
src/tests/balloon_framework_test.sh ae32753e8942f77f94752543c384d218d6e4d48d
Diff: https://reviews.apache.org/r/45604/diff/
Testing
-------
```
make check
sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
```
Thanks,
Joseph Wu
Re: Review Request 45604: Updated the balloon framework and executor.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45604/
-----------------------------------------------------------
(Updated April 8, 2016, 5:32 p.m.)
Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
Repository: mesos
Description
-------
This gives the example `balloon-framework` enough options to run
outside of the build environment.
This also updates:
* The style of the framework code.
* Adds an option for restricting the number of resources per task
(otherwise, it will eat up an entire node).
* Adds an option for persisting the framework and launching one task
after another.
* Adds filters for declined offers.
* Refines the shutdown logic for the executor. In particular, ironed
out bugs when the balloon executor does not exceed the memory limit.
Diffs
-----
src/examples/balloon_executor.cpp 399218478a596387814d123290eeeb33102fad7a
src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3
src/tests/balloon_framework_test.sh ae32753e8942f77f94752543c384d218d6e4d48d
Diff: https://reviews.apache.org/r/45604/diff/
Testing
-------
```
make check
sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
```
Thanks,
Joseph Wu
Re: Review Request 45604: Updated the balloon framework and executor.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45604/
-----------------------------------------------------------
(Updated April 8, 2016, 5:32 p.m.)
Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
Changes
-------
Test and iron out bugs, especially in the executor. Change logging to glog.
Repository: mesos
Description (updated)
-------
This gives the example `balloon-framework` enough options to run
outside of the build environment.
This also updates:
* The style of the framework code.
* Adds an option for restricting the number of resources per task
(otherwise, it will eat up an entire node).
* Adds an option for persisting the framework and launching one task
after another.
* Adds filters for declined offers.
* Refines the shutdown logic for the executor. In particular, ironed
out bugs when the balloon executor does not exceed the memory limit.
Diffs (updated)
-----
src/examples/balloon_executor.cpp 399218478a596387814d123290eeeb33102fad7a
src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3
src/tests/balloon_framework_test.sh ae32753e8942f77f94752543c384d218d6e4d48d
Diff: https://reviews.apache.org/r/45604/diff/
Testing
-------
```
make check
sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
```
Thanks,
Joseph Wu