On Mon, Nov 17, 2014 at 4:48 PM, Andrew Stitcher <[email protected]> wrote:
> On Mon, 2014-11-17 at 20:56 +0000, Gordon Sim wrote: > > On 11/17/2014 08:14 PM, Andrew Stitcher wrote: > > > As we currently don't have reviewboard set up for Qpid Proton (since > the > > > git migration). > > > > > > I've posted a pull request against the Github Apache proton mirror to > > > get review feedback for my IO layer refactoring. > > > > > > https://github.com/apache/qpid-proton/pull/1 > > > > > > Please give it some attention, I know that Rafi would like this change > > > to go in quickly as some important changes are queued up behind it. > > > > I'm confused around the intention with regards to backward > > compatibility. There seem to be changes that would break this, which is > > entirely fine, but then old functions are retained but deprecated. If > > the contract is indeed broken in some way, I think it would be better > > not to retain functions that are no longer intended for use. > > I don't think previously working code will stop working with these > changes (but I've not extensively tried). In other words I don't think > there is a broken contract at least not for pn_transport_t. > > Previously there was no transport server/client distinction, which > effectively made the transport what is the current client transport > which is why that is the default. > > As for pn_sasl_t. This now exactly follows the transport, so I guess > there is a possible broken contract. > > In either case I'm very happy to remove the deprecated APIs - I have > left the change so this is trivially easy. Rafi suggested leaving them > in so I left them in. > I think what I suggested here was not to deprecate, but to actually keep the old API. Prior to this change, the usage pattern for a transport is this: // construct t = pn_transport(); // configure pn_transport_set_blah(...); pn_ssl_t *ssl = pn_ssl(...); pn_ssl_set_blah(...); // use p = pn_transport_pending(...); pn_transport_peek(...); pn_transport_pop(...); As soon as you start using the transport, all the configuration is locked in, i.e. it is an error to change it and bad stuff happens if you try. The picture after the change is similar, but IMHO a bit muddied: // multiple constructors t = pn_transport_client(); // *or* t = pn_transport_server(); // configure (a bit cleaned up/modified, but still part of a distinct configuration phase) pn_transport_set_blah(...); pn_ssl_t *ssl = pn_ssl(...); pn_ssl_set_blah(...); // use p = pn_transport_pending(...); pn_transport_peek(...); pn_transport_pop(...); And again as soon as you start using the transport, all the configuration is locked in. There are a couple things I don't like about the API after the change. 1. pn_transport_client()/pn_transport_server() don't follow our naming conventions, as constructors you would expect them to return distinct types, however they both return pn_transport_t. 2. Given that we have a distinct configuration phase regardless, it is simply unnecessary to force people to choose client vs server up front. We can simply default the mode to client and have people set it to server during the configuration phase if that is what they wish. I would suggest the following options for this particular API point as both being preferable. 1. if we really want to force people to choose up front, the right way to do it is with a single constructor: pn_transport_t *t = pn_transport(CLIENT); /* or */ pn_transport(SERVER) at east this follows our API conventions 2. don't force people to choose up front pn_transport_t *t = pn_transport(); // defaults to client pn_transport_set_server(t, true); pn_transport_is_server(t) --> true As a general pattern, I prefer to stick with (2). I think it's more flexible/extensible and allows the addition of pretty much arbitrary configuration/construction parameters without ever actually breaking the API. As a general pattern, the alternative is IMHO really quite brittle, e.g. if we have another parameter we want to add in the next release we need to choose between a combinatorial explosion of constructors, or breaking the API yet again: pn_transport_websockets_server(); // confusing/bloated API pn_transport(SERVER, WEBSOCKETS); // breaks everytime we add a parameter > > > > In a similar vein for the python binding, for the Transport constructor, > > I think it would be better to insist that either a c transport object or > > a mode is provided, rather than defaulting to a client. > > I'd be happy with this although as I said above the previous usage is > equivalent to the new client transport. > > If you feel strongly, and there is no dissent I will follow this advice. > I think if you insist upon breaking the API then option (1) is preferable to what you currently have, however as I said above, I don't believe there is really any reason to break the API. --Rafael
