mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 70579: Added a Task Scheduler to simplify testing.
Date Fri, 10 May 2019 03:31:18 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message