Re: Review Request 72019: Updated master::Call::SUBSCRIBE to be served in parallel.

[Benjamin Mahler]

> On Jan. 27, 2020, 7:13 p.m., Andrei Sekretenko wrote:
> > src/master/master.hpp
> > Lines 1308-1311 (original), 1327-1333 (patched)
> > 
> >
> > Makes me wonder whether it is possible, instead of using `` to 
> > simply add `Nothing` to the variant, like
> > 
> > ```
> > struct PostProcessing {
> >   ...
> >   Variant state;
> > };
> > ...
> > pair
> > 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` with `Variant`
> `PostProcessing` with `Option>`
> `PostProcessing` with `Variant`
> 
> 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
> 
>

Re: Review Request 72019: Updated master::Call::SUBSCRIBE to be served in parallel.

[Andrei Sekretenko]

> On Jan. 27, 2020, 7:13 p.m., Andrei Sekretenko wrote:
> > src/master/master.hpp
> > Lines 1308-1311 (original), 1327-1333 (patched)
> > 
> >
> > Makes me wonder whether it is possible, instead of using `` to 
> > simply add `Nothing` to the variant, like
> > 
> > ```
> > struct PostProcessing {
> >   ...
> >   Variant state;
> > };
> > ...
> > pair
> > 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` with `Variant`
> `PostProcessing` with `Option>`
> `PostProcessing` with `Variant`
> 
> 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
> 
>

Re: Review Request 72019: Updated master::Call::SUBSCRIBE to be served in parallel.

[Benjamin Mahler]

> On Jan. 27, 2020, 7:13 p.m., Andrei Sekretenko wrote:
> > src/master/master.hpp
> > Lines 1306 (patched)
> > 
> >
> > 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)
> > 
> >
> > Makes me wonder whether it is possible, instead of using `` to 
> > simply add `Nothing` to the variant, like
> > 
> > ```
> > struct PostProcessing {
> >   ...
> >   Variant state;
> > };
> > ...
> > pair
> > 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` with `Variant`
`PostProcessing` with `Option>`
`PostProcessing` with `Variant`

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

Re: Review Request 72019: Updated master::Call::SUBSCRIBE to be served in parallel.

[Andrei Sekretenko]

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


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)


Makes me wonder whether it is possible, instead of using `` to 
simply add `Nothing` to the variant, like

