> 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 > >
