[
https://issues.apache.org/jira/browse/BOOKKEEPER-108?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13149603#comment-13149603
]
[email protected] commented on BOOKKEEPER-108:
----------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2771/#review3204
-----------------------------------------------------------
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/conf/bk_client.conf
<https://reviews.apache.org/r/2771/#comment7101>
I don't think we should provide a config file for clients. Clients will use
bookkeeper as a library, so they'll want to integrate bookkeeper configuration
into the overall application configuration. We should document the
configuration options in ClientConfiguration.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java
<https://reviews.apache.org/r/2771/#comment7102>
Make syncThread final. Move it up to where the other members are also.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerEntryPage.java
<https://reviews.apache.org/r/2771/#comment7103>
make final
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
<https://reviews.apache.org/r/2771/#comment7104>
You seem to have added a few extra constructors here. The constructors I
think we should have are:
1) BookKeeper(String zkServers);
2) BookKeeper(ClientConfiguration conf);
3) BookKeeper(ClientConfiguration conf, ZooKeeper zk);
4) BookKeeper(ClientConfiguration conf, ZooKeeper zk,
ClientSocketChannelFactory channelFactory);
I'd like to get rid of 1) also, but this should be put to Ben and Flavio
also.
It could be replaced with
new BookKeeper(new ClientConfiguration().setZkServers("localhost:2181"))
This is wordier, but I prefer to have fewer constructors. One thing which I
think is absolutely necessary, is that all but one of the constructors do very
little. For example,
1) should create a conf with zkservers set to zkServers and then call 2)
2) should create the zk instance, and call 3) with (conf, zk)
3) should create the channelFactory and call 4) with (conf, zk, factory)
This will ensure that the code in constructors doesn't rot.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
<https://reviews.apache.org/r/2771/#comment7105>
Make final
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
<https://reviews.apache.org/r/2771/#comment7106>
This class should inherit from
org.apache.commons.configuration.AbstractConfiguration to pull in a load of
utility methods. For example, if an application using BookKeeper has their own
configuration class, EtcAppConfiguration, with the bookkeeper keys in it, it
could instantiate BookKeeper with
ClientConfiguration bkconf = new ClientConfiguration();
bkconf.append(etcAppConfiguration);
BookKeeper bk = new BookKeeper(bkconf);
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
<https://reviews.apache.org/r/2771/#comment7118>
Server specific, shouldn't be in abstract class.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java
<https://reviews.apache.org/r/2771/#comment7119>
These are only used by BookKeeperAdmin so should be named differently so
that users dont think they'll be used in all calls to BookKeeper. I think these
should be.
setBookieRecoveryDigestType() & setBookieRecoveryPasswd()
and similar for get().
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
<https://reviews.apache.org/r/2771/#comment7117>
Why is this overridden? Couldn't ZkServers be set to null with
setZkServers(). Which test requires this?
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClient.java
<https://reviews.apache.org/r/2771/#comment7107>
Who instantiates this without configuration? I dont think this constructor
should exist.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
<https://reviews.apache.org/r/2771/#comment7108>
final
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
<https://reviews.apache.org/r/2771/#comment7109>
final
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
<https://reviews.apache.org/r/2771/#comment7111>
I think this logic would be better in main() as follows.
ServerConfiguration conf = null;
try {
conf = parseArgs(args);
} catch (IllegalArgumentException iae) {
LOG.error("Error parsing command line arguments");
System.err.println(iae.getMessage());
printUsage();
throw iae;
}
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
<https://reviews.apache.org/r/2771/#comment7112>
Should throw IllegalArgumentException
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
<https://reviews.apache.org/r/2771/#comment7110>
Instead of returning null on error throw IllegalArgumentException.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
<https://reviews.apache.org/r/2771/#comment7113>
move printUsage to main(). Throw IllegalArgumentException here.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
<https://reviews.apache.org/r/2771/#comment7114>
move printUsage to main, throw IllegalArgEx here.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
<https://reviews.apache.org/r/2771/#comment7115>
throws IllegalArgEx
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/NIOServerFactory.java
<https://reviews.apache.org/r/2771/#comment7116>
Why is ClientConfiguration imported?
- Ivan
On 2011-11-11 17:29:32, Sijie Guo wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2771/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-11-11 17:29:32)
bq.
bq.
bq. Review request for bookkeeper.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. manage all server-side settings in a Configuration object
bq.
bq.
bq. This addresses bug BOOKKEEPER-108.
bq. http://issues.apache.org/jira/browse/BOOKKEEPER-108
bq.
bq.
bq. Diffs
bq. -----
bq.
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/bin/bookkeeper
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/conf/bk_client.conf
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/conf/bk_server.conf
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/conf/bkenv.sh
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerEntryPage.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClient.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/NIOServerFactory.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/LocalBookKeeper.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperTestClient.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/LedgerCacheTest.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieClientTest.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieFailureTest.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieJournalRollingTest.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieReadWriteTest.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieRecoveryTest.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieZKExpireTest.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/ConcurrentLedgerTest.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/ConditionalSetTest.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/ConfigurationTest.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/LedgerDeleteTest.java
1200945
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/NIOServerFactoryTest.java
1200945
bq.
bq. Diff: https://reviews.apache.org/r/2771/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Sijie
bq.
bq.
> add configuration support for BK
> --------------------------------
>
> Key: BOOKKEEPER-108
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-108
> Project: Bookkeeper
> Issue Type: Improvement
> Affects Versions: 4.0.0
> Reporter: Sijie Guo
> Assignee: Sijie Guo
> Fix For: 4.0.0
>
> Attachments: BOOKKEEPER-108.patch_v2, bookkeeper-108.patch
>
>
> As Ivan's comment on BOOKKEEPER-39, we use lots of system properties in BK
> now. It's better to use a proper configuration object to manager them.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
