qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Rudyy (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (QPID-5615) [Java Broker] Broker and VirtualHost should use the same API for configuration stores
Date Fri, 25 Apr 2014 16:18:15 GMT

    [ 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: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org


Mime
View raw message