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