I think we have to deal with protocol type and check the directory consistencies based on that. Currently, confirmation check will check only namespace dirs.
To check the shared edits, we can not use this logic. We have to do it depending on the shared journal type. for: bk jouranal..etc

Also currently initialization of shareEditsDirs option also assumes that as file protocol. If we configure any other type it may not work.

Uma Maheswara Rao G
added a comment - 17/Apr/12 15:56 I think we have to deal with protocol type and check the directory consistencies based on that. Currently, confirmation check will check only namespace dirs.
To check the shared edits, we can not use this logic. We have to do it depending on the shared journal type. for: bk jouranal..etc
Also currently initialization of shareEditsDirs option also assumes that as file protocol. If we configure any other type it may not work.

Currently I have created a patch which work with shared dir configured with file protocol.
If any bookeeper related directory is configured then my patch will not fail the format.

for(Iterator<URI> it = dirsToFormat.iterator(); it.hasNext();) {
File curDir = new File(it.next().getPath());
// Its alright for a dir not to exist, or to exist (properly accessible)
// and be completely empty.
if (!curDir.exists() ||
(curDir.isDirectory() && FileUtil.listFiles(curDir).length == 0))
continue;

curDir.exist() (which will check locally and return false) and user is not prompted for formatting this shared dir.

I have another doubt If I format the HDFS cluster which used Bookeeper for shared storage, then ./hdfs namenode -format will not format the shared dir(bookeeper dir). Then how cluster works with older version details?

amith
added a comment - 17/Apr/12 16:23 I agree with uma.
Currently I have created a patch which work with shared dir configured with file protocol.
If any bookeeper related directory is configured then my patch will not fail the format.
for (Iterator<URI> it = dirsToFormat.iterator(); it.hasNext();) {
File curDir = new File(it.next().getPath());
// Its alright for a dir not to exist, or to exist (properly accessible)
// and be completely empty.
if (!curDir.exists() ||
(curDir.isDirectory() && FileUtil.listFiles(curDir).length == 0))
continue ;
curDir.exist() (which will check locally and return false) and user is not prompted for formatting this shared dir.
I have another doubt If I format the HDFS cluster which used Bookeeper for shared storage, then ./hdfs namenode -format will not format the shared dir(bookeeper dir). Then how cluster works with older version details?

I think that for this JIRA we should punt on the other types of shared dirs besides file-based. I think we should make format look at the journal type and print something like "not formatting non-file journal manager..."

How does that sound? At a later point in a different JIRA we can work on a more general initialization system which is totally agnostic to the type of journal manager.

Aaron T. Myers
added a comment - 17/Apr/12 18:14 I think that for this JIRA we should punt on the other types of shared dirs besides file-based. I think we should make format look at the journal type and print something like "not formatting non-file journal manager..."
How does that sound? At a later point in a different JIRA we can work on a more general initialization system which is totally agnostic to the type of journal manager.

Uma Maheswara Rao G
added a comment - 17/Apr/12 18:32
How does that sound? At a later point in a different JIRA we can work on a more general initialization system which is totally agnostic to the type of journal manager.
Sounds good to me. +1
Here is the JIRA to support shared edits dirs(other than file based). HDFS-3287
@Amith, you can go ahead with this change as a limitation of non-file based shared dirs.

Hadoop QA
added a comment - 18/Apr/12 19:42 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12523215/HDFS-3275.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 1 new or modified test files.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in .
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2297//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2297//console
This message is automatically generated.

Uma Maheswara Rao G
added a comment - 18/Apr/12 20:50 Amith, thanks a lot for working on this issue.
I just reviewed your patch! Some comments:
1) File base_dir = new File(System.getProperty("test.build.data",
+ "build/test/data"), "dfs/");
can't we use getBaseDirectory from minidfs cluster?
2)
NameNode.format(conf); // Namenode should not format dummy or any other
+ // non file schemes
instead of wrapping the comment into two lines, can we add it above to the foamt call?
3)
System .err
+ .println( "Storage directory "
+ + dirUri
+ + " is not in file scheme currently formatting is not supported for this scheme" );
can you please format this correctly?
ex:
System .err.println( "Storage directory "
+ " is not in file scheme currently "
+ "formatting is not supported for this scheme" );
4) File curDir = new File(dirUri.getPath());
File will take uri also, so need not cnvert it to string right?
5) Also message can be like, 'Formatting supported only for file based storage directories. Current directory scheme is " scheme " . So, ignoring it for format"
6) HATestUtil#setFailoverConfigurations would do almost similar setup as in test. is it possible to use it by passing mock cluster or slightly changed HATestUtil#setFailoverConfigurations?
7)you mean "Could not delete hdfs directory '" -> "Could not delete namespace directory '"
8) testOnlyFileSchemeDirsAreFormatted -> testFormatShouldBeIgnoredForNonFileBasedDirs ?

