----------------------------------------------------------- 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 > >
