mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benno Evers <bev...@mesosphere.com>
Subject Re: Review Request 69307: Changed master to hold subscribers in a circular buffer.
Date Tue, 27 Nov 2018 23:33:11 GMT

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



The basic approach still looks good to me, so this review mostly consists of the points we
already discussed verbally, captured as issues so that readers in the future have a chance
to reconstruct our thinking.


docs/configuration/master.md
Lines 434 (patched)
<https://reviews.apache.org/r/69307/#comment295700>

    Maximum number **of** simultaneous subscribers



docs/configuration/master.md
Lines 435 (patched)
<https://reviews.apache.org/r/69307/#comment295701>

    It is already implied here, but I would suggest to explicitly point out that older connections
will be terminated by the master when the maximum number is reached.



src/master/constants.hpp
Lines 96 (patched)
<https://reviews.apache.org/r/69307/#comment295703>

    While I don't have any experiments that suggest specific numbers, based on gut feeling
alone this still seems like at least an order of magnitude too low: The situation where we
have more active subscribers than this maximum is something we ideally never want to encounter
during normal operation of mesos, so it is probably good to have a generous safety margin.
    
    Also, I think there is some merit in the argument that we should try to avoid changing
the default behaviour, so maybe we should think about setting the default value to `-1`/`std::string::npos`.
(along with a comment in `boundedhashmap.hpp` that would elevate this from "working hack"
to "officially supported API" ;)



src/master/master.cpp
Line 12130 (original), 12130 (patched)
<https://reviews.apache.org/r/69307/#comment295702>

    We should probably check `subscribers.subscribed.size()` and print a warning if we're
about to terminate a connection.



src/tests/api_tests.cpp
Lines 3688 (patched)
<https://reviews.apache.org/r/69307/#comment295714>

    To be honest, I don't completely understand if this is creating some special "unclean
shutdown" condition for the third connection, or if it is just the simplest way to force removal
of the connection on the master side.
    
    In the former case, this might be better suited for a separate unit test.
    
    In the latter case, it seems like this some  missing API in Mesos (i.e. an `UNSUBSCRIBE`
call), since intuitively this is something that *should* be easy to do from the clients perspective.


- Benno Evers


On Nov. 26, 2018, 11:40 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69307/
> -----------------------------------------------------------
> 
> (Updated Nov. 26, 2018, 11:40 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9258
>     https://issues.apache.org/jira/browse/MESOS-9258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This adds a flag (--max_event_stream_subscribers) to the master which
> controls how many active subscribers on the Master's event stream will
> be allowed at any time.  The default is 25 subscribers.
> 
> 
> Diffs
> -----
> 
>   docs/configuration/master.md 575476728079d884fe86b1a3a56614647d1b572e 
>   src/master/constants.hpp 76ad0c3b1494cb97888af5545acc998b8fb15bf6 
>   src/master/flags.hpp ed2d76af2831d1b9718724936fd3e4850e99b332 
>   src/master/flags.cpp 2677738d19caa7d600e591aeb4fb37f0c2318d45 
>   src/master/master.hpp e77babf22126838c63cd05e483875c9beb3ac5ff 
>   src/master/master.cpp 1e326ec42a7f79a0835529a4655e7ec272f1cf40 
>   src/tests/api_tests.cpp fdd9f871f75617fc26a28679e2a1e41f506c6133 
> 
> 
> Diff: https://reviews.apache.org/r/69307/diff/1/
> 
> 
> Testing
> -------
> 
> ```
> make check
> 
> src/mesos-tests --gtest_filter="*MaxEventStreamSubscribers*" --gtest_repeat=-1 --gtest_break_on_failure
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


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