Re: Review Request 70579: Added a Task Scheduler to simplify testing.

[Chun-Hung Hsiao]

> On May 10, 2019, 3:31 a.m., Chun-Hung Hsiao wrote:
> > src/tests/utils/task_scheduler.cpp
> > Lines 289 (patched)
> > 
> >
> > ```
> > 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.

[Benjamin Mahler]

> On May 10, 2019, 3:31 a.m., Chun-Hung Hsiao wrote:
> > src/scheduler/scheduler.cpp
> > Lines 1010 (patched)
> > 
> >
> > Instead of introducing this, we could change the `TestMesos` template 
> > in `src/tests/mesos.hpp` to the following:
> > ```
> > namespace scheduler {
> > 
> > template
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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 launchQueue;

That won't work since the objects are getting sliced in this example. You'd 
have to do something like `queue>`.

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)
> > 
> >
> > ```
> > 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)
> > 
> >
> > ```
> > offers_.erase(std::remove_if(...), 

Re: Review Request 70579: Added a Task Scheduler to simplify testing.

[Chun-Hung Hsiao]

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


Instead of introducing this, we could change the `TestMesos` template in 
`src/tests/mesos.hpp` to the following:
```
namespace scheduler {

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


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)


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



src/tests/utils/task_scheduler.cpp
Lines 86 (patched)


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)


According to the style guide, this should be `subscribed_`.



src/tests/utils/task_scheduler.cpp
Lines 114 (patched)


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)


Would it make sense to just use `DEFAULT_FRAMEWORK_INFO`?



src/tests/utils/task_scheduler.cpp
Lines 144 (patched)


`createCallSubscribe(frameworkInfo)`.



src/tests/utils/task_scheduler.cpp
Lines 158 (patched)


How about `frameworkInfo.roles(0)`? Ditto below for all `allocate("*")`.



src/tests/utils/task_scheduler.cpp
Lines 265-268 (patched)


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


break the line apart



src/tests/utils/task_scheduler.cpp
Lines 289 (patched)


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


`createCallAcknowledge(frameworkId, status.agent_id(), event.update())`



src/tests/utils/task_scheduler.cpp
Lines 332 (patched)


How about just `JSON::protobuf(event.failure())`?



src/tests/utils/task_scheduler.cpp
Lines 381 (patched)


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)


Do you want to support launching more than one items within a single offer?



src/tests/utils/task_scheduler.cpp
Lines 456 (patched)


Instead of doing the wiring, we could instead make 

Review Request 70579: Added a Task Scheduler to simplify testing.

[Benjamin Mahler]

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