You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2019/05/01 21:17:55 UTC
Review Request 70579: Added a Task Scheduler to simplify testing.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70579/
-----------------------------------------------------------
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Bugs: MESOS-8511
https://issues.apache.org/jira/browse/MESOS-8511
Repository: mesos
Description
-------
The caller can submit tasks to the scheduler and get back statuses.
This reduces the boilerplate of using the low level scheduler
library.
Diffs
-----
include/mesos/v1/scheduler.hpp 2f72129ba48943c891d7020bfd2cad3066355026
src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
src/scheduler/scheduler.cpp 674483aa80bc74b654343c97892a96f49d5c7ed4
src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
src/tests/utils/task_scheduler.hpp PRE-CREATION
src/tests/utils/task_scheduler.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/70579/diff/1/
Testing
-------
Tested it via the subsequent patch.
Thanks,
Benjamin Mahler
Re: Review Request 70579: Added a Task Scheduler to simplify testing.
Posted by Benjamin Mahler <bm...@apache.org>.
> On May 10, 2019, 3:31 a.m., Chun-Hung Hsiao wrote:
> > src/scheduler/scheduler.cpp
> > Lines 1010 (patched)
> > <https://reviews.apache.org/r/70579/diff/1/?file=2142140#file2142140line1026>
> >
> > Instead of introducing this, we could change the `TestMesos` template in `src/tests/mesos.hpp` to the following:
> > ```
> > namespace scheduler {
> >
> > template<typename Mesos, typename Scheduler>
> > class TestMesos : public Mesos
> > {
> > ...
> > };
> >
> > } // namespace scheduler
> >
> > namespace v1 {
> > namespace scheduler {
> >
> > using TestMesos = tests::scheduler::TestMesos<
> > mesos::v1::scheduler::Mesos,
> > MockHTTPSchedluer<
> > mesos::v1::scheduler::Mesos,
> > mesos::v1::scheduler::Event>>;
> >
> > } // namespace scheduler
> > } // namespace v1
> > ```
> >
> > Then we can use `tests::scheduler::TestMesos` in `TaskScheduler`.
Hm.. I'm lost. What does that accomplish?
This patch is de-coupling construction from initialization, which IMO should have been done in the first place. Currently, you have to pass the callbacks when contructing and initialization of the Mesos class will happen immediately. The callbacks might not be ready to be invoked! Also, the caller may want to control when to "start" rather than it starting implicitly during construction.
Is this what you understood from the change?
> On May 10, 2019, 3:31 a.m., Chun-Hung Hsiao wrote:
> > src/tests/utils/task_scheduler.hpp
> > Lines 17 (patched)
> > <https://reviews.apache.org/r/70579/diff/1/?file=2142142#file2142142line17>
> >
> > Would it be more consistent if we put this file at `src/tests/scheduler.hpp`, and call the scheduler `TestScheduler`, just like what we have in `src/tests/allocator.hpp`?
> >
> > BTW we already have `src/tests/utils.hpp`, which has the same prefix `src/tests/utils`, not sure if we want to avoid this.
The gigantic tests/ folder is a disaster. I don't know about you, but it seems to me it sure could benefit from some more organization.
For example, I wish all the mocks were clearly organized into a mocks/ folder.
> BTW we already have src/tests/utils.hpp, which has the same prefix src/tests/utils, not sure if we want to avoid this.
Yes I was aware of this, I don't think utils.hpp is a good file. For example, most of the functions in there are related to "paths" and could be organized accordingly. The metrics and port/ip helpers could also be put into a more appropriately named file?
> On May 10, 2019, 3:31 a.m., Chun-Hung Hsiao wrote:
> > src/tests/utils/task_scheduler.cpp
> > Lines 70 (patched)
> > <https://reviews.apache.org/r/70579/diff/1/?file=2142143#file2142143line70>
> >
> > How about the following:
> >
> > struct Launchable
> > {
> > virtual v1::Resources resources() const = 0;
> > virtual v1::Offer::Operation operation(...) = 0;
> > };
> >
> > struct Task : public Launchable
> > {
> > ...
> > };
> >
> > struct TaskGroup : public Launchable
> > {
> > ...
> > };
> >
> > std::queue<Launchable> launchQueue;
That won't work since the objects are getting sliced in this example. You'd have to do something like `queue<Owned<Launchable>>`.
Personally, I would rather use a variant here than inheritance to keep value semantics. I forgot we already have variant, so I'll use that instead of the two options!
> On May 10, 2019, 3:31 a.m., Chun-Hung Hsiao wrote:
> > src/tests/utils/task_scheduler.cpp
> > Lines 265-268 (patched)
> > <https://reviews.apache.org/r/70579/diff/1/?file=2142143#file2142143line265>
> >
> > ```
> > case ...::HEARTBEAT:
> > case ...::UPDATE_OPERATION_STATUS:
> > case ...::INVERSE_OFFERS:
> > case ...::RESCIND_INVERSE_OFFERS:
> > break;
> > ```
> > Also let's move this to the end and merge with the other two events.
> >
> > If you prefer to keep the order and use multiple `break`s, let's just leave one space between the colon and `break`.
I'll move them down
> If you prefer to keep the order and use multiple breaks, let's just leave one space between the colon and break.
We often use whitespace alignment to improve readability, I think that's a good thing since readability is a top priority, some examples:
https://github.com/apache/mesos/blob/004fb5fa27c2992b11a2fa51a8ec5a3f3de404db/src/linux/capabilities.cpp#L153-L212
https://github.com/apache/mesos/blob/004fb5fa27c2992b11a2fa51a8ec5a3f3de404db/src/v1/attributes.cpp#L40-L43
https://github.com/apache/mesos/blob/004fb5fa27c2992b11a2fa51a8ec5a3f3de404db/3rdparty/stout/include/stout/gzip.hpp#L60-L69
> On May 10, 2019, 3:31 a.m., Chun-Hung Hsiao wrote:
> > src/tests/utils/task_scheduler.cpp
> > Lines 289 (patched)
> > <https://reviews.apache.org/r/70579/diff/1/?file=2142143#file2142143line289>
> >
> > ```
> > offers_.erase(std::remove_if(...), offers_.end());
> > ```
> > Although the effect is the same since there will be only one element removed, pairing `std::remove_if` and `end` will be more idomatic.
Hm.. why is this an "issue" and why is that more idiomatic..? Erase has two versions, one take a position and one takes a range.
Looking at this code again, it's rather unreadable, and I probably will just write the version with a loop to find the one to erase.
> On May 10, 2019, 3:31 a.m., Chun-Hung Hsiao wrote:
> > src/tests/utils/task_scheduler.cpp
> > Lines 409 (patched)
> > <https://reviews.apache.org/r/70579/diff/1/?file=2142143#file2142143line409>
> >
> > Do you want to support launching more than one items within a single offer?
Not yet, just wanted to keep it simple to start with and have it try to launch 1 item at a time.
> On May 10, 2019, 3:31 a.m., Chun-Hung Hsiao wrote:
> > src/tests/utils/task_scheduler.cpp
> > Lines 456 (patched)
> > <https://reviews.apache.org/r/70579/diff/1/?file=2142143#file2142143line456>
> >
> > Instead of doing the wiring, we could instead make `TaskSchedulerProcess::submit` to take `output` as its second argument.
Can you spell out how that would avoid the wiring? Not seeing it..
> On May 10, 2019, 3:31 a.m., Chun-Hung Hsiao wrote:
> > src/tests/utils/task_scheduler.cpp
> > Lines 486 (patched)
> > <https://reviews.apache.org/r/70579/diff/1/?file=2142143#file2142143line486>
> >
> > We could avoid this wiring by making `TaskSchedulerProcess::submit` take a second `hashmap<TaskID, Queue<TaskStatus>` parameter.
Ditto, how so?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70579/#review215174
-----------------------------------------------------------
On May 1, 2019, 9:17 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70579/
> -----------------------------------------------------------
>
> (Updated May 1, 2019, 9:17 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Bugs: MESOS-8511
> https://issues.apache.org/jira/browse/MESOS-8511
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The caller can submit tasks to the scheduler and get back statuses.
> This reduces the boilerplate of using the low level scheduler
> library.
>
>
> Diffs
> -----
>
> include/mesos/v1/scheduler.hpp 2f72129ba48943c891d7020bfd2cad3066355026
> src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
> src/scheduler/scheduler.cpp 674483aa80bc74b654343c97892a96f49d5c7ed4
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/utils/task_scheduler.hpp PRE-CREATION
> src/tests/utils/task_scheduler.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/70579/diff/1/
>
>
> Testing
> -------
>
> Tested it via the subsequent patch.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 70579: Added a Task Scheduler to simplify testing.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On May 10, 2019, 3:31 a.m., Chun-Hung Hsiao wrote:
> > src/tests/utils/task_scheduler.cpp
> > Lines 289 (patched)
> > <https://reviews.apache.org/r/70579/diff/1/?file=2142143#file2142143line289>
> >
> > ```
> > offers_.erase(std::remove_if(...), offers_.end());
> > ```
> > Although the effect is the same since there will be only one element removed, pairing `std::remove_if` and `end` will be more idomatic.
>
> Benjamin Mahler wrote:
> Hm.. why is this an "issue" and why is that more idiomatic..? Erase has two versions, one take a position and one takes a range.
>
> Looking at this code again, it's rather unreadable, and I probably will just write the version with a loop to find the one to erase.
The reason it's more idiomatic is because `it = std::remove_if(...)` returns an iterator that points to the new end of valid items, which means that, `[it, offers_.end())` should be removed. Since internally `std::remove_if` moves items past `it` to their "correct" positions before `it`, erasing only `it` but not things after it seems very strange at a first glance.
Calling `erase` with one iterator works here because we're relying on the fact that `std::remove_if` will only remove one item in this particular case, which is not trivial to me (before looking into what `offers_` contains and when the lambda returns true.
If you prefer to use `erase` with one position, how about the following:
```
offers_.erase(std::find_if(
offers_.begin(),
offers_.end(),
[&](const v1::Offer& offer) {
...
}));
```
Also, I just noticed that both the snippet you have and the one I wrote above relies on the invariant where `event.rescind().offer_id()` exists in `offers_`. `vector::erase(pos)` requires that the iterator passed in should be both valid and deferenceable. If there are more than one agents, then this invariant will be violated.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70579/#review215174
-----------------------------------------------------------
On May 1, 2019, 9:17 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70579/
> -----------------------------------------------------------
>
> (Updated May 1, 2019, 9:17 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Bugs: MESOS-8511
> https://issues.apache.org/jira/browse/MESOS-8511
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The caller can submit tasks to the scheduler and get back statuses.
> This reduces the boilerplate of using the low level scheduler
> library.
>
>
> Diffs
> -----
>
> include/mesos/v1/scheduler.hpp 2f72129ba48943c891d7020bfd2cad3066355026
> src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
> src/scheduler/scheduler.cpp 674483aa80bc74b654343c97892a96f49d5c7ed4
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/utils/task_scheduler.hpp PRE-CREATION
> src/tests/utils/task_scheduler.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/70579/diff/1/
>
>
> Testing
> -------
>
> Tested it via the subsequent patch.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 70579: Added a Task Scheduler to simplify testing.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70579/#review215174
-----------------------------------------------------------
Thanks for your work on this test scheduler!
src/scheduler/scheduler.cpp
Lines 1010 (patched)
<https://reviews.apache.org/r/70579/#comment301716>
Instead of introducing this, we could change the `TestMesos` template in `src/tests/mesos.hpp` to the following:
```
namespace scheduler {
template<typename Mesos, typename Scheduler>
class TestMesos : public Mesos
{
...
};
} // namespace scheduler
namespace v1 {
namespace scheduler {
using TestMesos = tests::scheduler::TestMesos<
mesos::v1::scheduler::Mesos,
MockHTTPSchedluer<
mesos::v1::scheduler::Mesos,
mesos::v1::scheduler::Event>>;
} // namespace scheduler
} // namespace v1
```
Then we can use `tests::scheduler::TestMesos` in `TaskScheduler`.
src/tests/utils/task_scheduler.hpp
Lines 17 (patched)
<https://reviews.apache.org/r/70579/#comment301711>
Would it be more consistent if we put this file at `src/tests/scheduler.hpp`, and call the scheduler `TestScheduler`, just like what we have in `src/tests/allocator.hpp`?
BTW we already have `src/tests/utils.hpp`, which has the same prefix `src/tests/utils`, not sure if we want to avoid this.
src/tests/utils/task_scheduler.cpp
Lines 70 (patched)
<https://reviews.apache.org/r/70579/#comment301712>
How about the following:
struct Launchable
{
virtual v1::Resources resources() const = 0;
virtual v1::Offer::Operation operation(...) = 0;
};
struct Task : public Launchable
{
...
};
struct TaskGroup : public Launchable
{
...
};
std::queue<Launchable> launchQueue;
src/tests/utils/task_scheduler.cpp
Lines 86 (patched)
<https://reviews.apache.org/r/70579/#comment301726>
If we take the `TestMesos` suggestion I proposed above, we won't need to store the master pid.
src/tests/utils/task_scheduler.cpp
Lines 90 (patched)
<https://reviews.apache.org/r/70579/#comment301715>
According to the style guide, this should be `subscribed_`.
src/tests/utils/task_scheduler.cpp
Lines 114 (patched)
<https://reviews.apache.org/r/70579/#comment301724>
It seems to me that `TaskScheduler` is redundent since glog will prepend the log message with the file name.
Ditto for all logging below.
src/tests/utils/task_scheduler.cpp
Lines 133 (patched)
<https://reviews.apache.org/r/70579/#comment301713>
Would it make sense to just use `DEFAULT_FRAMEWORK_INFO`?
src/tests/utils/task_scheduler.cpp
Lines 144 (patched)
<https://reviews.apache.org/r/70579/#comment301714>
`createCallSubscribe(frameworkInfo)`.
src/tests/utils/task_scheduler.cpp
Lines 158 (patched)
<https://reviews.apache.org/r/70579/#comment301717>
How about `frameworkInfo.roles(0)`? Ditto below for all `allocate("*")`.
src/tests/utils/task_scheduler.cpp
Lines 265-268 (patched)
<https://reviews.apache.org/r/70579/#comment301718>
```
case ...::HEARTBEAT:
case ...::UPDATE_OPERATION_STATUS:
case ...::INVERSE_OFFERS:
case ...::RESCIND_INVERSE_OFFERS:
break;
```
Also let's move this to the end and merge with the other two events.
If you prefer to keep the order and use multiple `break`s, let's just leave one space between the colon and `break`.
src/tests/utils/task_scheduler.cpp
Lines 286 (patched)
<https://reviews.apache.org/r/70579/#comment301719>
break the line apart
src/tests/utils/task_scheduler.cpp
Lines 289 (patched)
<https://reviews.apache.org/r/70579/#comment301721>
```
offers_.erase(std::remove_if(...), offers_.end());
```
Although the effect is the same since there will be only one element removed, pairing `std::remove_if` and `end` will be more idomatic.
src/tests/utils/task_scheduler.cpp
Lines 306 (patched)
<https://reviews.apache.org/r/70579/#comment301722>
`createCallAcknowledge(frameworkId, status.agent_id(), event.update())`
src/tests/utils/task_scheduler.cpp
Lines 332 (patched)
<https://reviews.apache.org/r/70579/#comment301723>
How about just `JSON::protobuf(event.failure())`?
src/tests/utils/task_scheduler.cpp
Lines 381 (patched)
<https://reviews.apache.org/r/70579/#comment301720>
Unfortunately there is no `createCallAccept` helper that takes a list of offers. How about adding one in `src/tests/mesos.hpp` then using it here?
src/tests/utils/task_scheduler.cpp
Lines 409 (patched)
<https://reviews.apache.org/r/70579/#comment301725>
Do you want to support launching more than one items within a single offer?
src/tests/utils/task_scheduler.cpp
Lines 456 (patched)
<https://reviews.apache.org/r/70579/#comment301727>
Instead of doing the wiring, we could instead make `TaskSchedulerProcess::submit` to take `output` as its second argument.
src/tests/utils/task_scheduler.cpp
Lines 486 (patched)
<https://reviews.apache.org/r/70579/#comment301728>
We could avoid this wiring by making `TaskSchedulerProcess::submit` take a second `hashmap<TaskID, Queue<TaskStatus>` parameter.
- Chun-Hung Hsiao
On May 1, 2019, 9:17 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70579/
> -----------------------------------------------------------
>
> (Updated May 1, 2019, 9:17 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Bugs: MESOS-8511
> https://issues.apache.org/jira/browse/MESOS-8511
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The caller can submit tasks to the scheduler and get back statuses.
> This reduces the boilerplate of using the low level scheduler
> library.
>
>
> Diffs
> -----
>
> include/mesos/v1/scheduler.hpp 2f72129ba48943c891d7020bfd2cad3066355026
> src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
> src/scheduler/scheduler.cpp 674483aa80bc74b654343c97892a96f49d5c7ed4
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/utils/task_scheduler.hpp PRE-CREATION
> src/tests/utils/task_scheduler.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/70579/diff/1/
>
>
> Testing
> -------
>
> Tested it via the subsequent patch.
>
>
> Thanks,
>
> Benjamin Mahler
>
>