Qpid
  1. Qpid
  2. QPID-4659

[Java Broker] Refactor broker to separate protocol independent from protocol specific classes

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.23
    • Component/s: Java Broker
    • Labels:
      None

      Description

      The Java Broker currently supports all versions of the AMQP protocol from 0-8 to 1.0, however the current structure of the code within the broker makes it hard to distinguish between code which is specific to a version of the protocol and code which is common across all protocols.

      By refactoring we can separate the protocol dependent and independent parts and allow for the possibility of separating out the different protocol implementations into independently loadable libraries.

      Fundamentally the refactoring takes the form of moving protocol specific classes into org.apache.qpid.server.protocol.v

      {0-8,0-10,1-0}

      and sub-packages and using the QpidClassLoader to load the protocol implementations (there are three separate implementations to load - the protocol delegate creators that interface to the IO code; the MessageMetaDataTypes used to (de)serialize the message data to stores; and MessageConverters used to convert between message formats and allow 0-8 messages to be received by 1-0 consumers.

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        120d 23h 31m 1 Rob Godfrey 17/Jul/13 22:31
        Resolved Resolved Closed Closed
        52d 16h 13m 1 Justin Ross 08/Sep/13 14:45
        Justin Ross made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Justin Ross added a comment -
        Show
        Justin Ross added a comment - Released in Qpid 0.24, http://qpid.apache.org/releases/qpid-0.24/index.html
        Rob Godfrey made changes -
        Field Original Value New Value
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 0.23 [ 12324273 ]
        Resolution Fixed [ 1 ]
        Hide
        ASF subversion and git services added a comment -

        Commit 1503798 from Rob Godfrey in branch 'qpid/trunk'
        [ https://svn.apache.org/r1503798 ]

        QPID-4659 : [Java Broker] fix pom generation

        Show
        ASF subversion and git services added a comment - Commit 1503798 from Rob Godfrey in branch 'qpid/trunk' [ https://svn.apache.org/r1503798 ] QPID-4659 : [Java Broker] fix pom generation
        Hide
        ASF subversion and git services added a comment -

        Commit 1503663 from Rob Godfrey in branch 'qpid/trunk'
        [ https://svn.apache.org/r1503663 ]

        QPID-4659 : [Java Broker] fix bdbstore dependencies on pluggable protocols

        Show
        ASF subversion and git services added a comment - Commit 1503663 from Rob Godfrey in branch 'qpid/trunk' [ https://svn.apache.org/r1503663 ] QPID-4659 : [Java Broker] fix bdbstore dependencies on pluggable protocols
        Hide
        ASF subversion and git services added a comment -

        Commit 1503651 from Rob Godfrey in branch 'qpid/trunk'
        [ https://svn.apache.org/r1503651 ]

        QPID-4659 : [Java Broker] move amqp 0-8 implementation into a plugin

        Show
        ASF subversion and git services added a comment - Commit 1503651 from Rob Godfrey in branch 'qpid/trunk' [ https://svn.apache.org/r1503651 ] QPID-4659 : [Java Broker] move amqp 0-8 implementation into a plugin
        Hide
        ASF subversion and git services added a comment -

        Commit 1503625 from Rob Godfrey in branch 'qpid/trunk'
        [ https://svn.apache.org/r1503625 ]

        QPID-4659 : [Java Broker] reduce unnecessary usage of 0-8 classes in tests

        Show
        ASF subversion and git services added a comment - Commit 1503625 from Rob Godfrey in branch 'qpid/trunk' [ https://svn.apache.org/r1503625 ] QPID-4659 : [Java Broker] reduce unnecessary usage of 0-8 classes in tests
        Hide
        ASF subversion and git services added a comment -

        Commit 1503575 from Rob Godfrey in branch 'qpid/trunk'
        [ https://svn.apache.org/r1503575 ]

        QPID-4659 : [Java Broker] reduce unnecessary usage of 0-8 classes in tests

        Show
        ASF subversion and git services added a comment - Commit 1503575 from Rob Godfrey in branch 'qpid/trunk' [ https://svn.apache.org/r1503575 ] QPID-4659 : [Java Broker] reduce unnecessary usage of 0-8 classes in tests
        Hide
        ASF subversion and git services added a comment -

        Commit 1503523 from Rob Godfrey in branch 'qpid/trunk'
        [ https://svn.apache.org/r1503523 ]

        QPID-4659 : [Java Broker] tidy up amqp 0-8 implementation, reduce unnecessary usage in tests

        Show
        ASF subversion and git services added a comment - Commit 1503523 from Rob Godfrey in branch 'qpid/trunk' [ https://svn.apache.org/r1503523 ] QPID-4659 : [Java Broker] tidy up amqp 0-8 implementation, reduce unnecessary usage in tests
        Hide
        ASF subversion and git services added a comment -

        Commit 1503446 from Rob Godfrey in branch 'qpid/trunk'
        [ https://svn.apache.org/r1503446 ]

        QPID-4659 : [Java Broker] move amqp 0-10 implementation into a plugin

        Show
        ASF subversion and git services added a comment - Commit 1503446 from Rob Godfrey in branch 'qpid/trunk' [ https://svn.apache.org/r1503446 ] QPID-4659 : [Java Broker] move amqp 0-10 implementation into a plugin
        Hide
        ASF subversion and git services added a comment -

        Commit 1503303 from Rob Godfrey in branch 'qpid/trunk'
        [ https://svn.apache.org/r1503303 ]

        QPID-4659 : [Java Broker] move amqp 1-0 implementation into a plugin

        Show
        ASF subversion and git services added a comment - Commit 1503303 from Rob Godfrey in branch 'qpid/trunk' [ https://svn.apache.org/r1503303 ] QPID-4659 : [Java Broker] move amqp 1-0 implementation into a plugin
        Hide
        ASF subversion and git services added a comment -

        Commit 1503272 from Rob Godfrey in branch 'qpid/trunk'
        [ https://svn.apache.org/r1503272 ]

        QPID-4659 : [Java Broker] remove redundant code

        Show
        ASF subversion and git services added a comment - Commit 1503272 from Rob Godfrey in branch 'qpid/trunk' [ https://svn.apache.org/r1503272 ] QPID-4659 : [Java Broker] remove redundant code
        Hide
        ASF subversion and git services added a comment -

        Commit 1503267 from Rob Godfrey in branch 'qpid/trunk'
        [ https://svn.apache.org/r1503267 ]

        QPID-4659 : [Java Broker] make message fomat conversions pluggable for different protcol versions

        Show
        ASF subversion and git services added a comment - Commit 1503267 from Rob Godfrey in branch 'qpid/trunk' [ https://svn.apache.org/r1503267 ] QPID-4659 : [Java Broker] make message fomat conversions pluggable for different protcol versions
        Hide
        ASF subversion and git services added a comment -

        Commit 1503192 from Rob Godfrey in branch 'qpid/trunk'
        [ https://svn.apache.org/r1503192 ]

        QPID-4659 : [Java Broker] make message meta data pluggable for different protcol versions

        Show
        ASF subversion and git services added a comment - Commit 1503192 from Rob Godfrey in branch 'qpid/trunk' [ https://svn.apache.org/r1503192 ] QPID-4659 : [Java Broker] make message meta data pluggable for different protcol versions
        Hide
        ASF subversion and git services added a comment -

        Commit 1503076 from Rob Godfrey in branch 'qpid/trunk'
        [ https://svn.apache.org/r1503076 ]

        QPID-4659 : [Java Broker] fix protocol version specific code in logging, subscriptions

        Show
        ASF subversion and git services added a comment - Commit 1503076 from Rob Godfrey in branch 'qpid/trunk' [ https://svn.apache.org/r1503076 ] QPID-4659 : [Java Broker] fix protocol version specific code in logging, subscriptions
        Hide
        ASF subversion and git services added a comment -

        Commit 1503024 from Rob Godfrey in branch 'qpid/trunk'
        [ https://svn.apache.org/r1503024 ]

        QPID-4659 : [Java Broker] make protocol engines pluggable

        Show
        ASF subversion and git services added a comment - Commit 1503024 from Rob Godfrey in branch 'qpid/trunk' [ https://svn.apache.org/r1503024 ] QPID-4659 : [Java Broker] make protocol engines pluggable
        Hide
        ASF subversion and git services added a comment -

        Commit 1502993 from Rob Godfrey in branch 'qpid/trunk'
        [ https://svn.apache.org/r1502993 ]

        QPID-4659 : [Java Broker] preliminary refactoring in preparation for pluggable protocols

        Show
        ASF subversion and git services added a comment - Commit 1502993 from Rob Godfrey in branch 'qpid/trunk' [ https://svn.apache.org/r1502993 ] QPID-4659 : [Java Broker] preliminary refactoring in preparation for pluggable protocols
        Hide
        Rob Godfrey added a comment - - edited

        MessageMetaDataTypeRegistry.java doesnt check whether there were no MessageMetaDataType protocol creator implementations (the service loader has a method to validate that for you), or whether diffrent types accidentally (or not) used the same ordinal, it should probably do both?

        Updated to use the method which checks for at least one instance, also throws an exception if two types are defined for the same ordinal.

        Commented out code in MessageConverter_0_10_to_1_0.java ?
        and MessageConverter_0_8_to_1_0.java?

        These were the properties not currently being set. I have changed the comments to make this clear and no longer make it look like code.

        Should MessageConverter_1_0_to_0_10.java return null from convert, or perhaps throw an Unsupported exception? Will it will just NPE after using that method? The new subscription interest checking seems like it would never try to convert the messages if we just removed the not-implemented converter.. ?

        These converters were not actually registered and thus never used. Obviously in time they should be implemented and registered, but for now I have simply removed them.

        Why move this line? Does it log connections to the vhost earlier now as a result, and then subsequently fail if its denied ACL access or the Vhost isnt the Master in a HA cluster? If so, do we really want it to do that?

        I have reverted this change and instead fixed the getVirtualHostName() method on the various implementations of AMQConnectionModel that were giving NPE with the setVirtualHost() in its original position.

        Not (yet?) addressed:

        Continuing that theme, should we throw an exception in MultiVersionProtocolEngineFactory.java if there are no protocol creator implementations, or more than one was to specify the same header value?

        Possibly, there probably should be some sort of validation to check that the intersection of supported versions and available creators is non empty.

        It seemed preferable when there was an enum for MessageMetaDataType to reference the ordinals from rather than each type just choosing the int directly. Obviously if we retained an Enum it would have to go somewhere common that all the impls could reference it though, and adding new plugins later would still involve choosing the new int and updating the enum at some point, so I can kind of see why you just removed it, it jsut felt a little nicer when it was there.

        Yeah - it's unfortunate... but I think having an Enum there would sort of defeat the point of Pluggability. I have modified the code so that each of the ordinals is now defined as a public constant in the implementing class so they can be more easily referred to.

        Show
        Rob Godfrey added a comment - - edited MessageMetaDataTypeRegistry.java doesnt check whether there were no MessageMetaDataType protocol creator implementations (the service loader has a method to validate that for you), or whether diffrent types accidentally (or not) used the same ordinal, it should probably do both? Updated to use the method which checks for at least one instance, also throws an exception if two types are defined for the same ordinal. Commented out code in MessageConverter_0_10_to_1_0.java ? and MessageConverter_0_8_to_1_0.java? These were the properties not currently being set. I have changed the comments to make this clear and no longer make it look like code. Should MessageConverter_1_0_to_0_10.java return null from convert, or perhaps throw an Unsupported exception? Will it will just NPE after using that method? The new subscription interest checking seems like it would never try to convert the messages if we just removed the not-implemented converter.. ? These converters were not actually registered and thus never used. Obviously in time they should be implemented and registered, but for now I have simply removed them. Why move this line? Does it log connections to the vhost earlier now as a result, and then subsequently fail if its denied ACL access or the Vhost isnt the Master in a HA cluster? If so, do we really want it to do that? I have reverted this change and instead fixed the getVirtualHostName() method on the various implementations of AMQConnectionModel that were giving NPE with the setVirtualHost() in its original position. Not (yet?) addressed: Continuing that theme, should we throw an exception in MultiVersionProtocolEngineFactory.java if there are no protocol creator implementations, or more than one was to specify the same header value? Possibly, there probably should be some sort of validation to check that the intersection of supported versions and available creators is non empty. It seemed preferable when there was an enum for MessageMetaDataType to reference the ordinals from rather than each type just choosing the int directly. Obviously if we retained an Enum it would have to go somewhere common that all the impls could reference it though, and adding new plugins later would still involve choosing the new int and updating the enum at some point, so I can kind of see why you just removed it, it jsut felt a little nicer when it was there. Yeah - it's unfortunate... but I think having an Enum there would sort of defeat the point of Pluggability. I have modified the code so that each of the ordinals is now defined as a public constant in the implementing class so they can be more easily referred to.
        Hide
        Robbie Gemmell added a comment -

        Looks good overall, and involved far less actual change than I expected, which is nice (the diff itself is large though ).

        Some nitpicks/questions:

        It seemed preferable when there was an enum for MessageMetaDataType to reference the ordinals from rather than each type just choosing the int directly. Obviously if we retained an Enum it would have to go somewhere common that all the impls could reference it though, and adding new plugins later would still involve choosing the new int and updating the enum at some point, so I can kind of see why you just removed it, it jsut felt a little nicer when it was there.

        MessageMetaDataTypeRegistry.java doesnt check whether there were no MessageMetaDataType protocol creator implementations (the service loader has a method to validate that for you), or whether diffrent types accidentally (or not) used the same ordinal, it should probably do both?

        Continuing that theme, should we throw an exception in MultiVersionProtocolEngineFactory.java if there are no protocol creator implementations, or more than one was to specify the same header value?

        Commented out code in MessageConverter_0_10_to_1_0.java ?
        and MessageConverter_0_8_to_1_0.java?

        Should MessageConverter_1_0_to_0_10.java return null from convert, or perhaps throw an Unsupported exception? Will it will just NPE after using that method? The new subscription interest checking seems like it would never try to convert the messages if we just removed the not-implemented converter.. ?

        Why move this line? Does it log connections to the vhost earlier now as a result, and then subsequently fail if its denied ACL access or the Vhost isnt the Master in a HA cluster? If so, do we really want it to do that?

        @@ -78,6 +78,8 @@ public class ConnectionOpenMethodHandler
                 }
                 else
                 {
        +            session.setVirtualHost(
        virtualHost);
        +
                     // Check virtualhost access
                     if (!virtualHost.getSecurityManager().accessVirtualhost(virtualHostName, session.getRemoteAddress()))
                     {
        @@ -88,7 +90,6 @@ public class ConnectionOpenMethodHandler
                         throw body.getConnectionException(AMQConstant.CONNECTION_FORCED, "Virtual host '" + virtualHost.getName() + "' is not active");
                     }
        
        -            session.setVirtualHost(virtualHost);
        
        
        Show
        Robbie Gemmell added a comment - Looks good overall, and involved far less actual change than I expected, which is nice (the diff itself is large though ). Some nitpicks/questions: It seemed preferable when there was an enum for MessageMetaDataType to reference the ordinals from rather than each type just choosing the int directly. Obviously if we retained an Enum it would have to go somewhere common that all the impls could reference it though, and adding new plugins later would still involve choosing the new int and updating the enum at some point, so I can kind of see why you just removed it, it jsut felt a little nicer when it was there. MessageMetaDataTypeRegistry.java doesnt check whether there were no MessageMetaDataType protocol creator implementations (the service loader has a method to validate that for you), or whether diffrent types accidentally (or not) used the same ordinal, it should probably do both? Continuing that theme, should we throw an exception in MultiVersionProtocolEngineFactory.java if there are no protocol creator implementations, or more than one was to specify the same header value? Commented out code in MessageConverter_0_10_to_1_0.java ? and MessageConverter_0_8_to_1_0.java? Should MessageConverter_1_0_to_0_10.java return null from convert, or perhaps throw an Unsupported exception? Will it will just NPE after using that method? The new subscription interest checking seems like it would never try to convert the messages if we just removed the not-implemented converter.. ? Why move this line? Does it log connections to the vhost earlier now as a result, and then subsequently fail if its denied ACL access or the Vhost isnt the Master in a HA cluster? If so, do we really want it to do that? @@ -78,6 +78,8 @@ public class ConnectionOpenMethodHandler } else { + session.setVirtualHost( virtualHost); + // Check virtualhost access if (!virtualHost.getSecurityManager().accessVirtualhost(virtualHostName, session.getRemoteAddress())) { @@ -88,7 +90,6 @@ public class ConnectionOpenMethodHandler throw body.getConnectionException(AMQConstant.CONNECTION_FORCED, "Virtual host '" + virtualHost.getName() + "' is not active"); } - session.setVirtualHost(virtualHost);
        Hide
        Rob Godfrey added a comment -

        Create a branch with the refactoring work committed:

        https://svn.apache.org/repos/asf/qpid/branches/QPID-4659/

        Show
        Rob Godfrey added a comment - Create a branch with the refactoring work committed: https://svn.apache.org/repos/asf/qpid/branches/QPID-4659/
        Rob Godfrey created issue -

          People

          • Assignee:
            Rob Godfrey
            Reporter:
            Rob Godfrey
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development