mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 61058: Added a lock-free event queue.
Date Sun, 30 Jul 2017 00:32:04 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61058/#review181758
-----------------------------------------------------------


Fix it, then Ship it!





3rdparty/libprocess/configure.ac
Lines 72-75 (patched)
<https://reviews.apache.org/r/61058/#comment257430>

    A TODO related to the long term plan for these would be great (i.e. make default and eventually
have as the only option?)
    
    Also, looks like you still need to update `docs/configuration.md` per my existing comment?



3rdparty/libprocess/src/event_queue.hpp
Lines 39-44 (patched)
<https://reviews.apache.org/r/61058/#comment257437>

    Thanks!



3rdparty/libprocess/src/event_queue.hpp
Lines 74 (patched)
<https://reviews.apache.org/r/61058/#comment257439>

    I like that there is a producer and consumer directly accessible, should we hide the Producer/Consumer
constructors?



3rdparty/libprocess/src/event_queue.hpp
Lines 100 (patched)
<https://reviews.apache.org/r/61058/#comment257436>

    Might be a bit easier to read if you define an EventQueue in each ifdef case rather than
split the body of EventQueue across the ifdef?



3rdparty/libprocess/src/event_queue.hpp
Lines 225 (patched)
<https://reviews.apache.org/r/61058/#comment257435>

    s/is/are/



3rdparty/libprocess/src/event_queue.hpp
Lines 256-257 (patched)
<https://reviews.apache.org/r/61058/#comment257434>

    Ditto my comment here and in count() about the number, and should we be consistent about
the bulk dequeue size?



3rdparty/libprocess/src/event_queue.hpp
Lines 322 (patched)
<https://reviews.apache.org/r/61058/#comment257433>

    Just curious why we wouldn't just pass the largest number possible here, e.g. SIZE_MAX.
Is there a tradeoff here where you're trying to bound the amount of work the consumer has
to do upon dequeue? Would be good to clarify in a comment if you have some thoughts on it.



3rdparty/libprocess/src/event_queue.hpp
Lines 333 (patched)
<https://reviews.apache.org/r/61058/#comment257432>

    If you want to be a little more succinct, ~585 years is enough, don't think people need
to see the exact number of nanoseconds :)



3rdparty/libprocess/src/event_queue.hpp
Lines 337-338 (patched)
<https://reviews.apache.org/r/61058/#comment257431>

    I read this as "reading or writing" to the queue, did you mean reading or writing to this
variable or just that there's only a single consumer?


- Benjamin Mahler


On July 29, 2017, 8:52 p.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61058/
> -----------------------------------------------------------
> 
> (Updated July 29, 2017, 8:52 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7798
>     https://issues.apache.org/jira/browse/MESOS-7798
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a lock-free event queue.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/configure.ac cb2cf4f32be5cbdf9df1e32f9aaf2bbba0a5ae03 
>   3rdparty/libprocess/src/event_queue.hpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp b268cdad776a3ca2a87cbe60eb098bde2a70667c 
> 
> 
> Diff: https://reviews.apache.org/r/61058/diff/3/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


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