[
https://issues.apache.org/jira/browse/QPID-2360?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12847436#action_12847436
]
Robbie Gemmell commented on QPID-2360:
--------------------------------------
The trunk patch does not compile because it also contains various other
(unfinished?) changes to classes unrelated to the update of the
server/virtualhost configuration and their tests, eg: reorganised imports,
addition of generic types, spacing, annotations etc. MockAMQQueue changes also
add a tab character that should be removed. It would be best to split these
types of change and any others which aren't related to the configuration
updates out into a new JIRA and patch, as it will make both a lot easier to
digest.
The changes to ServeConfiguration itself in order to address the issues raised
look good, however there are some points to address with the changes:
ServerConfigurationTest: L904 is now a function definition that does nothing
other than call itself, and so gets stuck in an infinite recursion when used by
the Firewall related test methods, leading to a StackOverFlowException when
they are run.
ServerConfiguration: L165 The exception should be updated to say "Only one
external virtualhosts configuration *file* allowed, *multiple filenames found*"
to avoid users thinking they have too many virtualhosts should the exception
ever be thrown, rather than that they have just specified multiple strings that
we then assume to be file locations.
ServerConfiguration: L192 This comment has been made invalid by the patch
changes and should be removed: "// Add the keys of the virtual host to the main
config"
I think that the broker/etc/log4j.xml configuration should not be modified to
suppress warnings regarding optional config files not being present, with the
associated logging level left as-is. Primarily only the tests use the
ConfigurationFactory abilities and then the optional ability on top of that, so
this will rarely if ever be logged in actual use, but it should be output when
it is.
broker/etc/transient_config.xml and broker/etc/persistent_config.xml should
just be removed (they are not really used at all, at worst some tests use them
and if so they should just be updated to use overrides where required.) rather
than updating them and adding new individualised virtualhosts files for each
and causing more clutter in the repository and Apache release bundles.
Related to the above, the new virtualhost files referenced in the patch changes
are not actually included in the patch and so are missing. Similarly for the
main new broker/etc/virtualhosts.xml file and various new virtualhosts files
for the Systests module.
> declaring virtualhost level firewall configuration in virtualhosts.xml leads
> to NPE on startup
> ----------------------------------------------------------------------------------------------
>
> Key: QPID-2360
> URL: https://issues.apache.org/jira/browse/QPID-2360
> Project: Qpid
> Issue Type: Bug
> Components: Java Broker
> Affects Versions: 0.6
> Reporter: Robbie Gemmell
> Fix For: 0.7
>
> Attachments: qpid-2360-broker-etc.patch, qpid-2360-broker-src.patch,
> qpid-2360-systests-etc.patch, qpid-2360-systests-src.patch,
> qpid-2360-trunk-all.patch
>
>
> Declaring virtualhost level firewall configuration in virtualhosts.xml leads
> to NPE on startup.
> After moving the same configuration into config.xml the broker then loads ok
> and the configuration works as expected.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscr...@qpid.apache.org
