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 72019: Updated master::Call::SUBSCRIBE to be served in parallel.
Date Tue, 28 Jan 2020 17:43:16 GMT


> On Jan. 27, 2020, 7:13 p.m., Andrei Sekretenko wrote:
> > src/master/master.hpp
> > Lines 1308-1311 (original), 1327-1333 (patched)
> > <https://reviews.apache.org/r/72019/diff/2/?file=2209289#file2209289line1327>
> >
> >     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.
> 
> Benjamin Mahler wrote:
>     Certainly, you could have:
>     
>     `Option<PostProcessing>` with `Variant<Subscribe, ...>`
>     `PostProcessing` with `Option<Variant<Subscribe, ...>>`
>     `PostProcessing` with `Variant<Nothing, Subscribe, ...>`
>     
>     The first to me had seemed the clearest expression of what's happening: there may
be some post-processing, or there may not be. I didn't add a comment explaining this, seems
it's self-evident from the type (optional post-processing, if there is no post-processing
it is None). What would the comment help clarify here?
>     
>     We could look at it another way as you suggested: there is always post-processing,
but it might be a no-op.
>     
>     I'm not sure why this warranted opening an issue though reading over your comment,
was there something that surprised you about seeing optional post-processing as opposed to
required post-processing with the no-op case? I think it's ok as is and putting nothing in
the variant type doesn't seem clearly better to me, so I will leave this open and follow up
based on this thread.
> 
> Andrei Sekretenko wrote:
>     Well, if you are sure this improves readability by making clear that in most cases
post-processing is optional, then let's leave it like this for now. 
>     I don't have a strong opinion here (to me, "there are different kinds of post-processing,
one of them is no-op" is more transparent than "there might be postprocessing of one of several
kinds, or might be none at all", but maybe this is just me ;) 
>     
>     Another minor issue is that we have to CHECK against dereferencing empty Option (and
ensure that it is covered in tests), despite having a Variant (which makes this particular
kind of bug impossible). 
>     Given the frequency with which Option is used in our code, I can't say this is a
real issue here.
>     
>     If there are no reasons other than readability, then no comment is needed.
>     
>     In any case, this choice is local to the `ReadOnlyHandler` and can be reconsidered
in the future.

Ok, thank you for clarifying! I can go either way, and your point about the benefit of pattern
matching (too bad we don't have this more broadly in C++!) is a good one, certainly makes
Variant less error prone


- Benjamin


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


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