Uma Maheswara Rao G
added a comment - 24/Apr/12 03:43 Patch looks good. Assert has been added in format api. So, test ensures that there is no exceptions out of it when we include non-file based journals.
+1
Re-attaching the same patch as Amith to trigger Jenkins.

Hadoop QA
added a comment - 24/Apr/12 05:13 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12523911/HDFS-3275_1.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 2 new or modified test files.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in .
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2316//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2316//console
This message is automatically generated.

Aaron T. Myers
added a comment - 24/Apr/12 06:49 Patch looks pretty good to me. Just a few little comments. +1 once these are addressed:
Don't declare the "DEFAULT_SCHEME" constant in the NameNode class. Instead, use the NNStorage.LOCAL_URI_SCHEME constant, which is used in FSEditLog to identify local edits logs.
I think it's better to include the URI of the dir we're skipping, and the scheme we expect. So, instead of this:
System .err.println( "Formatting supported only for file based storage"
+ " directories. Current directory scheme is \" "
+ dirUri.getScheme() + "\" . So, ignoring it for format");
How about something like this:
System .err.println( "Skipping format for directory \" " + dirUri
+ "\" . Can only format local directories with scheme \""
+ NNStorage.LOCAL_URI_SCHEME + "\" .");
"supported for" + dirUri; - put a space after "for"
Odd javadoc formatting, and typo "with out" -> "without":
+ /** Sets the required configurations for performing failover.
+ * with out any dependency on MiniDFSCluster
+ * */
Recommend adding a comment to the assert in NameNode#confirmFormat that the presence of the assert is necessary for the validity of the test.

Aaron T. Myers
added a comment - 24/Apr/12 18:47 This comment still isn't formatted correctly, and I think you can remove the "." in this sentence.
+ /** Sets the required configurations for performing failover.
+ * without any dependency on MiniDFSCluster
+ */
Otherwise it looks good. +1.

Uma Maheswara Rao G
added a comment - 24/Apr/12 19:28 Amith, small comment
+ * Sets the required configurations for performing failover
+ * without any dependency on MiniDFSCluster
Why do we need to mention that 'no dependancy on MiniDFSCluster'? Since this is a Util method, we need not mention this right?
very sorry for not figuring out to you in my previous review.
Thanks for your work!

java.util.NoSuchElementException
at java.util.AbstractList$Itr.next(AbstractList.java:350)
at org.apache.hadoop.hdfs.server.namenode.NameNode.confirmFormat(NameNode.java:731)
at org.apache.hadoop.hdfs.server.namenode.NameNode.format(NameNode.java:685)
at org.apache.hadoop.hdfs.server.namenode.NameNode.format(NameNode.java:228)
at org.apache.hadoop.hdfs.DFSTestUtil.formatNameNode(DFSTestUtil.java:122)
at org.apache.hadoop.hdfs.MiniDFSCluster.createNameNodesAndSetConf(MiniDFSCluster.java:680)

Uma Maheswara Rao G
added a comment - 24/Apr/12 19:56 Looks you have missed one line in HDFS-3275 _2.patch and HDFS-3275 _3.patch
Below code from HDFS-3275 _1.patch
+ assert dirUri.getScheme().equals(DEFAULT_SCHEME) : "formatting is not "
+ + "supported for " + dirUri;
+
+ File curDir = new File(dirUri.getPath());
// Its alright for a dir not to exist, or to exist (properly accessible)
Please take care in next version of the patch.

Hadoop QA
added a comment - 28/Apr/12 20:13 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12524985/HDFS-3275-4.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 2 new or modified test files.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in .
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2350//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2350//console
This message is automatically generated.