You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Cliff Jansen <cl...@gmail.com> on 2015/10/27 19:21:14 UTC
Review Request 39694: C++ binding connection options and reconnect
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39694/
-----------------------------------------------------------
Review request for qpid, Alan Conway, Andrew Stitcher, and Justin Ross.
Bugs: PROTON-865
https://issues.apache.org/jira/browse/PROTON-865
Repository: qpid-proton-git
Description
-------
This provides connection options (broadly similar to link options).
As suggested by Alan, the collection connection_options is a subclass of the base connection_option so that they can be grouped and nested.
They can also be copied and persisted via respective clone methods. Reconnect and transport options need to live as long as the connection. The handler option is special and needs to be extracted before other options to be available when the connection is created.
Incoming (listener) connection options are perhaps missing a feature. You can set global server connection options on a container, and you can set separate ssl_domain creds per listener, but you cannot set other connection options (idle_timeout/max_frame_size etc) per listener. This is reflected in Proton-C's acceptor and the readable events that it intercepts. There is no way to query the C API to find out which listener is the parent of an incoming connection or transfer other listener-specific context to the connection.
As suggested in the link options review, I attempted to provide an initializer_list<foo> constructor and append, but that requires array elements of identical size. The varying size of the connection option types is unhelpful here. Maybe they should be counted_ptr based pimpl structs of identical size.
This patch can also be viewed as the "reconnect" branch of https://github.com/cliffjansen/qpid-proton.
Diffs
-----
examples/cpp/CMakeLists.txt 34edb83
examples/cpp/connection_options.cpp PRE-CREATION
proton-c/bindings/cpp/CMakeLists.txt 9f423fb
proton-c/bindings/cpp/include/proton/connection_options.hpp PRE-CREATION
proton-c/bindings/cpp/include/proton/container.hpp 059416f
proton-c/bindings/cpp/include/proton/reconnect_timer.hpp PRE-CREATION
proton-c/bindings/cpp/include/proton/transport.hpp 3d602b7
proton-c/bindings/cpp/src/blocking_connection_impl.cpp 8abd24b
proton-c/bindings/cpp/src/connection.cpp 1b35e03
proton-c/bindings/cpp/src/connection_options.cpp PRE-CREATION
proton-c/bindings/cpp/src/connector.hpp b311cc9
proton-c/bindings/cpp/src/connector.cpp 49660ea
proton-c/bindings/cpp/src/container.cpp 13f12d9
proton-c/bindings/cpp/src/container_impl.hpp ec356e2
proton-c/bindings/cpp/src/container_impl.cpp fc97a3e
proton-c/bindings/cpp/src/reconnect_timer.cpp PRE-CREATION
proton-c/bindings/cpp/src/transport.cpp 58114ae
Diff: https://reviews.apache.org/r/39694/diff/
Testing
-------
Linux only
Thanks,
Cliff Jansen
Re: Review Request 39694: C++ binding connection options and reconnect
Posted by Andrew Stitcher <as...@apache.org>.
> On Oct. 30, 2015, 1:19 a.m., Alan Conway wrote:
> > I have what I think is a simpler implementation of connection options, I can't update the reviewboard look at:
> > https://github.com/alanconway/qpid-proton/commit/7eb1e87c1fdc8a72422b36770acbe0d3fa2deb1c
> >
> > The logic is Cliff's (if I didn't miss anything) but the impl is shorter and I think more readable. If we like this we could do the same for link options.
> >
> > NO-JIRA: Simpler implementation of connection options.
> >
> > This is a simpler implementation of connection_options
> >
> > - More compact, option tests and apply logic together in one apply() function.
> > - No limit on number of options that can be chained in call to connect()
> > - Simple value semantics. No need for virtual clone(), inheritance, pointers.
> > - Fewer heap allocations - one per option set, not per individual option.
> >
> > Options class is pimpl with no virtuals so no binary compat issues adding new options.
> >
> > /** Options for creating a connection.
> > *
> > * Options can be "chained" like this:
> > *
> > * c = container.connect(url, connection_options().handler(h).max_frame_size(1234));
> > *
> > * You can also create an options object with common settings and use it as a base
> > * for different connections that have mostly the same settings:
> > *
> > * connection_options opts;
> > * opts.idle_timeout(1000).max_frame_size(10000);
> > * c1 = container.connect(url1, opts.handler(h1));
> > * c2 = container.connect(url2, opts.handler(h2));
> > *
> > * Normal value semantics, copy or assign creates a separate copy of the options.
> > */
>
> Andrew Stitcher wrote:
> I think I prefer Alan's implementation.
>
> I note that using variadic templates (in C++11 and on) you can get a perhaps maore natural connection constructor allowing multiple options - Either as a constructor for connection_options or at the end of the connection constructor. So we should definately allow that in a modern C++ compiler, but we can add that later.
>
> That would look like:
>
> Either
> c = container.connect(url, handler(h), max_frame_size(1234));
> Or
> c = container.connect(url, connection_options(handler(h).max_frame_size(1234)));
>
> Andrew Stitcher wrote:
> Oops - for that last I meant:
> c = container.connect(url, connection_options(handler(h), max_frame_size(1234)));
>
> Alan Conway wrote:
> I'm assuming your Oops was meant to say:
>
> c = container.connect(url, connection_options().handler(h).max_frame_size(1234));
>
> Also one issue with the varadic approach: presumably your option names would have to be in the global scope, so "handler" wouldn't work - it works in my approach because it is a member of connection_options. One way to deal with that would be to put the varadic versions in a namespace:
>
> namespace copts {
> // define handler, max_frame_size etc.
> }
>
> // Now you have a choice:
> c = container.connect(url, copts::handler(h), copts::max_frame_size(1234))
> // OR
> using namespace copts;
> c = container.connect(url, handler(h), max_frame_size(1234))
>
> That's how we managed the naming of boost optional arguments waaay back in the old days of the qpid::client.
No the correction was indeed correct - it uses a variadic constructor for connection_options instead of for connection - your correction is exactly what was already there!
Your point about names is a good one, I think namespacing works.
Actually going in this direction does take you quite far towards named parameters (if you want to take that journey!)
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39694/#review104505
-----------------------------------------------------------
On Oct. 27, 2015, 6:21 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39694/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2015, 6:21 p.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, and Justin Ross.
>
>
> Bugs: PROTON-865
> https://issues.apache.org/jira/browse/PROTON-865
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> This provides connection options (broadly similar to link options).
>
> As suggested by Alan, the collection connection_options is a subclass of the base connection_option so that they can be grouped and nested.
>
> They can also be copied and persisted via respective clone methods. Reconnect and transport options need to live as long as the connection. The handler option is special and needs to be extracted before other options to be available when the connection is created.
>
> Incoming (listener) connection options are perhaps missing a feature. You can set global server connection options on a container, and you can set separate ssl_domain creds per listener, but you cannot set other connection options (idle_timeout/max_frame_size etc) per listener. This is reflected in Proton-C's acceptor and the readable events that it intercepts. There is no way to query the C API to find out which listener is the parent of an incoming connection or transfer other listener-specific context to the connection.
>
> As suggested in the link options review, I attempted to provide an initializer_list<foo> constructor and append, but that requires array elements of identical size. The varying size of the connection option types is unhelpful here. Maybe they should be counted_ptr based pimpl structs of identical size.
>
> This patch can also be viewed as the "reconnect" branch of https://github.com/cliffjansen/qpid-proton.
>
>
> Diffs
> -----
>
> examples/cpp/CMakeLists.txt 34edb83
> examples/cpp/connection_options.cpp PRE-CREATION
> proton-c/bindings/cpp/CMakeLists.txt 9f423fb
> proton-c/bindings/cpp/include/proton/connection_options.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/container.hpp 059416f
> proton-c/bindings/cpp/include/proton/reconnect_timer.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/transport.hpp 3d602b7
> proton-c/bindings/cpp/src/blocking_connection_impl.cpp 8abd24b
> proton-c/bindings/cpp/src/connection.cpp 1b35e03
> proton-c/bindings/cpp/src/connection_options.cpp PRE-CREATION
> proton-c/bindings/cpp/src/connector.hpp b311cc9
> proton-c/bindings/cpp/src/connector.cpp 49660ea
> proton-c/bindings/cpp/src/container.cpp 13f12d9
> proton-c/bindings/cpp/src/container_impl.hpp ec356e2
> proton-c/bindings/cpp/src/container_impl.cpp fc97a3e
> proton-c/bindings/cpp/src/reconnect_timer.cpp PRE-CREATION
> proton-c/bindings/cpp/src/transport.cpp 58114ae
>
> Diff: https://reviews.apache.org/r/39694/diff/
>
>
> Testing
> -------
>
> Linux only
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 39694: C++ binding connection options and reconnect
Posted by Andrew Stitcher <as...@apache.org>.
> On Oct. 30, 2015, 1:19 a.m., Alan Conway wrote:
> > I have what I think is a simpler implementation of connection options, I can't update the reviewboard look at:
> > https://github.com/alanconway/qpid-proton/commit/7eb1e87c1fdc8a72422b36770acbe0d3fa2deb1c
> >
> > The logic is Cliff's (if I didn't miss anything) but the impl is shorter and I think more readable. If we like this we could do the same for link options.
> >
> > NO-JIRA: Simpler implementation of connection options.
> >
> > This is a simpler implementation of connection_options
> >
> > - More compact, option tests and apply logic together in one apply() function.
> > - No limit on number of options that can be chained in call to connect()
> > - Simple value semantics. No need for virtual clone(), inheritance, pointers.
> > - Fewer heap allocations - one per option set, not per individual option.
> >
> > Options class is pimpl with no virtuals so no binary compat issues adding new options.
> >
> > /** Options for creating a connection.
> > *
> > * Options can be "chained" like this:
> > *
> > * c = container.connect(url, connection_options().handler(h).max_frame_size(1234));
> > *
> > * You can also create an options object with common settings and use it as a base
> > * for different connections that have mostly the same settings:
> > *
> > * connection_options opts;
> > * opts.idle_timeout(1000).max_frame_size(10000);
> > * c1 = container.connect(url1, opts.handler(h1));
> > * c2 = container.connect(url2, opts.handler(h2));
> > *
> > * Normal value semantics, copy or assign creates a separate copy of the options.
> > */
I think I prefer Alan's implementation.
I note that using variadic templates (in C++11 and on) you can get a perhaps maore natural connection constructor allowing multiple options - Either as a constructor for connection_options or at the end of the connection constructor. So we should definately allow that in a modern C++ compiler, but we can add that later.
That would look like:
Either
c = container.connect(url, handler(h), max_frame_size(1234));
Or
c = container.connect(url, connection_options(handler(h).max_frame_size(1234)));
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39694/#review104505
-----------------------------------------------------------
On Oct. 27, 2015, 6:21 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39694/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2015, 6:21 p.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, and Justin Ross.
>
>
> Bugs: PROTON-865
> https://issues.apache.org/jira/browse/PROTON-865
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> This provides connection options (broadly similar to link options).
>
> As suggested by Alan, the collection connection_options is a subclass of the base connection_option so that they can be grouped and nested.
>
> They can also be copied and persisted via respective clone methods. Reconnect and transport options need to live as long as the connection. The handler option is special and needs to be extracted before other options to be available when the connection is created.
>
> Incoming (listener) connection options are perhaps missing a feature. You can set global server connection options on a container, and you can set separate ssl_domain creds per listener, but you cannot set other connection options (idle_timeout/max_frame_size etc) per listener. This is reflected in Proton-C's acceptor and the readable events that it intercepts. There is no way to query the C API to find out which listener is the parent of an incoming connection or transfer other listener-specific context to the connection.
>
> As suggested in the link options review, I attempted to provide an initializer_list<foo> constructor and append, but that requires array elements of identical size. The varying size of the connection option types is unhelpful here. Maybe they should be counted_ptr based pimpl structs of identical size.
>
> This patch can also be viewed as the "reconnect" branch of https://github.com/cliffjansen/qpid-proton.
>
>
> Diffs
> -----
>
> examples/cpp/CMakeLists.txt 34edb83
> examples/cpp/connection_options.cpp PRE-CREATION
> proton-c/bindings/cpp/CMakeLists.txt 9f423fb
> proton-c/bindings/cpp/include/proton/connection_options.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/container.hpp 059416f
> proton-c/bindings/cpp/include/proton/reconnect_timer.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/transport.hpp 3d602b7
> proton-c/bindings/cpp/src/blocking_connection_impl.cpp 8abd24b
> proton-c/bindings/cpp/src/connection.cpp 1b35e03
> proton-c/bindings/cpp/src/connection_options.cpp PRE-CREATION
> proton-c/bindings/cpp/src/connector.hpp b311cc9
> proton-c/bindings/cpp/src/connector.cpp 49660ea
> proton-c/bindings/cpp/src/container.cpp 13f12d9
> proton-c/bindings/cpp/src/container_impl.hpp ec356e2
> proton-c/bindings/cpp/src/container_impl.cpp fc97a3e
> proton-c/bindings/cpp/src/reconnect_timer.cpp PRE-CREATION
> proton-c/bindings/cpp/src/transport.cpp 58114ae
>
> Diff: https://reviews.apache.org/r/39694/diff/
>
>
> Testing
> -------
>
> Linux only
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 39694: C++ binding connection options and reconnect
Posted by Andrew Stitcher <as...@apache.org>.
> On Oct. 30, 2015, 1:19 a.m., Alan Conway wrote:
> > I have what I think is a simpler implementation of connection options, I can't update the reviewboard look at:
> > https://github.com/alanconway/qpid-proton/commit/7eb1e87c1fdc8a72422b36770acbe0d3fa2deb1c
> >
> > The logic is Cliff's (if I didn't miss anything) but the impl is shorter and I think more readable. If we like this we could do the same for link options.
> >
> > NO-JIRA: Simpler implementation of connection options.
> >
> > This is a simpler implementation of connection_options
> >
> > - More compact, option tests and apply logic together in one apply() function.
> > - No limit on number of options that can be chained in call to connect()
> > - Simple value semantics. No need for virtual clone(), inheritance, pointers.
> > - Fewer heap allocations - one per option set, not per individual option.
> >
> > Options class is pimpl with no virtuals so no binary compat issues adding new options.
> >
> > /** Options for creating a connection.
> > *
> > * Options can be "chained" like this:
> > *
> > * c = container.connect(url, connection_options().handler(h).max_frame_size(1234));
> > *
> > * You can also create an options object with common settings and use it as a base
> > * for different connections that have mostly the same settings:
> > *
> > * connection_options opts;
> > * opts.idle_timeout(1000).max_frame_size(10000);
> > * c1 = container.connect(url1, opts.handler(h1));
> > * c2 = container.connect(url2, opts.handler(h2));
> > *
> > * Normal value semantics, copy or assign creates a separate copy of the options.
> > */
>
> Andrew Stitcher wrote:
> I think I prefer Alan's implementation.
>
> I note that using variadic templates (in C++11 and on) you can get a perhaps maore natural connection constructor allowing multiple options - Either as a constructor for connection_options or at the end of the connection constructor. So we should definately allow that in a modern C++ compiler, but we can add that later.
>
> That would look like:
>
> Either
> c = container.connect(url, handler(h), max_frame_size(1234));
> Or
> c = container.connect(url, connection_options(handler(h).max_frame_size(1234)));
Oops - for that last I meant:
c = container.connect(url, connection_options(handler(h), max_frame_size(1234)));
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39694/#review104505
-----------------------------------------------------------
On Oct. 27, 2015, 6:21 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39694/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2015, 6:21 p.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, and Justin Ross.
>
>
> Bugs: PROTON-865
> https://issues.apache.org/jira/browse/PROTON-865
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> This provides connection options (broadly similar to link options).
>
> As suggested by Alan, the collection connection_options is a subclass of the base connection_option so that they can be grouped and nested.
>
> They can also be copied and persisted via respective clone methods. Reconnect and transport options need to live as long as the connection. The handler option is special and needs to be extracted before other options to be available when the connection is created.
>
> Incoming (listener) connection options are perhaps missing a feature. You can set global server connection options on a container, and you can set separate ssl_domain creds per listener, but you cannot set other connection options (idle_timeout/max_frame_size etc) per listener. This is reflected in Proton-C's acceptor and the readable events that it intercepts. There is no way to query the C API to find out which listener is the parent of an incoming connection or transfer other listener-specific context to the connection.
>
> As suggested in the link options review, I attempted to provide an initializer_list<foo> constructor and append, but that requires array elements of identical size. The varying size of the connection option types is unhelpful here. Maybe they should be counted_ptr based pimpl structs of identical size.
>
> This patch can also be viewed as the "reconnect" branch of https://github.com/cliffjansen/qpid-proton.
>
>
> Diffs
> -----
>
> examples/cpp/CMakeLists.txt 34edb83
> examples/cpp/connection_options.cpp PRE-CREATION
> proton-c/bindings/cpp/CMakeLists.txt 9f423fb
> proton-c/bindings/cpp/include/proton/connection_options.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/container.hpp 059416f
> proton-c/bindings/cpp/include/proton/reconnect_timer.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/transport.hpp 3d602b7
> proton-c/bindings/cpp/src/blocking_connection_impl.cpp 8abd24b
> proton-c/bindings/cpp/src/connection.cpp 1b35e03
> proton-c/bindings/cpp/src/connection_options.cpp PRE-CREATION
> proton-c/bindings/cpp/src/connector.hpp b311cc9
> proton-c/bindings/cpp/src/connector.cpp 49660ea
> proton-c/bindings/cpp/src/container.cpp 13f12d9
> proton-c/bindings/cpp/src/container_impl.hpp ec356e2
> proton-c/bindings/cpp/src/container_impl.cpp fc97a3e
> proton-c/bindings/cpp/src/reconnect_timer.cpp PRE-CREATION
> proton-c/bindings/cpp/src/transport.cpp 58114ae
>
> Diff: https://reviews.apache.org/r/39694/diff/
>
>
> Testing
> -------
>
> Linux only
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 39694: C++ binding connection options and reconnect
Posted by Alan Conway <ac...@redhat.com>.
> On Oct. 30, 2015, 1:19 a.m., Alan Conway wrote:
> > I have what I think is a simpler implementation of connection options, I can't update the reviewboard look at:
> > https://github.com/alanconway/qpid-proton/commit/7eb1e87c1fdc8a72422b36770acbe0d3fa2deb1c
> >
> > The logic is Cliff's (if I didn't miss anything) but the impl is shorter and I think more readable. If we like this we could do the same for link options.
> >
> > NO-JIRA: Simpler implementation of connection options.
> >
> > This is a simpler implementation of connection_options
> >
> > - More compact, option tests and apply logic together in one apply() function.
> > - No limit on number of options that can be chained in call to connect()
> > - Simple value semantics. No need for virtual clone(), inheritance, pointers.
> > - Fewer heap allocations - one per option set, not per individual option.
> >
> > Options class is pimpl with no virtuals so no binary compat issues adding new options.
> >
> > /** Options for creating a connection.
> > *
> > * Options can be "chained" like this:
> > *
> > * c = container.connect(url, connection_options().handler(h).max_frame_size(1234));
> > *
> > * You can also create an options object with common settings and use it as a base
> > * for different connections that have mostly the same settings:
> > *
> > * connection_options opts;
> > * opts.idle_timeout(1000).max_frame_size(10000);
> > * c1 = container.connect(url1, opts.handler(h1));
> > * c2 = container.connect(url2, opts.handler(h2));
> > *
> > * Normal value semantics, copy or assign creates a separate copy of the options.
> > */
>
> Andrew Stitcher wrote:
> I think I prefer Alan's implementation.
>
> I note that using variadic templates (in C++11 and on) you can get a perhaps maore natural connection constructor allowing multiple options - Either as a constructor for connection_options or at the end of the connection constructor. So we should definately allow that in a modern C++ compiler, but we can add that later.
>
> That would look like:
>
> Either
> c = container.connect(url, handler(h), max_frame_size(1234));
> Or
> c = container.connect(url, connection_options(handler(h).max_frame_size(1234)));
>
> Andrew Stitcher wrote:
> Oops - for that last I meant:
> c = container.connect(url, connection_options(handler(h), max_frame_size(1234)));
I'm assuming your Oops was meant to say:
c = container.connect(url, connection_options().handler(h).max_frame_size(1234));
Also one issue with the varadic approach: presumably your option names would have to be in the global scope, so "handler" wouldn't work - it works in my approach because it is a member of connection_options. One way to deal with that would be to put the varadic versions in a namespace:
namespace copts {
// define handler, max_frame_size etc.
}
// Now you have a choice:
c = container.connect(url, copts::handler(h), copts::max_frame_size(1234))
// OR
using namespace copts;
c = container.connect(url, handler(h), max_frame_size(1234))
That's how we managed the naming of boost optional arguments waaay back in the old days of the qpid::client.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39694/#review104505
-----------------------------------------------------------
On Oct. 27, 2015, 6:21 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39694/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2015, 6:21 p.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, and Justin Ross.
>
>
> Bugs: PROTON-865
> https://issues.apache.org/jira/browse/PROTON-865
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> This provides connection options (broadly similar to link options).
>
> As suggested by Alan, the collection connection_options is a subclass of the base connection_option so that they can be grouped and nested.
>
> They can also be copied and persisted via respective clone methods. Reconnect and transport options need to live as long as the connection. The handler option is special and needs to be extracted before other options to be available when the connection is created.
>
> Incoming (listener) connection options are perhaps missing a feature. You can set global server connection options on a container, and you can set separate ssl_domain creds per listener, but you cannot set other connection options (idle_timeout/max_frame_size etc) per listener. This is reflected in Proton-C's acceptor and the readable events that it intercepts. There is no way to query the C API to find out which listener is the parent of an incoming connection or transfer other listener-specific context to the connection.
>
> As suggested in the link options review, I attempted to provide an initializer_list<foo> constructor and append, but that requires array elements of identical size. The varying size of the connection option types is unhelpful here. Maybe they should be counted_ptr based pimpl structs of identical size.
>
> This patch can also be viewed as the "reconnect" branch of https://github.com/cliffjansen/qpid-proton.
>
>
> Diffs
> -----
>
> examples/cpp/CMakeLists.txt 34edb83
> examples/cpp/connection_options.cpp PRE-CREATION
> proton-c/bindings/cpp/CMakeLists.txt 9f423fb
> proton-c/bindings/cpp/include/proton/connection_options.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/container.hpp 059416f
> proton-c/bindings/cpp/include/proton/reconnect_timer.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/transport.hpp 3d602b7
> proton-c/bindings/cpp/src/blocking_connection_impl.cpp 8abd24b
> proton-c/bindings/cpp/src/connection.cpp 1b35e03
> proton-c/bindings/cpp/src/connection_options.cpp PRE-CREATION
> proton-c/bindings/cpp/src/connector.hpp b311cc9
> proton-c/bindings/cpp/src/connector.cpp 49660ea
> proton-c/bindings/cpp/src/container.cpp 13f12d9
> proton-c/bindings/cpp/src/container_impl.hpp ec356e2
> proton-c/bindings/cpp/src/container_impl.cpp fc97a3e
> proton-c/bindings/cpp/src/reconnect_timer.cpp PRE-CREATION
> proton-c/bindings/cpp/src/transport.cpp 58114ae
>
> Diff: https://reviews.apache.org/r/39694/diff/
>
>
> Testing
> -------
>
> Linux only
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 39694: C++ binding connection options and reconnect
Posted by Alan Conway <ac...@redhat.com>.
> On Oct. 30, 2015, 1:19 a.m., Alan Conway wrote:
> > I have what I think is a simpler implementation of connection options, I can't update the reviewboard look at:
> > https://github.com/alanconway/qpid-proton/commit/7eb1e87c1fdc8a72422b36770acbe0d3fa2deb1c
> >
> > The logic is Cliff's (if I didn't miss anything) but the impl is shorter and I think more readable. If we like this we could do the same for link options.
> >
> > NO-JIRA: Simpler implementation of connection options.
> >
> > This is a simpler implementation of connection_options
> >
> > - More compact, option tests and apply logic together in one apply() function.
> > - No limit on number of options that can be chained in call to connect()
> > - Simple value semantics. No need for virtual clone(), inheritance, pointers.
> > - Fewer heap allocations - one per option set, not per individual option.
> >
> > Options class is pimpl with no virtuals so no binary compat issues adding new options.
> >
> > /** Options for creating a connection.
> > *
> > * Options can be "chained" like this:
> > *
> > * c = container.connect(url, connection_options().handler(h).max_frame_size(1234));
> > *
> > * You can also create an options object with common settings and use it as a base
> > * for different connections that have mostly the same settings:
> > *
> > * connection_options opts;
> > * opts.idle_timeout(1000).max_frame_size(10000);
> > * c1 = container.connect(url1, opts.handler(h1));
> > * c2 = container.connect(url2, opts.handler(h2));
> > *
> > * Normal value semantics, copy or assign creates a separate copy of the options.
> > */
>
> Andrew Stitcher wrote:
> I think I prefer Alan's implementation.
>
> I note that using variadic templates (in C++11 and on) you can get a perhaps maore natural connection constructor allowing multiple options - Either as a constructor for connection_options or at the end of the connection constructor. So we should definately allow that in a modern C++ compiler, but we can add that later.
>
> That would look like:
>
> Either
> c = container.connect(url, handler(h), max_frame_size(1234));
> Or
> c = container.connect(url, connection_options(handler(h).max_frame_size(1234)));
>
> Andrew Stitcher wrote:
> Oops - for that last I meant:
> c = container.connect(url, connection_options(handler(h), max_frame_size(1234)));
>
> Alan Conway wrote:
> I'm assuming your Oops was meant to say:
>
> c = container.connect(url, connection_options().handler(h).max_frame_size(1234));
>
> Also one issue with the varadic approach: presumably your option names would have to be in the global scope, so "handler" wouldn't work - it works in my approach because it is a member of connection_options. One way to deal with that would be to put the varadic versions in a namespace:
>
> namespace copts {
> // define handler, max_frame_size etc.
> }
>
> // Now you have a choice:
> c = container.connect(url, copts::handler(h), copts::max_frame_size(1234))
> // OR
> using namespace copts;
> c = container.connect(url, handler(h), max_frame_size(1234))
>
> That's how we managed the naming of boost optional arguments waaay back in the old days of the qpid::client.
>
> Andrew Stitcher wrote:
> No the correction was indeed correct - it uses a variadic constructor for connection_options instead of for connection - your correction is exactly what was already there!
>
> Your point about names is a good one, I think namespacing works.
>
> Actually going in this direction does take you quite far towards named parameters (if you want to take that journey!)
Gotcha. Dots and commas, gotta love a language made almost entirely of punctuation marks.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39694/#review104505
-----------------------------------------------------------
On Oct. 27, 2015, 6:21 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39694/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2015, 6:21 p.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, and Justin Ross.
>
>
> Bugs: PROTON-865
> https://issues.apache.org/jira/browse/PROTON-865
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> This provides connection options (broadly similar to link options).
>
> As suggested by Alan, the collection connection_options is a subclass of the base connection_option so that they can be grouped and nested.
>
> They can also be copied and persisted via respective clone methods. Reconnect and transport options need to live as long as the connection. The handler option is special and needs to be extracted before other options to be available when the connection is created.
>
> Incoming (listener) connection options are perhaps missing a feature. You can set global server connection options on a container, and you can set separate ssl_domain creds per listener, but you cannot set other connection options (idle_timeout/max_frame_size etc) per listener. This is reflected in Proton-C's acceptor and the readable events that it intercepts. There is no way to query the C API to find out which listener is the parent of an incoming connection or transfer other listener-specific context to the connection.
>
> As suggested in the link options review, I attempted to provide an initializer_list<foo> constructor and append, but that requires array elements of identical size. The varying size of the connection option types is unhelpful here. Maybe they should be counted_ptr based pimpl structs of identical size.
>
> This patch can also be viewed as the "reconnect" branch of https://github.com/cliffjansen/qpid-proton.
>
>
> Diffs
> -----
>
> examples/cpp/CMakeLists.txt 34edb83
> examples/cpp/connection_options.cpp PRE-CREATION
> proton-c/bindings/cpp/CMakeLists.txt 9f423fb
> proton-c/bindings/cpp/include/proton/connection_options.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/container.hpp 059416f
> proton-c/bindings/cpp/include/proton/reconnect_timer.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/transport.hpp 3d602b7
> proton-c/bindings/cpp/src/blocking_connection_impl.cpp 8abd24b
> proton-c/bindings/cpp/src/connection.cpp 1b35e03
> proton-c/bindings/cpp/src/connection_options.cpp PRE-CREATION
> proton-c/bindings/cpp/src/connector.hpp b311cc9
> proton-c/bindings/cpp/src/connector.cpp 49660ea
> proton-c/bindings/cpp/src/container.cpp 13f12d9
> proton-c/bindings/cpp/src/container_impl.hpp ec356e2
> proton-c/bindings/cpp/src/container_impl.cpp fc97a3e
> proton-c/bindings/cpp/src/reconnect_timer.cpp PRE-CREATION
> proton-c/bindings/cpp/src/transport.cpp 58114ae
>
> Diff: https://reviews.apache.org/r/39694/diff/
>
>
> Testing
> -------
>
> Linux only
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 39694: C++ binding connection options and reconnect
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39694/#review104505
-----------------------------------------------------------
I have what I think is a simpler implementation of connection options, I can't update the reviewboard look at:
https://github.com/alanconway/qpid-proton/commit/7eb1e87c1fdc8a72422b36770acbe0d3fa2deb1c
The logic is Cliff's (if I didn't miss anything) but the impl is shorter and I think more readable. If we like this we could do the same for link options.
NO-JIRA: Simpler implementation of connection options.
This is a simpler implementation of connection_options
- More compact, option tests and apply logic together in one apply() function.
- No limit on number of options that can be chained in call to connect()
- Simple value semantics. No need for virtual clone(), inheritance, pointers.
- Fewer heap allocations - one per option set, not per individual option.
Options class is pimpl with no virtuals so no binary compat issues adding new options.
/** Options for creating a connection.
*
* Options can be "chained" like this:
*
* c = container.connect(url, connection_options().handler(h).max_frame_size(1234));
*
* You can also create an options object with common settings and use it as a base
* for different connections that have mostly the same settings:
*
* connection_options opts;
* opts.idle_timeout(1000).max_frame_size(10000);
* c1 = container.connect(url1, opts.handler(h1));
* c2 = container.connect(url2, opts.handler(h2));
*
* Normal value semantics, copy or assign creates a separate copy of the options.
*/
- Alan Conway
On Oct. 27, 2015, 6:21 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39694/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2015, 6:21 p.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, and Justin Ross.
>
>
> Bugs: PROTON-865
> https://issues.apache.org/jira/browse/PROTON-865
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> This provides connection options (broadly similar to link options).
>
> As suggested by Alan, the collection connection_options is a subclass of the base connection_option so that they can be grouped and nested.
>
> They can also be copied and persisted via respective clone methods. Reconnect and transport options need to live as long as the connection. The handler option is special and needs to be extracted before other options to be available when the connection is created.
>
> Incoming (listener) connection options are perhaps missing a feature. You can set global server connection options on a container, and you can set separate ssl_domain creds per listener, but you cannot set other connection options (idle_timeout/max_frame_size etc) per listener. This is reflected in Proton-C's acceptor and the readable events that it intercepts. There is no way to query the C API to find out which listener is the parent of an incoming connection or transfer other listener-specific context to the connection.
>
> As suggested in the link options review, I attempted to provide an initializer_list<foo> constructor and append, but that requires array elements of identical size. The varying size of the connection option types is unhelpful here. Maybe they should be counted_ptr based pimpl structs of identical size.
>
> This patch can also be viewed as the "reconnect" branch of https://github.com/cliffjansen/qpid-proton.
>
>
> Diffs
> -----
>
> examples/cpp/CMakeLists.txt 34edb83
> examples/cpp/connection_options.cpp PRE-CREATION
> proton-c/bindings/cpp/CMakeLists.txt 9f423fb
> proton-c/bindings/cpp/include/proton/connection_options.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/container.hpp 059416f
> proton-c/bindings/cpp/include/proton/reconnect_timer.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/transport.hpp 3d602b7
> proton-c/bindings/cpp/src/blocking_connection_impl.cpp 8abd24b
> proton-c/bindings/cpp/src/connection.cpp 1b35e03
> proton-c/bindings/cpp/src/connection_options.cpp PRE-CREATION
> proton-c/bindings/cpp/src/connector.hpp b311cc9
> proton-c/bindings/cpp/src/connector.cpp 49660ea
> proton-c/bindings/cpp/src/container.cpp 13f12d9
> proton-c/bindings/cpp/src/container_impl.hpp ec356e2
> proton-c/bindings/cpp/src/container_impl.cpp fc97a3e
> proton-c/bindings/cpp/src/reconnect_timer.cpp PRE-CREATION
> proton-c/bindings/cpp/src/transport.cpp 58114ae
>
> Diff: https://reviews.apache.org/r/39694/diff/
>
>
> Testing
> -------
>
> Linux only
>
>
> Thanks,
>
> Cliff Jansen
>
>