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/03/09 07:38:44 UTC
Re: Review Request: BOOKKEEPER-163 Prevent incorrect NoSuchLedgerException
for readLastConfirmed.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3926/#review5766
-----------------------------------------------------------
most of the patch is good beside some small issues. I commented them inline.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Cookie.java
<https://reviews.apache.org/r/3926/#comment12563>
if more than two bookie server try to create COOKIE_PATH at the same time, it would fail. it would be better to catch NodeExists Exception.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileSystemUpgrade.java
<https://reviews.apache.org/r/3926/#comment12565>
it seems that you missed that file 'lastId' and 'lastMark'
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileSystemUpgrade.java
<https://reviews.apache.org/r/3926/#comment12564>
it would be better to copy data to a temp directory, such as upgrading, then rename the upgrading directory to current.
otherwise, if the upgrading is broken after created current directory, but user restarts bookie server with this upgrade-failed directory, bookie server would treat it as a new environment.
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CookieTest.java
<https://reviews.apache.org/r/3926/#comment12566>
for a new disk (new directory) added case, it would not cause data missing.
could we provide a tool to upgrade a bookie's cookie when adding a new directory?
- Sijie
On 2012-02-16 16:15:58, Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3926/
> -----------------------------------------------------------
>
> (Updated 2012-02-16 16:15:58)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> bookkeeper client treats NoSuchLedgerException as valid response when reading last confirmed. If NoSuchLedgerException is caused due to an empty directory in following cases, it is an incorrect response.
>
> 1) A disk is replaced or ledger index is removed by a sloppy admin.
> 2) A disk is not mounted when a bookie machine is restarted.
>
> We need a mechanism to prevent such incorrect responses.
>
> Ivan suggested to generate a instance key for each bookie and write it into the ledger directories. If a directory doesn't have the key, and other directories do, then it shouldn't start. This would also resolve the issue that someone starting a new bookie with the same IP as a bookie which has previously died.
>
>
> This addresses bug BOOKKEEPER-163.
> https://issues.apache.org/jira/browse/BOOKKEEPER-163
>
>
> Diffs
> -----
>
> bookkeeper-server/pom.xml 601104f
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 2fb7c6c
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieException.java 1a5b313
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Cookie.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java aca66e6
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileSystemUpgrade.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java 3e96d46
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java 2df0ed0
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieLayoutVersionTest.java 10f9538
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CookieTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java f661e90
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/UpgradeTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 99258ac
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestFencing.java 015e4e4
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java dada67a
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieFailureTest.java ea51118
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieZKExpireTest.java 5ed7061
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/ZooKeeperUtil.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/3926/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ivan
>
>
Re: Review Request: BOOKKEEPER-163 Prevent incorrect NoSuchLedgerException
for readLastConfirmed.
Posted by Ivan Kelly <iv...@apache.org>.
> On 2012-03-09 06:38:44, Sijie Guo wrote:
> > bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CookieTest.java, line 151
> > <https://reviews.apache.org/r/3926/diff/1/?file=75432#file75432line151>
> >
> > for a new disk (new directory) added case, it would not cause data missing.
> >
> > could we provide a tool to upgrade a bookie's cookie when adding a new directory?
I did have this in the back of my mind when writing this, but it seems an extra feature to me and would bloat the scope a bit much. Could we put this in an new JIRA?
> On 2012-03-09 06:38:44, Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileSystemUpgrade.java, line 173
> > <https://reviews.apache.org/r/3926/diff/1/?file=75428#file75428line173>
> >
> > it would be better to copy data to a temp directory, such as upgrading, then rename the upgrading directory to current.
> >
> > otherwise, if the upgrading is broken after created current directory, but user restarts bookie server with this upgrade-failed directory, bookie server would treat it as a new environment.
Very good point, will do this.
> On 2012-03-09 06:38:44, Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileSystemUpgrade.java, line 63
> > <https://reviews.apache.org/r/3926/diff/1/?file=75428#file75428line63>
> >
> > it seems that you missed that file 'lastId' and 'lastMark'
Ah, well spotted, will add them.
> On 2012-03-09 06:38:44, Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Cookie.java, line 135
> > <https://reviews.apache.org/r/3926/diff/1/?file=75426#file75426line135>
> >
> > if more than two bookie server try to create COOKIE_PATH at the same time, it would fail. it would be better to catch NodeExists Exception.
Fixed
- Ivan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3926/#review5766
-----------------------------------------------------------
On 2012-02-16 16:15:58, Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3926/
> -----------------------------------------------------------
>
> (Updated 2012-02-16 16:15:58)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> bookkeeper client treats NoSuchLedgerException as valid response when reading last confirmed. If NoSuchLedgerException is caused due to an empty directory in following cases, it is an incorrect response.
>
> 1) A disk is replaced or ledger index is removed by a sloppy admin.
> 2) A disk is not mounted when a bookie machine is restarted.
>
> We need a mechanism to prevent such incorrect responses.
>
> Ivan suggested to generate a instance key for each bookie and write it into the ledger directories. If a directory doesn't have the key, and other directories do, then it shouldn't start. This would also resolve the issue that someone starting a new bookie with the same IP as a bookie which has previously died.
>
>
> This addresses bug BOOKKEEPER-163.
> https://issues.apache.org/jira/browse/BOOKKEEPER-163
>
>
> Diffs
> -----
>
> bookkeeper-server/pom.xml 601104f
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 2fb7c6c
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieException.java 1a5b313
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Cookie.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java aca66e6
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileSystemUpgrade.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java 3e96d46
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java 2df0ed0
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieLayoutVersionTest.java 10f9538
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CookieTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java f661e90
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/UpgradeTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 99258ac
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestFencing.java 015e4e4
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java dada67a
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieFailureTest.java ea51118
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieZKExpireTest.java 5ed7061
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/ZooKeeperUtil.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/3926/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ivan
>
>