[
https://issues.apache.org/jira/browse/BOOKKEEPER-300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13437896#comment-13437896
]
Sijie Guo commented on BOOKKEEPER-300:
--------------------------------------
@Vinay, thanks for the patch. please check my comments as below:
{quote}
+ && (c.layoutVersion == 3 || (instanceId != null && c.instanceId
+ .equals(instanceId))))) {
{quote}
for layoutVersion 3, should not have instanceId. if there is instanceId, it
would needs to throw InvalidCookieException.
And you don't check c.instanceId is null or not before calling equals. you'd
better change it to 'instanceId.equals(c.instanceId)'
For layoutVersion 4, should we fail it if there is no instanceId?
If an invalid cookie contains a different instance id, it would provide more
detail information in the InvalidCookieException.
so users could know how to do it when encountering it.
BTW, it would be better to provide test cases in CookieTest for different
layout versions and compatibility.
{quote}
+ private static void loadConfFile(AbstractConfiguration conf, String
confFile)
+ throws IllegalArgumentException {
+ try {
+ conf.loadConf(new File(confFile).toURI().toURL());
+ } catch (MalformedURLException e) {
+ LOG.error("Could not open configuration file: " + confFile, e);
+ throw new IllegalArgumentException();
+ } catch (ConfigurationException e) {
+ LOG.error("Malformed configuration file: " + confFile, e);
+ throw new IllegalArgumentException();
+ }
+ LOG.info("Using configuration file " + confFile);
+ }
{quote}
the indent is not right.
{quote}
+ public boolean format() {
+ final AtomicInteger successResults = new AtomicInteger();
+ try {
+ Set<Long> zkLedgers = getLedgersInSingleNode(ledgerRootPath);
{quote}
the format logic doesn't work correctly for HierarchicalLedgerManager. actually
you could use the asyncProcessLedgers to delete the ledgers. For
HierarchicalLedgerManagers, you also need to delete hierarchical znodes.
but I am doubting why not use ZKUtils.deleteRecusive?
{quote}
+ boolean ledgersFormatted = bkc.ledgerManager.format();
+ if (!ledgersFormatted) {
+ LOG.error("Error in formatting ledgers");
+ return false;
+ }
+ bkc.ledgerManagerFactory.format(conf, zk);
{quote}
I think it should only have one format method for per ledger manager factory.
the format method takes the responsibility of removing ledgers metadata (not
only ledgers but also znode for id generation and hierarchical znodes) and its
layout data. The format is not only delete old ledger metadatas, but it also
needs to create necessary znodes for new system. you missed it in this patch.
{quote}
+ public static void main(String[] args) throws IOException,
+ InterruptedException, KeeperException {
+ AbstractConfiguration conf = null;
+ try {
+ conf = parseArgs(args);
+ } catch (ParseException e) {
+ LOG.error("Error parsing command line arguments : ", e);
+ System.err.println(e.getMessage());
+ printUsage();
+ System.exit(ExitCode.INVALID_CONF);
+ }
{quote}
Actually there is a tool class BookKeeperTools in util package for bookie
recovery which also uses BKAdmin. I am guessing it would be better to leverage
it to support different admin commands like what BookieShell does for bookie
side. BTW, for Bookie format, it would better to put it in BookieShell to put
all bookie commands together.
> 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
>
>
> 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
