[
https://issues.apache.org/jira/browse/BOOKKEEPER-39?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13156870#comment-13156870
]
[email protected] commented on BOOKKEEPER-39:
---------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2817/#review3504
-----------------------------------------------------------
Looking good. There's some cleanup and simplifications to do, but the patch is
close now. I think it may take 1 or 2 more iterations, but this is a big patch,
so that's pretty normal. A general comment is that scope creeped a little in
the last patch with LedgerLayout and the dynamic loading changes. These are
separate patches, and should be in separate JIRAs. Having the content of a
patch focused on one thing makes it much easier to review, especially when the
main part is as big as it is in this one. This is something I did too when I
started sending patches to Apache, but I found quite quickly that the smaller
the changes, the quicker things get review :)
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/conf/bk_server.conf
<https://reviews.apache.org/r/2817/#comment7792>
Versions should be handled by the application, not by the user. We don't
want to user to be able to configure how which version of the layout they use.
They should use the version that the software supports. If their metadata is in
an older version which can't be read by the current version, they need to
upgrade their metadata. We should provide tools to do this if it is required in
future.
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/2817/#comment7793>
typo, relay should be replay
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/2817/#comment7797>
There is only ever one instance of entrylogger, which has one instance of
GarbageCollectorThread. Why is this needed?
I think what you're intend may be here (correct me if im wrong) may be that
you don't want doGcLedger() to be called more than once in parallel.
doGcLedgers and doGcEntryLogs are async calls, so this synchronization method
won't work for that case.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/2817/#comment7800>
While is correct here (when is also correct, but changes the meaning
slightly).
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java
<https://reviews.apache.org/r/2817/#comment7808>
activeLedgerManager should be set to alm before here. The check in the
gcThread for activeLedgerManager isn't very nice. In fact it's not needed. If
you look at the old code, the check is entryLogger.activeLedgers == null, which
can never be the case as activeLedgers is assigned to a new HashMap on
construction.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java
<https://reviews.apache.org/r/2817/#comment7807>
make final
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java
<https://reviews.apache.org/r/2817/#comment7809>
As I said above, this shouldn't be here, it should be in the constructor.
Also, mamanger is a typo
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
<https://reviews.apache.org/r/2817/#comment7810>
Could you move Processor out of the method invokation. I.e. put
Processor<Long> processor = new Processor<Long>() { ... }; and then pass
processor to asyncProcessLedgers.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
<https://reviews.apache.org/r/2817/#comment7811>
Break this line, so it doesn't go past 120 chars. Preferably 80, but if 80
makes it too squashed looking, 120. I think 120 is the limit for the ZK coding
style.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java
<https://reviews.apache.org/r/2817/#comment7818>
Instead of having instanceof here, define newLedgerPath on LedgerManager as:
LedgerManager#newLedgerPath(GenericCallback<String> cb);
Then you should need no instanceof calls.
GenericCallback is defined in proto/BookkeeperInternalCallbacks.java.
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/2817/#comment7812>
I don't think we should provide this level of pluggability yet, until we
finalize what a ledger manager interface looks like, which won't be until we
try to integrate hbase. For the moment, I think it's enough to have
getLedgerManager return a String, which we use with an if-else in the factory.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java
<https://reviews.apache.org/r/2817/#comment7825>
class should be package private
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java
<https://reviews.apache.org/r/2817/#comment7813>
private
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java
<https://reviews.apache.org/r/2817/#comment7814>
private
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java
<https://reviews.apache.org/r/2817/#comment7815>
break line
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java
<https://reviews.apache.org/r/2817/#comment7816>
typo: actived->active
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java
<https://reviews.apache.org/r/2817/#comment7823>
private
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java
<https://reviews.apache.org/r/2817/#comment7822>
Define "0000" and "9999" as String constants at top of class.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java
<https://reviews.apache.org/r/2817/#comment7824>
private
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerLayout.java
<https://reviews.apache.org/r/2817/#comment7817>
This is barely used at the moment and shouldn't be in this patch. It is
something we need to address in the future, but it's better to keep patches
focused on one thing, which in this case is the ledger manager interface.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java
<https://reviews.apache.org/r/2817/#comment7819>
Remove this version LedgerLayout, from the parameters. Instead of the
reflection stuff, just use
if ("flat".equals(conf.getLayoutManager())) {
return new FlatLayoutManager(conf, zk);
} else if ("hierarchical".equals etc etc
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/CallbackUtils.java
<https://reviews.apache.org/r/2817/#comment7820>
These could be moved to BookKeeperInternalCallbacks.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/StringUtils.java
<https://reviews.apache.org/r/2817/#comment7821>
Remove this also, it's unused.
- Ivan
On 2011-11-22 10:39:27, Sijie Guo wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2817/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-11-22 10:39:27)
bq.
bq.
bq. Review request for bookkeeper.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Create a interface LedgerManager to handle 'how to store ledger meta in
zookeeper, how bookie server manages active ledgers and garbage collect those
inactive/deleted ledgers'. Implement this interface using hash mechanism, which
hides detail from client/server code and makes code more clearer.
bq.
bq.
bq. This addresses bug BOOKKEEPER-39.
bq. http://issues.apache.org/jira/browse/BOOKKEEPER-39
bq.
bq.
bq. Diffs
bq. -----
bq.
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/conf/bk_server.conf
1204867
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java
1204867
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
1204867
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java
1204867
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
1204867
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
1204867
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java
1204867
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerDeleteOp.java
1204867
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
1204867
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java
1204867
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
1204867
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerLayout.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/CallbackUtils.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/StringUtils.java
1204867
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ZkUtils.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java
1204867
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java
1204867
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/LedgerLayoutTest.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/hierarchical/HierarchicalAsyncLedgerOpsTest.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/hierarchical/HierarchicalBookieFailureTest.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/hierarchical/HierarchicalBookieReadWriteTest.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/hierarchical/HierarchicalBookieRecoveryTest.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/hierarchical/HierarchicalLedgerDeleteTest.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/doc/bookkeeperConfig.textile
1204867
bq.
bq. Diff: https://reviews.apache.org/r/2817/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Sijie
bq.
bq.
> Bookie server failed to restart because of too many ledgers (more than
> ~50,000 ledgers)
> ---------------------------------------------------------------------------------------
>
> Key: BOOKKEEPER-39
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-39
> Project: Bookkeeper
> Issue Type: Bug
> Components: bookkeeper-server
> Affects Versions: 4.0.0
> Reporter: Sijie Guo
> Assignee: Sijie Guo
> Fix For: 4.0.0
>
> Attachments: bookkeeper-39.patch, bookkeeper-39.patch_v2,
> bookkeeper-39.patch_v3
>
>
> If we have ~500,000 topics in hedwig, we might have more than ~500,000
> ledgers in bookkeeper (a topic has more than 1 ledger). So when the bookie
> server restarted, a logfile GC thread is started, which will call
> zk.getChildren to fetch all ledgers, and it failed because of package length
> limitation.
> 2011-08-01 01:18:46,373 - ERROR
> [main-EventThread:EntryLogger$GarbageCollectorThread$1@164] - Error polling
> ZK for the available ledger nodes:
> org.apache.zookeeper.KeeperException$ConnectionLossException: KeeperErrorCode
> = ConnectionLoss for /ledgers
> at
> org.apache.zookeeper.KeeperException.create(KeeperException.java:99)
> at
> org.apache.zookeeper.KeeperException.create(KeeperException.java:51)
> at org.apache.zookeeper.ZooKeeper.getChildren(ZooKeeper.java:1519)
> at
> org.apache.bookkeeper.bookie.EntryLogger$GarbageCollectorThread$1.processResult(EntryLogger.java:162)
> at
> org.apache.zookeeper.ClientCnxn$EventThread.processEvent(ClientCnxn.java:592)
> at
> org.apache.zookeeper.ClientCnxn$EventThread.run(ClientCnxn.java:481)
> 2011-08-01 01:18:46,373 - WARN [main-EventThread:Bookie$1@242] - ZK client
> has been disconnected to the ZK server!
> 2011-08-01 01:18:47,278 - WARN
> [main-SendThread(perf13.platform.mobile.sp2.yahoo.com:2181):ClientCnxn$SendThread@980]
> - Session 0x131833dec850034 for server
> perf13.platform.mobile.sp2.yahoo.com/98.139.43.86:2181, unexpected error,
> closing socket connection and attempting reconnect
> java.io.IOException: Packet len9976413 is out of range!
> at
> org.apache.zookeeper.ClientCnxnSocket.readLength(ClientCnxnSocket.java:112)
> at
> org.apache.zookeeper.ClientCnxnSocketNIO.doIO(ClientCnxnSocketNIO.java:78)
> at
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:264)
> at
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:958)
--
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
