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 19:30:35 GMT


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


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