[
https://issues.apache.org/jira/browse/BOOKKEEPER-300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13440315#comment-13440315
]
Sijie Guo commented on BOOKKEEPER-300:
--------------------------------------
thanks Vinay. the patch looks good. I have several minor comments, after that I
think it is ready to be in. please check as below:
{code}
+ public static boolean format(ServerConfiguration conf,
+ boolean isInteractive, boolean force) {
+ File journalDir = conf.getJournalDir();
+ File[] ledgerDirs = conf.getLedgerDirs();
+ for (File dir : ledgerDirs) {
{code}
It is not a good idea to remove journalDir and ledgerDirs directly. since at
most of cases, journalDir and ledgerDirs might be mount points for separated
disks. It would be better to just remove files under these directories.
FileSystemUpgrade is the example you could refer.
{code}
+ // In case of format we will create BK instance later
+ if (!conf.getBoolean("bookkeeper.format", false)) {
+ // Create the BookKeeper client instance
+ bkc = new BookKeeper(conf, zk);
+ this.lfr = new LedgerFragmentReplicator(bkc);
+ }
{code}
I don't think you need such a variable, since you would construct a bookkeeper
instance again. so why not keep it clean, so we don't have two much branch
conditions and additional setting.
{code}
+ } else {
+ zk.create(conf.getZkLedgersRootPath(), "".getBytes(),
+ Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+
+ // Create available path if not exists
+ try {
+ zk.create(conf.getZkAvailableBookiesPath(), "".getBytes(),
+ Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ } catch (KeeperException.NodeExistsException e) {
+ LOG.info("available node already exists.");
+ }
+
{code}
seems that these two lines are doing same thing. why not make it easy. if there
is an old instance, we clean it first, then fall into same path to create it.
{code}
+ // Recreate the BookKeeper Client to recreate the Ledger Layout
+ // information
+ if (null != bkc) {
+ bkc.close();
+ }
+ bkc = new BookKeeper(this.conf, zk);
{code}
I think the ledger layout could be created when calling
LedgerManagerFactory#format. so all ledger manager related meta are handled by
ledger manage factory itself, not scatter the code over files.
BookKeeperAdmin takes the responsibility of removing cookies and instanceid.
{code}
+ HierarchicalLedgerManager ledgerManager = (HierarchicalLedgerManager)
newLedgerManager();
+ String ledgersRootPath = conf.getZkLedgersRootPath();
+ List<String> children = zk.getChildren(ledgersRootPath, false);
+ for (String child : children) {
+ if (ledgerManager.isSpecialZnode(child)) {
+ continue;
+ }
+ ZKUtil.deleteRecursive(zk, ledgersRootPath + "/" + child);
+ }
{code}
You need to delete idgen znode also.
{code}
+ BKAdminCmd() {
+ super(CMD_BKADMIN);
+ bkadminOpts.addOption("r", "recover", true,
+ "Recover the bookie failure. <arg> refers to <bookieSrc>
and [bookieDest], where [bookieDest] is optional");
+ bkadminOpts.addOption("fzk", "formatZk", false,
+ "Format the bookkeeper metadata from zookeeper");
+ }
{code}
since recover and 'formatZk' would be never used together. so why not just let
them be two separated shell commands, one is 'formatZk', the other one is
'recover', which makes the shell clear?
{quote}
+ * Bookie Shell to read/check bookie files and execute admin commands.
{quote}
How about 'Bookie Shell is to provide utilities for users to admin a bookkeeper
cluster.'?
>> tests
It would be better to test an old version cookie to access new formatted
cluster. You could add it in TestBackwardCompat, which uses an old version
bookie.
> Create Bookie format command
> ----------------------------
>
> Key: BOOKKEEPER-300
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-300
> Project: Bookkeeper
> Issue Type: New Feature
> Components: bookkeeper-server
> Affects Versions: 4.1.0
> Reporter: Rakesh R
> Attachments: BOOKKEEPER-300.patch, BOOKKEEPER-300.patch,
> BOOKKEEPER-300.patch
>
>
> Provide a bookie format command. Then the admin would just have to run the
> command on each machine, which will prepare the bookie env
> +Zookeeper paths (znodes):+
> - ledger's root path
> - bookie's available path
> +Directories:+
> - Journal directories
> - Ledger directories
--
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
