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