qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alan Conway" <acon...@redhat.com>
Subject Re: Review Request 39694: C++ binding connection options and reconnect
Date Fri, 30 Oct 2015 01:19:03 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message