> On Jan. 27, 2020, 7:13 p.m., Andrei Sekretenko wrote:
> > src/master/master.hpp
> > Lines 1306 (patched)
> > <https://reviews.apache.org/r/72019/diff/2/?file=2209289#file2209289line1306>
> >
> > 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;)
Thanks for pointing this out!
> 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.
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.
- 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
>
>
|