mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@mesosphere.io>
Subject Re: Review Request 72019: Updated master::Call::SUBSCRIBE to be served in parallel.
Date Mon, 27 Jan 2020 19:13:27 GMT

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


Fix it, then Ship it!





src/master/master.hpp
Lines 1306 (patched)
<https://reviews.apache.org/r/72019/#comment307559>

    Hmm... to me, "caching of responses" sounds misleading here. 
    Yes, `ReadOnlyHandler` is caching - in a sense - but it is not caching what already exists,
it is *caching the future*.
    
    Given that you are touching this comment now, probably it is time to replace this phrase
with something more precise.
    I would suggest s/caching of responses/deduplicating requests/; if you know a better word,
feel free to use it;)



src/master/master.hpp
Lines 1308-1311 (original), 1327-1333 (patched)
<https://reviews.apache.org/r/72019/#comment307558>

    Makes me wonder whether it is possible, instead of using `<Option>` to simply add
`Nothing` to the variant, like
    
    ```
    struct PostProcessing {
      ...
      Variant<Nothing, Subscribe> state;
    };
    ...
    pair<Response, Master::ReadOnlyHandler::PostProcessing>
    Master::ReadOnlyHandler::frameworks(...) {
      ...
      return {OK(jsonify(frameworks), query.get("jsonp")), {Nothing()}};
    }
    ```
    
    If there are reasons to have this variant wrapped into `Option`, I think they deserve
a comment in the code.


- Andrei Sekretenko


On Jan. 21, 2020, 7:26 p.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72019/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2020, 7:26 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Greg Mann.
> 
> 
> Bugs: MESOS-9497
>     https://issues.apache.org/jira/browse/MESOS-9497
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This call is not entirely read-only, unlike the other GET_* v1 master
> calls, and therefore it warranted its own patch.
> 
> The approach used is to add a post-processing "write" step to the
> handler return type. The post-processing step gets executed
> synchronously. In order to deal with different potential post-
> processing steps, we use a Variant.
> 
> Note that SUBSCRIBE cannot asynchronously register the subscriber
> after the read-only state is served, because it will miss events
> in the interim!
> 
> 
> Diffs
> -----
> 
>   src/common/http.hpp 47a4d6a1ad4897155448a6ba64e789b15a78c7a2 
>   src/master/http.cpp 8a588635e688eb52cd7b8320426dc412e7b44e18 
>   src/master/master.hpp 3074918d677430b588c7765f5ed82f4e324eeff4 
>   src/master/readonly_handler.cpp fbe748d99c2520b520f56afa50dc0b9bd809778d 
>   src/tests/master_load_tests.cpp 6cee2488413b6a4f9a69092a8b06cf6eb79f360b 
> 
> 
> Diff: https://reviews.apache.org/r/72019/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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