[jira] [Comment Edited] (QPID-5800) Refactoring: Introduce MessageStoreProvider interface to improve message/configuration store implementations

["Alex Rudyy (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/QPID-5800?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14025175#comment-14025175 ] 

Alex Rudyy edited comment on QPID-5800 at 6/9/14 1:23 PM:
----------------------------------------------------------

Keith,
I reviewed the changes made as part of this JIRA.
Here are my review comments:

DurableConfigurationStore:
Methods openConfigurationStore and closeConfigurationStore can be renamed into open/close accordingly.
The same for MessageStore open and close methods. 

BDBConfigurationStore:
The message store field in BDBConfigurationStore is named "_messageStoreFacade". I would rename that into "_messageStore"

BDBMessageStore is not fully split into BDBConfigurationStore and BDBMessageStore.
IMHO, such methods like getMessageContentDb, getMessageMetaDataDb, getMessageMetaDataSeqDb etc should be moved into BDBMessageStore
Fields like _committer, _messageStoreOpen belongs to the BDBMessageStore.
I am curious how easy it would be to move BDBMessageStore into a separate class?

 
DerbyMessageStore/JDBCMessageStore:
For consistency they can be renamed into ConfigurationStores like DerbyConfigurationStore and JDBCConfigurationStore
MessageStore field is called _messageStoreFacade
MessageStoreWrapper can be moved into AbstractJDBCMessageStore and could be renamed into JDBCMessageStore or something more appropriate in order to move the message store functionality into a separate class later on.


was (Author: alex.rufous):
Keith,
I reviewed the changes made as part of this JIRA.
Here are my review comments:

DurableConfigurationStore:
Methods openConfigurationStore and closeConfigurationStore can be renamed into open/close accordingly.
The same for MessageStore open and close methods. 

BDBConfigurationStore:
The message store field in BDBConfigurationStore is named "_messageStoreFacade". I would rename that into "_messageStore"

BDBMessageStore is not fully split into BDBConfigurationStore and BDBMessageStore.
IMHO, such methods like getMessageContentDb, getMessageMetaDataDb, getMessageMetaDataSeqDb etc should be moved into BDBMessageStore
Fields like _committer, _messageStoreOpen belongs to the BDBMessageStore.
I am curious how easy it would be to move BDBMessageStore into a separate class?

Is there any reason why openSequence code is not called in the try/catch block in order to handle the environment restart exceptions like InsufficientReplicasException? I am guessing that this operation could be replicated?

I see that openSequence is duplicated in both SEF and REF. It looks like we can move the identical code into the abstract parent class.

Also, I am not sure why sequence is called MESSAGE_METADATA_SEQ_KEY instead of MESSAGE_ID_SEQUENCE as the primary purpose of the sequence is to generate message id for the message instance?
 
DerbyMessageStore/JDBCMessageStore:
For consistency they can be renamed into ConfigurationStores like DerbyConfigurationStore and JDBCConfigurationStore
MessageStore field is called _messageStoreFacade
MessageStoreWrapper can be moved into AbstractJDBCMessageStore and could be renamed into JDBCMessageStore or something more appropriate in order to move the message store functionality into a separate class later on.

> Refactoring: Introduce MessageStoreProvider interface to improve message/configuration store implementations
> ------------------------------------------------------------------------------------------------------------
>
>                 Key: QPID-5800
>                 URL: https://issues.apache.org/jira/browse/QPID-5800
>             Project: Qpid
>          Issue Type: Improvement
>          Components: Java Broker
>            Reporter: Keith Wall
>            Assignee: Alex Rudyy
>             Fix For: 0.29
>
>
> A configuration store implementation that wishes to additionally provide a message store must currently implements the {{MessageStore}} interface. This design has led to rather monolithic implementations.
> This refactoring will introduce a {{MesageStoreProvider}} interface.  Configuration stores implementations that wish to also expose a MessageStore will implement this interface.  They can then provide the MessageStore store implementation as they see fit, hopefully leading to better, more cohesive, code.



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