-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5101/#review8653
-----------------------------------------------------------
I don't think it will happen that a user will want a metadata manager of one
type and a topic manager of another. This would make no sense, as the reason
for having a non-zk metadata manager would be that there are too many topics,
for which reason you'd need to use a non-zk topic manager also.
Also, topic owner itself is a type of metadata. We need to be more careful of
updating it since a topic can have only one owner, but I think it fits into the
same category.
I think it would be cleaner to have one interface, MetadataManagerFactory,
which is implemented for ZK and metastore etc. This would look like
MetadataManager does now.
{code}
public interface MetadataManagerFactory {
public TopicPersistenceManager newTopicPersistenceManager();
public SubscriptionDataManager newSubscriptionDataManager();
public TopicOwnershipManager newTopicOwnershipManager();
}
{code}
Any one implementing the pluggable interface would have to implement all three
managers. This will create more work in BOOKKEEPER-259, as the TopicManager
role will probably change to use a TopicOwnershipManager rather than acting in
this role itself. For BOOKKEEPER-250, you should only put in
newTopicPersistenceManager() and newSubscriptionDataManager(). With this
design, the concept of manager type is removed. There's only one type, a
metadata manager. Subsequently, the plugin loading code and layout code can be
a little simpler.
hedwig-protocol/src/main/protobuf/PubSubProtocol.proto
<https://reviews.apache.org/r/5101/#comment18312>
would be better to use METADATA rather than META.
A META manager would be a manager which manages managers. A METADATA
manager is a manager which managed metadata.
hedwig-protocol/src/main/protobuf/PubSubProtocol.proto
<https://reviews.apache.org/r/5101/#comment18314>
Versions should be specified and handled in ManagerFactory. ManagerFactory
should have a defined current version, and a min compat version, which will be
the same as the current version for now.
hedwig-protocol/src/main/protobuf/PubSubProtocol.proto
<https://reviews.apache.org/r/5101/#comment18311>
I wouldn't put a default for the version.
hedwig-protocol/src/main/protobuf/PubSubProtocol.proto
<https://reviews.apache.org/r/5101/#comment18313>
I think it'd be better to call this managerImpl or managerImplementation or
even managerImplementationClass.
hedwig-server/src/main/java/org/apache/hedwig/server/managers/Manager.java
<https://reviews.apache.org/r/5101/#comment18315>
I think instead of having a second phase init like this, specify in the
javadoc that for manager to be loaded with the plugin mechanism it must have a
constructor with signature
(ServerConfiguration cfg, ZooKeeper zk, int version).
Object... can be removed. For the ManagerFactory to pass in Object... it
would need to know something specific about that manager which defeats the
purpose of the pluggability.
hedwig-server/src/main/java/org/apache/hedwig/server/managers/ManagerLayoutUtils.java
<https://reviews.apache.org/r/5101/#comment18310>
It would be better to use TextFormat here, so that an admin can check the
info by eye. TextFormat can serialize and deserialize.
- Ivan Kelly
On June 20, 2012, 2:39 p.m., Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5101/
> -----------------------------------------------------------
>
> (Updated June 20, 2012, 2:39 p.m.)
>
>
> Review request for bookkeeper.
>
>
> Description
> -------
>
> it would be better to a ledger-manager like interface to manage metadata
> operations in Hedwig, which might be easy for use to adapt to meta store api.
>
>
> This addresses bug BOOKKEEPER-250.
> https://issues.apache.org/jira/browse/BOOKKEEPER-250
>
>
> Diffs
> -----
>
>
> hedwig-protocol/src/main/java/org/apache/hedwig/exceptions/PubSubException.java
> 5cbd318
>
> hedwig-protocol/src/main/java/org/apache/hedwig/protocol/PubSubProtocol.java
> 92f409d
> hedwig-protocol/src/main/protobuf/PubSubProtocol.proto b845c40
>
> hedwig-server/src/main/java/org/apache/hedwig/server/common/ServerConfiguration.java
> a37e336
> hedwig-server/src/main/java/org/apache/hedwig/server/managers/Manager.java
> PRE-CREATION
>
> hedwig-server/src/main/java/org/apache/hedwig/server/managers/ManagerFactory.java
> PRE-CREATION
>
> hedwig-server/src/main/java/org/apache/hedwig/server/managers/ManagerLayoutUtils.java
> PRE-CREATION
>
> hedwig-server/src/main/java/org/apache/hedwig/server/meta/MetadataManager.java
> PRE-CREATION
>
> hedwig-server/src/main/java/org/apache/hedwig/server/meta/SubscriptionDataManager.java
> PRE-CREATION
>
> hedwig-server/src/main/java/org/apache/hedwig/server/meta/TopicPersistenceManager.java
> PRE-CREATION
>
> hedwig-server/src/main/java/org/apache/hedwig/server/meta/ZkMetadataManager.java
> PRE-CREATION
>
> hedwig-server/src/main/java/org/apache/hedwig/server/netty/PubSubServer.java
> cb11955
>
> hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java
> c8a936a
>
> hedwig-server/src/main/java/org/apache/hedwig/server/persistence/LocalDBPersistenceManager.java
> a8b9f74
>
> hedwig-server/src/main/java/org/apache/hedwig/server/persistence/PersistenceManager.java
> 0ee0a9b
>
> hedwig-server/src/main/java/org/apache/hedwig/server/subscriptions/MMSubscriptionManager.java
> PRE-CREATION
>
> hedwig-server/src/main/java/org/apache/hedwig/server/subscriptions/ZkSubscriptionManager.java
> 4d39335
> hedwig-server/src/main/java/org/apache/hedwig/zookeeper/ZkUtils.java
> c377e04
>
> hedwig-server/src/test/java/org/apache/hedwig/server/managers/TestManagerFactory.java
> PRE-CREATION
>
> hedwig-server/src/test/java/org/apache/hedwig/server/managers/TestManagerLayout.java
> PRE-CREATION
>
> hedwig-server/src/test/java/org/apache/hedwig/server/meta/MetadataManagerTestCase.java
> PRE-CREATION
>
> hedwig-server/src/test/java/org/apache/hedwig/server/meta/TestMetadataManager.java
> PRE-CREATION
>
> hedwig-server/src/test/java/org/apache/hedwig/server/persistence/StubPersistenceManager.java
> fecf861
>
> hedwig-server/src/test/java/org/apache/hedwig/server/persistence/TestBookKeeperPersistenceManagerBlackBox.java
> 57bc95d
>
> hedwig-server/src/test/java/org/apache/hedwig/server/persistence/TestBookkeeperPersistenceManagerWhiteBox.java
> 216cd8a
>
> hedwig-server/src/test/java/org/apache/hedwig/server/subscriptions/TestMMSubscriptionManager.java
> PRE-CREATION
>
> hedwig-server/src/test/java/org/apache/hedwig/server/subscriptions/TestZkSubscriptionManager.java
> 9558acf
>
> Diff: https://reviews.apache.org/r/5101/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sijie Guo
>
>