```
struct PostProcessing {
  ...
  Variant state;
};
...
pair
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
> 
>

Re: Review Request 72019: Updated master::Call::SUBSCRIBE to be served in parallel.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [71906, 71907, 71908, 72019]

Failed command: ['bash', '-c', "set -o pipefail; export OS='ubuntu:14.04' 
BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose 
--disable-libtool-wrappers --disable-parallel-test-execution' 
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh 2>&1 | tee 
build_72019"]

Error:
..
1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 
-DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 
-DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 
-DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 
-DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 -DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 
-DMESOS_HAS_JAVA=1 -DENABLE_NVML=1 -DHAVE_LIBSASL2=1 -DHAVE_SVN_VERSION_H=1 
-DHAVE_LIBSVN_SUBR_1=1 -DHAVE_SVN_DELTA_H=1 -DHAVE_LIBSVN_DELTA_1=1 
-DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 -DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. 
-I../../../3rdparty/libprocess 
-DBUILD_DIR=\"/mesos/mesos-1.10.0/_build/3rdparty/libprocess\" 
-I../../../3rdparty/libprocess/include -I../../../3rdparty/libprocess/src 
-I../boost-1.65.0 -I../concurrentqueue-7b69a8f -I../elfio-3.2 
-I../glog-0.4.0/src -I../grpc-1.10.0/include -I../http-parser-2.6.2 
-I../libev-4.22 -D__STDC_FORMAT_MACROS -I../picojson-1.3.0 
-I../protobuf-3.5.0/src -I../rapidjson-1.1.0/include
  -I../../../3rdparty/libprocess/../stout/include -DLIBPROCESS_ALLOW_JEMALLOC 
-I/usr/include/subversion-1 -I/usr/include/apr-1 -I/usr/include/apr-1.0 -Wall 
-Wsign-compare -Wformat-security -fstack-protector -fPIC -g1 -O0 
-Wno-unused-local-typedefs -std=c++11 -c 
../../../3rdparty/libprocess/src/grpc.cpp  -fPIC -DPIC -o 
src/.libs/libprocess_la-grpc.o
libtool: compile:  g++ -DPACKAGE_NAME=\"mesos\" -DPACKAGE_TARNAME=\"mesos\" 
-DPACKAGE_VERSION=\"1.10.0\" "-DPACKAGE_STRING=\"mesos 1.10.0\"" 
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesos\" 
-DVERSION=\"1.10.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 
-DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 
-DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 
-DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 
-DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 -DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 
-DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 -DMESOS_HAS_JAVA=1 -DENABLE_NVML=1 
-DHAVE_LIBSASL2=1 -DHAVE_SVN_VERSION_H=1 -DHAVE_LIBSVN_SUBR_1=1 
-DHAVE_SVN_DELTA_H=1 -DHAVE_LIBSVN_DELTA_1=1 -DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 
-DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. -I../../../3rdparty/libprocess 
-DBUILD_DIR=\"/mesos/mesos-1.10.0/_build/3rdparty/libprocess\" 
-I../../../3rdparty/libprocess/include -I../../../3rdparty/libprocess/src 
 -I../boost-1.65.0 -I../concurrentqueue-7b69a8f -I../elfio-3.2 
-I../glog-0.4.0/src -I../grpc-1.10.0/include -I../http-parser-2.6.2 
-I../libev-4.22 -D__STDC_FORMAT_MACROS -I../picojson-1.3.0 
-I../protobuf-3.5.0/src -I../rapidjson-1.1.0/include 
-I../../../3rdparty/libprocess/../stout/include -DLIBPROCESS_ALLOW_JEMALLOC 
-I/usr/include/subversion-1 -I/usr/include/apr-1 -I/usr/include/apr-1.0 -Wall 
-Wsign-compare -Wformat-security -fstack-protector -fPIC -g1 -O0 
-Wno-unused-local-typedefs -std=c++11 -c 
../../../3rdparty/libprocess/src/gtest_constants.cpp  -fPIC -DPIC -o 
src/.libs/libprocess_la-gtest_constants.o
libtool: compile:  g++ -DPACKAGE_NAME=\"mesos\" -DPACKAGE_TARNAME=\"mesos\" 
-DPACKAGE_VERSION=\"1.10.0\" "-DPACKAGE_STRING=\"mesos 1.10.0\"" 
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesos\" 
-DVERSION=\"1.10.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 
-DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 
-DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 
-DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 
-DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 -DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 
-DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 -DMESOS_HAS_JAVA=1 -DENABLE_NVML=1 
-DHAVE_LIBSASL2=1 -DHAVE_SVN_VERSION_H=1 -DHAVE_LIBSVN_SUBR_1=1 
-DHAVE_SVN_DELTA_H=1 -DHAVE_LIBSVN_DELTA_1=1 -DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 
-DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. -I../../../3rdparty/libprocess 
-DBUILD_DIR=\"/mesos/mesos-1.10.0/_build/3rdparty/libprocess\" 
-I../../../3rdparty/libprocess/include -I../../../3rdparty/libprocess/src 
 -I../boost-1.65.0 -I../concurrentqueue-7b69a8f -I../elfio-3.2 
-I../glog-0.4.0/src -I../grpc-1.10.0/include -I../http-parser-2.6.2 
-I../libev-4.22 -D__STDC_FORMAT_MACROS -I../picojson-1.3.0 
-I../protobuf-3.5.0/src -I../rapidjson-1.1.0/include 
-I../../../3rdparty/libprocess/../stout/include -DLIBPROCESS_ALLOW_JEMALLOC 
-I/usr/include/subversion-1 -I/usr/include/apr-1 

Re: Review Request 72019: Updated master::Call::SUBSCRIBE to be served in parallel.

[Benjamin Mahler]

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


Changes
---

- Went back to the original approach after andrei pushed for it. Figured out 
how to make a post-processing step work using a variant instead of templatizing 
the return types.


Bugs: MESOS-9497
https://issues.apache.org/jira/browse/MESOS-9497


Repository: mesos


Description (updated)
---

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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/72019/diff/1-2/


Testing
---


Thanks,

Benjamin Mahler

Re: Review Request 72019: Updated master::Call::SUBSCRIBE to be served in parallel.

[Andrei Sekretenko]

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




src/master/master.hpp
Lines 1941-1943 (original), 1959-1961 (patched)


Shouldn't this comment be amended now? Seems to contradict the way 
`subscribe()` works.



src/master/master.hpp
Lines 1969-1971 (patched)


Having two public mutable members isn't something that makes reasonoing 
about the code simpler:)
 
Also, having this stuff specific for subscribe will complicate things if we 
ever need to add a second readonly request handler with a non-readonly 
postprocessing.

I'm wondering how difficult it would be to extend the return of 
`ReadOnlyRequestHandler` with a postprocessing callback that would be 
guaranteed to be executed immediately after the batch in the `Master` 
context... 
Might be not worth it right now; in this case, it probably should be added 
as TODO.


- Andrei Sekretenko


On Jan. 16, 2020, 10:22 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72019/
> ---
> 
> (Updated Jan. 16, 2020, 10:22 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 have a mutex protected list of new subscribers
> that gets written to during the parallel serving. Afterwards, once
> things are serial again on the Master, we add the new subscribers to
> the masters data structures.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 47a4d6a1ad4897155448a6ba64e789b15a78c7a2 
>   src/master/http.cpp 8a588635e688eb52cd7b8320426dc412e7b44e18 
>   src/master/master.hpp 3074918d677430b588c7765f5ed82f4e324eeff4 
>   src/master/readonly_handler.cpp fbe748d99c2520b520f56afa50dc0b9bd809778d 
> 
> 
> Diff: https://reviews.apache.org/r/72019/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 72019: Updated master::Call::SUBSCRIBE to be served in parallel.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [71906, 71907, 71908, 72019]

Failed command: ['bash', '-c', "set -o pipefail; export OS='ubuntu:14.04' 
BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose 
--disable-libtool-wrappers --disable-parallel-test-execution' 
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh 2>&1 | tee 
build_72019"]

Error:
..
_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 
-DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 
-DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 
-DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 
-DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 -DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 
-DMESOS_HAS_JAVA=1 -DENABLE_NVML=1 -DHAVE_LIBSASL2=1 -DHAVE_SVN_VERSION_H=1 
-DHAVE_LIBSVN_SUBR_1=1 -DHAVE_SVN_DELTA_H=1 -DHAVE_LIBSVN_DELTA_1=1 
-DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 -DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. 
-I../../../3rdparty/libprocess 
-DBUILD_DIR=\"/mesos/mesos-1.10.0/_build/3rdparty/libprocess\" 
-I../../../3rdparty/libprocess/include -I../../../3rdparty/libprocess/src 
-I../boost-1.65.0 -I../concurrentqueue-7b69a8f -I../elfio-3.2 
-I../glog-0.4.0/src -I../grpc-1.10.0/include -I../http-parser-2.6.2 
-I../libev-4.22 -D__STDC_FORMAT_MACROS -I../picojson-1.3.0 
-I../protobuf-3.5.0/src -I../rapidjson-1.1.0/include -I../../../
 3rdparty/libprocess/../stout/include -DLIBPROCESS_ALLOW_JEMALLOC 
-I/usr/include/subversion-1 -I/usr/include/apr-1 -I/usr/include/apr-1.0 -Wall 
-Wsign-compare -Wformat-security -fstack-protector -fPIC -g1 -O0 
-Wno-unused-local-typedefs -std=c++11 -c 
../../../3rdparty/libprocess/src/clock.cpp  -fPIC -DPIC -o 
src/.libs/libprocess_la-clock.o
libtool: compile:  g++ -DPACKAGE_NAME=\"mesos\" -DPACKAGE_TARNAME=\"mesos\" 
-DPACKAGE_VERSION=\"1.10.0\" "-DPACKAGE_STRING=\"mesos 1.10.0\"" 
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesos\" 
-DVERSION=\"1.10.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 
-DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 
-DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 
-DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 
-DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 -DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 
-DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 -DMESOS_HAS_JAVA=1 -DENABLE_NVML=1 
-DHAVE_LIBSASL2=1 -DHAVE_SVN_VERSION_H=1 -DHAVE_LIBSVN_SUBR_1=1 
-DHAVE_SVN_DELTA_H=1 -DHAVE_LIBSVN_DELTA_1=1 -DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 
-DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. -I../../../3rdparty/libprocess 
-DBUILD_DIR=\"/mesos/mesos-1.10.0/_build/3rdparty/libprocess\" 
-I../../../3rdparty/libprocess/include -I../../../3rdparty/libprocess/src 
 -I../boost-1.65.0 -I../concurrentqueue-7b69a8f -I../elfio-3.2 
-I../glog-0.4.0/src -I../grpc-1.10.0/include -I../http-parser-2.6.2 
-I../libev-4.22 -D__STDC_FORMAT_MACROS -I../picojson-1.3.0 
-I../protobuf-3.5.0/src -I../rapidjson-1.1.0/include 
-I../../../3rdparty/libprocess/../stout/include -DLIBPROCESS_ALLOW_JEMALLOC 
-I/usr/include/subversion-1 -I/usr/include/apr-1 -I/usr/include/apr-1.0 -Wall 
-Wsign-compare -Wformat-security -fstack-protector -fPIC -g1 -O0 
-Wno-unused-local-typedefs -std=c++11 -c 
../../../3rdparty/libprocess/src/firewall.cpp  -fPIC -DPIC -o 
src/.libs/libprocess_la-firewall.o
libtool: compile:  g++ -DPACKAGE_NAME=\"mesos\" -DPACKAGE_TARNAME=\"mesos\" 
-DPACKAGE_VERSION=\"1.10.0\" "-DPACKAGE_STRING=\"mesos 1.10.0\"" 
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesos\" 
-DVERSION=\"1.10.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 
-DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 
-DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 
-DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 
-DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 -DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 
-DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 -DMESOS_HAS_JAVA=1 -DENABLE_NVML=1 
-DHAVE_LIBSASL2=1 -DHAVE_SVN_VERSION_H=1 -DHAVE_LIBSVN_SUBR_1=1 
-DHAVE_SVN_DELTA_H=1 -DHAVE_LIBSVN_DELTA_1=1 -DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 
-DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. -I../../../3rdparty/libprocess 
-DBUILD_DIR=\"/mesos/mesos-1.10.0/_build/3rdparty/libprocess\" 
-I../../../3rdparty/libprocess/include -I../../../3rdparty/libprocess/src 
 -I../boost-1.65.0 -I../concurrentqueue-7b69a8f -I../elfio-3.2 
-I../glog-0.4.0/src -I../grpc-1.10.0/include -I../http-parser-2.6.2 
-I../libev-4.22 -D__STDC_FORMAT_MACROS -I../picojson-1.3.0 
-I../protobuf-3.5.0/src -I../rapidjson-1.1.0/include 
-I../../../3rdparty/libprocess/../stout/include -DLIBPROCESS_ALLOW_JEMALLOC 
-I/usr/include/subversion-1 -I/usr/include/apr-1 -I/usr/include/apr-1.0 -Wall 

Re: Review Request 72019: Updated master::Call::SUBSCRIBE to be served in parallel.

[Benjamin Mahler]

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




src/master/master.hpp
Lines 1388-1392 (original), 1395-1399 (patched)


Remove this TODO and make these private. Turns out they need to remain 
members to get private access to the master's private members.


- Benjamin Mahler


On Jan. 16, 2020, 10:22 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72019/
> ---
> 
> (Updated Jan. 16, 2020, 10:22 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 have a mutex protected list of new subscribers
> that gets written to during the parallel serving. Afterwards, once
> things are serial again on the Master, we add the new subscribers to
> the masters data structures.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 47a4d6a1ad4897155448a6ba64e789b15a78c7a2 
>   src/master/http.cpp 8a588635e688eb52cd7b8320426dc412e7b44e18 
>   src/master/master.hpp 3074918d677430b588c7765f5ed82f4e324eeff4 
>   src/master/readonly_handler.cpp fbe748d99c2520b520f56afa50dc0b9bd809778d 
> 
> 
> Diff: https://reviews.apache.org/r/72019/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>