You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Sijie Guo <gu...@gmail.com> on 2012/05/31 11:57:47 UTC
Re: Review Request: BOOKKEEPER-191: Hub server should change ledger to write,
so consumed messages have chance to be garbage collected.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4731/
-----------------------------------------------------------
(Updated 2012-05-31 09:57:47.482007)
Review request for bookkeeper.
Changes
-------
bring the patch to latest trunk
Summary
-------
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 (updated)
-----
hedwig-server/src/main/java/org/apache/hedwig/server/common/ServerConfiguration.java a37e336
hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java c8a936a
hedwig-server/src/test/java/org/apache/hedwig/server/persistence/TestBookkeeperPersistenceManagerWhiteBox.java 216cd8a
Diff: https://reviews.apache.org/r/4731/diff
Testing
-------
Thanks,
Sijie
Re: Review Request: BOOKKEEPER-191: Hub server should change ledger to write,
so consumed messages have chance to be garbage collected.
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request: BOOKKEEPER-191: Hub server should change ledger to write,
so consumed messages have chance to be garbage collected.
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4731/
-----------------------------------------------------------
(Updated Aug. 9, 2012, 9:14 a.m.)
Review request for bookkeeper.
Changes
-------
update the patch to address Ivan's comments.
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 (updated)
-----
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
Re: Review Request: BOOKKEEPER-191: Hub server should change ledger to write,
so consumed messages have chance to be garbage collected.
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
update the patch to latest trunk.
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 (updated)
-----
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
Re: Review Request: BOOKKEEPER-191: Hub server should change ledger to write,
so consumed messages have chance to be garbage collected.
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4731/#review8294
-----------------------------------------------------------
hedwig-server/src/main/java/org/apache/hedwig/server/common/ServerConfiguration.java
<https://reviews.apache.org/r/4731/#comment17911>
I would set the default to something like 100000 or so. Setting to 0L disabled the code. This is a useful feature, so i think it's better to enable by default.
hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java
<https://reviews.apache.org/r/4731/#comment17912>
typo, _indicates_
hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java
<https://reviews.apache.org/r/4731/#comment17915>
doChangeLedger -> changingLedger
hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java
<https://reviews.apache.org/r/4731/#comment17914>
better to call this pendingRequests.
hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java
<https://reviews.apache.org/r/4731/#comment17913>
Use atomic boolean and checkandset in callback. Two callbacks shouldn't run at the same time, but it's not guaranteed.
hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java
<https://reviews.apache.org/r/4731/#comment17916>
Im unsure about the thread safety of this operation. You set doChangeLedger to false, and then run all the queued requests. However, there is space for the TopicOp thread for this topic to start persisting messages out of order inbetween. Remember, this code runs in the bookkeeper callback thread or the zookeeper callback thread. It's not actually clear.
- Ivan
On 2012-05-31 09:57:47, Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4731/
> -----------------------------------------------------------
>
> (Updated 2012-05-31 09:57:47)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> 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 a37e336
> hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java c8a936a
> hedwig-server/src/test/java/org/apache/hedwig/server/persistence/TestBookkeeperPersistenceManagerWhiteBox.java 216cd8a
>
> Diff: https://reviews.apache.org/r/4731/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sijie
>
>