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 14:11:30 GMT


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


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