You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org> on 2012/03/09 07:40:00 UTC

[jira] [Commented] (BOOKKEEPER-163) Prevent incorrect NoSuchLedgerException for readLastConfirmed.

    [ https://issues.apache.org/jira/browse/BOOKKEEPER-163?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13225885#comment-13225885 ] 

jiraposter@reviews.apache.org commented on BOOKKEEPER-163:
----------------------------------------------------------


-----------------------------------------------------------
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:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/3926/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2012-02-16 16:15:58)
bq.  
bq.  
bq.  Review request for bookkeeper.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  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.
bq.  
bq.  1) A disk is replaced or ledger index is removed by a sloppy admin.
bq.  2) A disk is not mounted when a bookie machine is restarted.
bq.  
bq.  We need a mechanism to prevent such incorrect responses.
bq.  
bq.  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.
bq.  
bq.  
bq.  This addresses bug BOOKKEEPER-163.
bq.      https://issues.apache.org/jira/browse/BOOKKEEPER-163
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    bookkeeper-server/pom.xml 601104f 
bq.    bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 2fb7c6c 
bq.    bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieException.java 1a5b313 
bq.    bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Cookie.java PRE-CREATION 
bq.    bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java aca66e6 
bq.    bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileSystemUpgrade.java PRE-CREATION 
bq.    bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java 3e96d46 
bq.    bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java 2df0ed0 
bq.    bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieLayoutVersionTest.java 10f9538 
bq.    bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CookieTest.java PRE-CREATION 
bq.    bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java f661e90 
bq.    bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/UpgradeTest.java PRE-CREATION 
bq.    bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 99258ac 
bq.    bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestFencing.java 015e4e4 
bq.    bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java dada67a 
bq.    bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieFailureTest.java ea51118 
bq.    bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieZKExpireTest.java 5ed7061 
bq.    bookkeeper-server/src/test/java/org/apache/bookkeeper/test/ZooKeeperUtil.java PRE-CREATION 
bq.  
bq.  Diff: https://reviews.apache.org/r/3926/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Ivan
bq.  
bq.


                
> Prevent incorrect NoSuchLedgerException for readLastConfirmed.
> --------------------------------------------------------------
>
>                 Key: BOOKKEEPER-163
>                 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-163
>             Project: Bookkeeper
>          Issue Type: Bug
>          Components: bookkeeper-client, bookkeeper-server
>    Affects Versions: 4.0.0
>            Reporter: Sijie Guo
>            Assignee: Ivan Kelly
>             Fix For: 4.1.0
>
>         Attachments: BOOKKEEPER-163.diff
>
>
> 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 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