[
https://issues.apache.org/jira/browse/QPID-5615?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13981205#comment-13981205
]
Alex Rudyy commented on QPID-5615:
----------------------------------
Rob,
Here are my review comments:
Potential issues:
* In org.apache.qpid.server.model.AbstractConfiguredObject.getAttribute(String)
an evaluation of automated or derived attribute value might cause a stack
overflow if an Exception is thrown from the getter method in
ConfiguredObjectAttributeOrStatistic.getValue(X). Which calls back
AbstractConfiguredObject.getAttribute(String) on exception in
getGetter().invoke(configuredObject).
* If validation fails in any of configured object, it looks like in Management
mode we will not be able to start the broker anymore....
* Port implementations perhaps need to have a code restricting protocol changes
to only supported protocols. For instance, changing of port protocols to AMQP
protocols on HTTP port should not be allowed. It seems like it is allowed now
via REST api. I am not sure if it is an existing issue.
* org.apache.qpid.server.model.BrokerModel.getInstance() IMHO, we can stop
using singleton pattern for this. In broker code it called only once on the
broker start-up in org.apache.qpid.server.Broker.startupImpl(BrokerOptions)
Dead code:
*
org.apache.qpid.server.security.auth.manager.AbstractAuthenticationManager.instantiatePreferencesProvider(PreferencesProvider)
*
org.apache.qpid.server.model.adapter.FileSystemPreferencesProviderImpl.openNewStore()
*
org.apache.qpid.server.model.AbstractConfiguredObject.generateEffectiveAttributes(Map<String,
Object>)
*
org.apache.qpid.server.model.AbstractConfiguredObject.AbstractConfiguredObject(Map<Class<?
extends ConfiguredObject>, ConfiguredObject<?>>, Map<String, Object>)
* org.apache.qpid.server.stats.StatisticsGatherer.Source
* org.apache.qpid.server.model.adapter.BrokerAdapter.getStatisticsGatherer()
*
org.apache.qpid.server.model.ConfiguredObject.getAttribute(ConfiguredObjectAttribute<?
super X, T>)
Various minor issues:
*
org.apache.qpid.server.security.auth.manager.AbstractAuthenticationManagerFactory<X>
looks like redundant as it does not contain any code. Thus, auth provider
factories can extend AbstractConfiguredObjectTypeFactory directly.
*
org.apache.qpid.server.model.adapter.FileSystemPreferencesProviderFactory.createInstance(Map<String,
Object>, ConfiguredObject<?>...) contains redundant code for UUID
UUID id = idObj == null ? UUID.randomUUID() : idObj instanceof UUID ? (UUID)
idObj : UUID.fromString(idObj.toString());
* public modifier in method
org.apache.qpid.server.model.adapter.FileSystemPreferencesProviderImpl.createStoreIfNotExist()
It should be private
* org.apache.qpid.server.model.ConfiguredObject.getType() should it have a
derived annotation instead of automate? It does not look that we support type
change operation yet. Thus, user cannot change type through the management
interfaces.Also, it might be useful to add type into actual attributes if it is
not there.
*
org.apache.qpid.server.management.plugin.servlet.rest.action.ListAccessControlProviderAttributes.perform(Map<String,
Object>, Broker) contains the commented code with TODO RG - fix.
Not essential issues in a temporary code:
*
org.apache.qpid.server.configuration.store.ManagementModeStoreHandler.quiesceEntries(BrokerOptions)
looks like a redundant method. The same logic is executed in
org.apache.qpid.server.configuration.store.ManagementModeStoreHandler.openConfigurationStore(ConfiguredObject<?>,
Map<String, Object>)
* org.apache.qpid.server.configuration.store.MemoryConfigurationEntryStore and
org.apache.qpid.server.configuration.store.JsonConfigurationEntryStore It seems
that they slightly violates the contract of DurableConfigurationStore by
performing opening logic inside of a constructor rather then in
openConfigurationStore method
> [Java Broker] Broker and VirtualHost should use the same API for
> configuration stores
> -------------------------------------------------------------------------------------
>
> Key: QPID-5615
> URL: https://issues.apache.org/jira/browse/QPID-5615
> Project: Qpid
> Issue Type: Sub-task
> Components: Java Broker
> Reporter: Rob Godfrey
> Assignee: Rob Godfrey
>
> Currently there are two different interfaces for the persisting of configured
> object, one which is used by objects that live directly under the broker, and
> one which is used by objects underneath the virtual host.
> The two APIs should be unified, the recovery process should me made generic,
> and a standardised way of differentiating between objects which inherit their
> parents store and those which manage their own should be made (Broker and
> VirtualHost should not be special-cased in any store logic).
--
This message was sent by Atlassian JIRA
(v6.2#6252)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
