Qpid
  1. Qpid
  2. QPID-3342

Rationalise transport layer by introducing common interfaces shared by all protocols

    Details

      Description

      Refactor the transport stack by introducing two new interfaces NetworkTransport and NetworkConnection. Remove dependencies between IoSender and IoReceiver.

        Activity

        Hide
        Rajith Attapattu added a comment -

        Keith,

        Is this different from the work Andrew Kennedy is doing?
        And is the NetworkTransport interface you are referring to is a new one or is it the one in org.apache.qpid.transport.network package ?

        I have created a minimal abstraction for transports on the client side where transports implementing the NetworkTransport interface can be loaded via Reflection (See Transport.java and NetworkTransport.java in org.apache.qpid.transport.network)
        Wondering what your thoughts are on that ?

        Rajith

        Show
        Rajith Attapattu added a comment - Keith, Is this different from the work Andrew Kennedy is doing? And is the NetworkTransport interface you are referring to is a new one or is it the one in org.apache.qpid.transport.network package ? I have created a minimal abstraction for transports on the client side where transports implementing the NetworkTransport interface can be loaded via Reflection (See Transport.java and NetworkTransport.java in org.apache.qpid.transport.network) Wondering what your thoughts are on that ? Rajith
        Hide
        Keith Wall added a comment -

        Patch to introduce common interface layer, shared by all transport.s

        Show
        Keith Wall added a comment - Patch to introduce common interface layer, shared by all transport.s
        Hide
        Robbie Gemmell added a comment -

        Hi Rajith,

        This work is similar to the work Andrew was doing, yes, but he isn't working on it anymore and this is fresh implementation with some commonality.

        The NetworkTransport being mentioned is different one, partially replacing the existing one (NetworkTransport extended by IncomingNetworkTransport and OutgoingNetworkTransport). There is a final JIRA to be raised to make these transports using these new interfaces pluggable in similar fashion to what you did.

        Show
        Robbie Gemmell added a comment - Hi Rajith, This work is similar to the work Andrew was doing, yes, but he isn't working on it anymore and this is fresh implementation with some commonality. The NetworkTransport being mentioned is different one, partially replacing the existing one (NetworkTransport extended by IncomingNetworkTransport and OutgoingNetworkTransport). There is a final JIRA to be raised to make these transports using these new interfaces pluggable in similar fashion to what you did.
        Hide
        Robbie Gemmell added a comment -

        Small elaboration on the above, Keith and I have been working on this together as discussed on the mailing list previously. It is being attached here to grant the appropriate permissions for inclusion.

        Show
        Robbie Gemmell added a comment - Small elaboration on the above, Keith and I have been working on this together as discussed on the mailing list previously. It is being attached here to grant the appropriate permissions for inclusion.
        Hide
        Rajith Attapattu added a comment -

        Hi Robbie,

        Thanks for the explanation. I will have a look at the patches as well.
        As long as we retain the ability to load transports via reflection I am quite happy.

        The work I did was motivated by the following,
        1. Getting rid of MINA deps from the client/common.
        2. Have the default transport (the java.io) bundled with the client.
        3. Other transports to be housed under a different module (ex transports) and bundle that as a separate jar.

        Show
        Rajith Attapattu added a comment - Hi Robbie, Thanks for the explanation. I will have a look at the patches as well. As long as we retain the ability to load transports via reflection I am quite happy. The work I did was motivated by the following, 1. Getting rid of MINA deps from the client/common. 2. Have the default transport (the java.io) bundled with the client. 3. Other transports to be housed under a different module (ex transports) and bundle that as a separate jar.
        Hide
        Rajith Attapattu added a comment -

        Robbie,

        This is a significant amount of code that has been committed with zero review or discussion.
        I will comment on the changes later, but overall I am quite disappointed in the way this was done !

        1. The commit was made, just a few hours after the patches were attached to the JIRA.

        2. For a change of this magnitude I would have appreciated if it was put up for review first. It's only fair to do so as lot of others folks in the project are doing it.

        3. There is no summary or a design doc that explains what's been done or what exactly the vision here is. When Andrew Kennedy attempted to do this, he had detailed JIRA's and a decent design doc.

        4. I can't seem to find if transports are configurable or not - i.e the existing mechanism for loading transports via Reflection was removed without a replacement. This was done for a reason and I have repeatedly stressed this on the JIRA's created by Andrew.

        I will comment on some of the code changes separately. Again I am not happy with the way these changes were done.

        Rajith

        Show
        Rajith Attapattu added a comment - Robbie, This is a significant amount of code that has been committed with zero review or discussion. I will comment on the changes later, but overall I am quite disappointed in the way this was done ! 1. The commit was made, just a few hours after the patches were attached to the JIRA. 2. For a change of this magnitude I would have appreciated if it was put up for review first. It's only fair to do so as lot of others folks in the project are doing it. 3. There is no summary or a design doc that explains what's been done or what exactly the vision here is. When Andrew Kennedy attempted to do this, he had detailed JIRA's and a decent design doc. 4. I can't seem to find if transports are configurable or not - i.e the existing mechanism for loading transports via Reflection was removed without a replacement. This was done for a reason and I have repeatedly stressed this on the JIRA's created by Andrew. I will comment on some of the code changes separately. Again I am not happy with the way these changes were done. Rajith
        Hide
        Rajith Attapattu added a comment -

        I briefly looked at some of changes on the client side and let me start with a positive comment
        It seems a lot of unused code has been deleted. This was long overdue and kudos for doing so.

        One thing I wasn't very happy about was the removal of TransportBuilder.java - All though it wasn't perfect, this class at least abstracted out the building of sender and receiver pipelines. Now that code is been put back directly into the Connection class. Not exactly an improvement unless what I see so far is interim code with more changes being planed.

        This is where a review/discussion along with a write up could have helped. I now see QPID-3345. It's quite possible that I am missing pieces of the final puzzle. But then again these things should have been communicated upfront.

        Show
        Rajith Attapattu added a comment - I briefly looked at some of changes on the client side and let me start with a positive comment It seems a lot of unused code has been deleted. This was long overdue and kudos for doing so. One thing I wasn't very happy about was the removal of TransportBuilder.java - All though it wasn't perfect, this class at least abstracted out the building of sender and receiver pipelines. Now that code is been put back directly into the Connection class. Not exactly an improvement unless what I see so far is interim code with more changes being planed. This is where a review/discussion along with a write up could have helped. I now see QPID-3345 . It's quite possible that I am missing pieces of the final puzzle. But then again these things should have been communicated upfront.
        Hide
        Robbie Gemmell added a comment -

        Hi Rajith

        Im sorry you feel that way, and have to say I disagree in general, but as some attempt at explanation:

        This is as mentioned above, very much in line with the work Andrew was doing previously so its design has as you mention been proposed, and has had the previous code available on branches for close to 9 months now. Given this, despite this being a reimplementation (involving less change in a more controlled fashion), I dont think this is entirely out of the blue and it seems like ample discussion had taken place and time to consider it been given.

        I can conceed your point about possibly putting it up for review first, although I would usually only take course for something that hadnt been given prior discussion, hadnt had any review, and had some doubt about it. I would stress that this (and Andrews code before it) has already seen weeks of review from myself before I committed it, and again the previous implemtnation has been available for all to look at. Great care was taken to try and ensure consistency of the IO behaviour during the restructuring (which was actually fairly minimal for the 0-10 code path), and no IO features which were present and worked or were being used are being (permanantly) removed. Much of the change that went in was nothing to do with the IO layer alongside work, and is purely to the Java broker or associated dead Mina related features.

        The transports are not currently configurable, however as I mentioned above they will again be made so via a final JIRA (quite possibly tomorrow), in similar fashion to the way they were already. I didn't think this small delay was a particular sticking point for the rest of the code given that until these changes were made there were no two interface-compatible IO implementations available to use such plugability.

        Show
        Robbie Gemmell added a comment - Hi Rajith Im sorry you feel that way, and have to say I disagree in general, but as some attempt at explanation: This is as mentioned above, very much in line with the work Andrew was doing previously so its design has as you mention been proposed, and has had the previous code available on branches for close to 9 months now. Given this, despite this being a reimplementation (involving less change in a more controlled fashion), I dont think this is entirely out of the blue and it seems like ample discussion had taken place and time to consider it been given. I can conceed your point about possibly putting it up for review first, although I would usually only take course for something that hadnt been given prior discussion, hadnt had any review, and had some doubt about it. I would stress that this (and Andrews code before it) has already seen weeks of review from myself before I committed it, and again the previous implemtnation has been available for all to look at. Great care was taken to try and ensure consistency of the IO behaviour during the restructuring (which was actually fairly minimal for the 0-10 code path), and no IO features which were present and worked or were being used are being (permanantly) removed. Much of the change that went in was nothing to do with the IO layer alongside work, and is purely to the Java broker or associated dead Mina related features. The transports are not currently configurable, however as I mentioned above they will again be made so via a final JIRA (quite possibly tomorrow), in similar fashion to the way they were already. I didn't think this small delay was a particular sticking point for the rest of the code given that until these changes were made there were no two interface-compatible IO implementations available to use such plugability.
        Hide
        Robbie Gemmell added a comment -

        Cross-posted comments. Yes, that JIRA is the final piece in the puzzle. As per Andrews previous design, the pluggability will occur at the NetworkTansport level in similar fashion to the previous 'getTransport' behaviour you added, with the building and lifecycle maintenance of the Sender/Reciever pipes being the responsability of the NetworkConnections those NetworkTransports produce.

        Show
        Robbie Gemmell added a comment - Cross-posted comments. Yes, that JIRA is the final piece in the puzzle. As per Andrews previous design, the pluggability will occur at the NetworkTansport level in similar fashion to the previous 'getTransport' behaviour you added, with the building and lifecycle maintenance of the Sender/Reciever pipes being the responsability of the NetworkConnections those NetworkTransports produce.
        Hide
        Rajith Attapattu added a comment -

        Robbie,

        While it is true that Andrew proposal was covered with detailed JIRA's a design doc and a fair amount of discussion, there was no indication that the changes committed were infact from that branch. As you've mentioned it's a reimplementation and therefore would have been nice if some heads up was given. At the least an email to the dev list highlighting the scope of the impending commit (and any future work being planned) and mentioning that this was an extension of Andrew's work.

        If these changes were infact from that branch, then they could have been committed under Andrew's JIRA's which I believe is the right thing to do. Instead those JIRA's were marked invalid and two new JIRA's were created this morning and the changes were committed under them, giving the impression this was a completely different thing.

        I don't doubt your competence at all and as I have mentioned you do quite a good job at reviews. But the fact remains that it's a substantial amount of work and some advance notification about the impending commit would have not only being courteous but also fair. I agree that minimal changes was done on the client side and most of the changes were on the broker side. I also agree that this is not the first instance that this has happened. But none of these are excuses for the clear lack of communication here.

        Regards,

        Rajith

        Show
        Rajith Attapattu added a comment - Robbie, While it is true that Andrew proposal was covered with detailed JIRA's a design doc and a fair amount of discussion, there was no indication that the changes committed were infact from that branch. As you've mentioned it's a reimplementation and therefore would have been nice if some heads up was given. At the least an email to the dev list highlighting the scope of the impending commit (and any future work being planned) and mentioning that this was an extension of Andrew's work. If these changes were infact from that branch, then they could have been committed under Andrew's JIRA's which I believe is the right thing to do. Instead those JIRA's were marked invalid and two new JIRA's were created this morning and the changes were committed under them, giving the impression this was a completely different thing. I don't doubt your competence at all and as I have mentioned you do quite a good job at reviews. But the fact remains that it's a substantial amount of work and some advance notification about the impending commit would have not only being courteous but also fair. I agree that minimal changes was done on the client side and most of the changes were on the broker side. I also agree that this is not the first instance that this has happened. But none of these are excuses for the clear lack of communication here. Regards, Rajith
        Hide
        Robbie Gemmell added a comment -

        These changes were done from scratch and are not from Andrews branch, but they do share a common goal and overall design. Two of the four older JIRAs I closed are not being underaken due to prior failure to make them work and our subsequent removal of the vm transport, as per recent discussion on the mailing list concerning the work. We had initially planned to reuse the old IO JIRAs, however when it came time to commit we decided it would be better to create new ones and distinguish from the earlier work because the same steps were not followed and the commits are not related enough at the code level in the way that reusing old JIRAs associated with 9 months of work on 2 old branches would seem to imply, and so we thought that to do so would be confusing for people looking at them.

        The code changed at the IO layer is so rarely touched and the Java broker doesnt see a whole lot of people changing it, so I didnt think a heads up to e,g. help other developers avoid conflict was necessary.

        Show
        Robbie Gemmell added a comment - These changes were done from scratch and are not from Andrews branch, but they do share a common goal and overall design. Two of the four older JIRAs I closed are not being underaken due to prior failure to make them work and our subsequent removal of the vm transport, as per recent discussion on the mailing list concerning the work. We had initially planned to reuse the old IO JIRAs, however when it came time to commit we decided it would be better to create new ones and distinguish from the earlier work because the same steps were not followed and the commits are not related enough at the code level in the way that reusing old JIRAs associated with 9 months of work on 2 old branches would seem to imply, and so we thought that to do so would be confusing for people looking at them. The code changed at the IO layer is so rarely touched and the Java broker doesnt see a whole lot of people changing it, so I didnt think a heads up to e,g. help other developers avoid conflict was necessary.
        Hide
        Robbie Gemmell added a comment -

        Patch applied, closing out.

        Show
        Robbie Gemmell added a comment - Patch applied, closing out.
        Hide
        Keith Wall added a comment -

        This refactoring is causing an exception in the client when SSL is in-use, and can be highlighted by running SSLTest:

        Testcase: testCreateSSLContextFromConnectionURLParams took 1.642 sec
                FAILED
        SSL Connection should be successful
        junit.framework.AssertionFailedError: SSL Connection should be successful
                at org.apache.qpid.client.ssl.SSLTest.testCreateSSLContextFromConnectionURLParams(SSLTest.java:81)
                at org.apache.qpid.test.utils.QpidBrokerTestCase.runBare(QpidBrokerTestCase.java:243)
                at org.apache.qpid.test.utils.QpidTestCase.run(QpidTestCase.java:125)
        
        Testcase: testMultipleCertsInSingleStore took 0.056 sec
                Caused an ERROR
        SecurityLayer.sender method should be invoked before SecurityLayer.receiver
        org.apache.qpid.AMQConnectionFailureException: SecurityLayer.sender method should be invoked before SecurityLayer.receiver
                at org.apache.qpid.client.AMQConnection.<init>(AMQConnection.java:473)
                at org.apache.qpid.client.AMQConnection.<init>(AMQConnection.java:246)
                at org.apache.qpid.client.AMQTestConnection_0_10.<init>(AMQTestConnection_0_10.java:27)
                at org.apache.qpid.client.ssl.SSLTest.testMultipleCertsInSingleStore(SSLTest.java:101)
                at org.apache.qpid.test.utils.QpidBrokerTestCase.runBare(QpidBrokerTestCase.java:243)
                at org.apache.qpid.test.utils.QpidTestCase.run(QpidTestCase.java:125)
        Caused by: java.lang.IllegalStateException: SecurityLayer.sender method should be invoked before SecurityLayer.receiver
        
        
        
        Show
        Keith Wall added a comment - This refactoring is causing an exception in the client when SSL is in-use, and can be highlighted by running SSLTest: Testcase: testCreateSSLContextFromConnectionURLParams took 1.642 sec FAILED SSL Connection should be successful junit.framework.AssertionFailedError: SSL Connection should be successful at org.apache.qpid.client.ssl.SSLTest.testCreateSSLContextFromConnectionURLParams(SSLTest.java:81) at org.apache.qpid.test.utils.QpidBrokerTestCase.runBare(QpidBrokerTestCase.java:243) at org.apache.qpid.test.utils.QpidTestCase.run(QpidTestCase.java:125) Testcase: testMultipleCertsInSingleStore took 0.056 sec Caused an ERROR SecurityLayer.sender method should be invoked before SecurityLayer.receiver org.apache.qpid.AMQConnectionFailureException: SecurityLayer.sender method should be invoked before SecurityLayer.receiver at org.apache.qpid.client.AMQConnection.<init>(AMQConnection.java:473) at org.apache.qpid.client.AMQConnection.<init>(AMQConnection.java:246) at org.apache.qpid.client.AMQTestConnection_0_10.<init>(AMQTestConnection_0_10.java:27) at org.apache.qpid.client.ssl.SSLTest.testMultipleCertsInSingleStore(SSLTest.java:101) at org.apache.qpid.test.utils.QpidBrokerTestCase.runBare(QpidBrokerTestCase.java:243) at org.apache.qpid.test.utils.QpidTestCase.run(QpidTestCase.java:125) Caused by: java.lang.IllegalStateException: SecurityLayer.sender method should be invoked before SecurityLayer.receiver
        Hide
        Keith Wall added a comment -

        Hi Robbie

        Can you review this patch to resolve this Jira?

        Show
        Keith Wall added a comment - Hi Robbie Can you review this patch to resolve this Jira?
        Hide
        Robbie Gemmell added a comment -

        Patch (by me and Keith) applied.

        Show
        Robbie Gemmell added a comment - Patch (by me and Keith) applied.

          People

          • Assignee:
            Robbie Gemmell
            Reporter:
            Keith Wall
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development