You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by "Rob Godfrey (JIRA)" <ji...@apache.org> on 2013/03/27 14:51:17 UTC
[jira] [Comment Edited] (QPID-4659) [Java Broker] Refactor broker
to separate protocol independent from protocol specific classes
[ https://issues.apache.org/jira/browse/QPID-4659?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13615264#comment-13615264 ]
Rob Godfrey edited comment on QPID-4659 at 3/27/13 1:50 PM:
------------------------------------------------------------
{quote}
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?
{quote}
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.
{quote}
Commented out code in MessageConverter_0_10_to_1_0.java ?
and MessageConverter_0_8_to_1_0.java?
{quote}
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.
{quote}
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.. ?
{quote}
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.
{quote}
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?
{quote}
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:
{quote}
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?
{quote}
Possibly, there probably should be some sort of validation to check that the intersection of supported versions and available creators is non empty.
{quote}
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.
{quote}
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.
was (Author: rgodfrey):
{quote}
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?
{quote}
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.
{quote}
Commented out code in MessageConverter_0_10_to_1_0.java ?
and MessageConverter_0_8_to_1_0.java?
{quote}
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.
{quote}
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.. ?
{quote}
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.
{quote}
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?
{quote}
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?) addresses:
{quote}
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?
{quote}
Possibly, there probably should be some sort of validation to check that the intersection of supported versions and available creators is non empty.
{quote}
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.
{quote}
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.
> [Java Broker] Refactor broker to separate protocol independent from protocol specific classes
> ---------------------------------------------------------------------------------------------
>
> Key: QPID-4659
> URL: https://issues.apache.org/jira/browse/QPID-4659
> Project: Qpid
> Issue Type: Improvement
> Components: Java Broker
> Reporter: Rob Godfrey
> Assignee: Rob Godfrey
>
> 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.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org