----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4731/#review10012 -----------------------------------------------------------
Ship it! Once the minor issues are addressed, this is ready to go. hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java <https://reviews.apache.org/r/4731/#comment21211> rename -> lastSeqIdBeforeLedgerChange hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java <https://reviews.apache.org/r/4731/#comment21212> rename -> deferredRequests hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java <https://reviews.apache.org/r/4731/#comment21216> The operator here is confusing, without looking up the definition of UNLIMITED_ENTRIES. unlimited applies a lot, so one could assume it's a very high number. use != instead. hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java <https://reviews.apache.org/r/4731/#comment21215> Could you move this calculation to before the if statement. long entriesInThisLedger = (localSeqId - topicInfo.currentLedgerRange.startSeqIdIncluded) + 1; hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java <https://reviews.apache.org/r/4731/#comment21213> This shouldn't happen with the bk client, but if we are assuming it can happen, then we should also assume that it can happen in two different threads. i.e. Use an AtomicBoolean hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java <https://reviews.apache.org/r/4731/#comment21214> Calling this releaseTopicOnFailure implies that a failure has occurred. I think it would be better to call it releaseTopicIfRequested. - Ivan Kelly On Aug. 7, 2012, 3:47 p.m., Sijie Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4731/ > ----------------------------------------------------------- > > (Updated Aug. 7, 2012, 3:47 p.m.) > > > Review request for bookkeeper. > > > Description > ------- > > currently, hub server write entries to only one ledger, if a topic doesn't > change ownership, all entries will be added to this ledger. so those consumed > messages don't have chance to be garbage collected. > > > This addresses bug BOOKKEEPER-191. > https://issues.apache.org/jira/browse/BOOKKEEPER-191 > > > Diffs > ----- > > > hedwig-server/src/main/java/org/apache/hedwig/server/common/ServerConfiguration.java > c8c6610 > > hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java > 873fecd > > hedwig-server/src/test/java/org/apache/hedwig/server/persistence/TestBookkeeperPersistenceManagerWhiteBox.java > eac141c > > Diff: https://reviews.apache.org/r/4731/diff/ > > > Testing > ------- > > > Thanks, > > Sijie Guo > >
