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