Integrated in Hadoop-Mapreduce-trunk #1175 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1175/)
HADOOP-8709. globStatus changed behavior from 0.20/1.x (Jason Lowe via bobby) (Revision 1376653)

Result = FAILURE
bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376653
Files :

• /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileContext.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FSMainOperationsBaseTest.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java

Integrated in Hadoop-Hdfs-trunk #1144 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1144/)
HADOOP-8709. globStatus changed behavior from 0.20/1.x (Jason Lowe via bobby) (Revision 1376653)

Result = FAILURE
bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376653
Files :

• /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileContext.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FSMainOperationsBaseTest.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java

Integrated in Hadoop-Hdfs-0.23-Build #353 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/353/)
svn merge -c 1376653 FIXES: HADOOP-8709. globStatus changed behavior from 0.20/1.x (Jason Lowe via bobby) (Revision 1376657)

Result = UNSTABLE
bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376657
Files :

• /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
• /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileContext.java
• /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
• /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FSMainOperationsBaseTest.java
• /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java

Integrated in Hadoop-Mapreduce-trunk-Commit #2657 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2657/)
HADOOP-8709. globStatus changed behavior from 0.20/1.x (Jason Lowe via bobby) (Revision 1376653)

Result = FAILURE
bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376653
Files :

• /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileContext.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FSMainOperationsBaseTest.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java

+1 on the patch.

Integrated in Hadoop-Hdfs-trunk-Commit #2693 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2693/)
HADOOP-8709. globStatus changed behavior from 0.20/1.x (Jason Lowe via bobby) (Revision 1376653)

Result = SUCCESS
bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376653
Files :

• /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileContext.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FSMainOperationsBaseTest.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java

Integrated in Hadoop-Common-trunk-Commit #2629 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2629/)
HADOOP-8709. globStatus changed behavior from 0.20/1.x (Jason Lowe via bobby) (Revision 1376653)

Result = SUCCESS
bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376653
Files :

• /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileContext.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FSMainOperationsBaseTest.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java

Thanks Jason,

I pulled the patch into trunk, branch-2, branch-2.1.0-alpha and branch-0.23.

I just filed HADOOP-8724 to add in the new API. I assigned it to myself and I will try and post an initial patch in the next day or so for discussion. I am going to check in the patch to maintain 1.0 compatibility now.

If we do want different behavior we should add in a new API and deprecate the old one. This is how most projects evolve while still maintaining backwards compatibility. I like Chris's idea for the new API, and I would actually go a step further and create an Glob class, that holds the base Path and the regexp/pattern. That way other methods that may take a glob, like in Shell or Util, it is obvious when something will be interpreted as a Glob and when it will be interpreted as a Path. But all of that can and should be done in a separate JIRA. I agree with Sanjay also. This new API should remove the null return values. It will through a FNFE when the base path is not found, and will return an empty iterator when the pattern does not match anything.

I think this solution would address all concerns. I am happy to file the separate JIRA for the updated API and I owuld like to check in the current patch, assuming that there are no objections to this.

+1 on restoring the 1.x semantics. Backwards compatibility is a concrete goal, and the correct semantics are not clear with the current API anyway.

The globStatus signature is unfortunate. It mixes URIs and regular expressions. It would be much clearer if the caller provided a base URI and a regexp to match descendants:

public RemoteIterator<FileStatus> glob(Path base, String regexp)
  throws IOException, FileNotFoundException, InterruptedException;

This can throw FNF if the base URI doesn't exist and return an empty iterator when the regexp doesn't match anything. At the time, the change to globStatus seemed to model this reasonably well (pre-glob char specify a base resource, segments after the first glob are part of the regexp), but I agree with the above: it's not worth the break in compatibility.

If we are breaking compatibility to get to a more correct API then should we fix the null returns (rows 1 and 2 in the table above)?

One note: HDFS's FileSystem create/mkdir methods create missing parent directories. Hence apps have tended to not rely on parent dirs always being pre-created
as part of configuration/setup. (We changed this default behavior in FileContext.) Hence it is not completely unreasonable for glob Status applications to view missing parent dirs as normal rather than error.

Daryn, there are two areas of consistency:

1. /a/nonExist/c and /a/nonExist*/c - your argument above - consistency when parent dir is missing
2. /a/nonExist*/c and /a/nonExist/c* - consistency for globing regardless of where it occurs - return empty or the set that matches.
   I prefer the 2nd one.

The shell uses all the documented semantics.

Currently, sometimes FNFE and sometimes empty array for the same condition seems bizarre. I don't understand why there should be a difference between "/a/nonExist/c*" and "/a/nonExist*/c"? Let's say they are consistent and both throw FNFE. Now that would mean "/a/

/c*" should throw (for consistency) even though there are some matches? That defeats the purpose of "tell me what might exist that matches this pattern". It's really hard for the caller to understand the context-sensitive nature of when it might throw. Users are unlikely to handle all the conditions correctly, leading to odd edges based on the given path.

It makes my head hurt just thinking about it, which is why I keep leaning towards the currently documented semantics which are very clear.

To summarize:

Below "nonExist" means that the component does not exist
  | Hadoop 20, Hadoop 1     | Trunk, hadoop 2
--|-------------------------------------------------------
1 | /a/nonExist/c    null   |  null
2 | /a/b/nonExist    null   |  null
3 | /a/b/nonExist*   empty  |  Empty
4 | /a/nonExist/c*   empty  |  fnfException
5 | /a/nonExist*/c   empty  |  empty

Note the the return of null for row 1 and 2 is apparently used by the shell (see daryn's comment).

The issue with inconsistently throwing FNF for a glob depending on a parent dir makes it harder to tell if something is a non-existent glob. I'd rather not risk having issues with FsShell reverting to 1.x behavior of sometimes unexpectedly creating paths with glob chars in them. Or user code for that matter.

If left as-is, I think we'll need to change MR code that tries to batch up certain exceptions. It doesn't expect an IOException, so it rips further up the stack and discards the batched exceptions. I'm sure there's plenty of user code not expecting IOException either. Esp. in case like "/path-exists/$TIMESTAMP/*.dat" where it used to return an empty array, but now throws an exception because $TIMESTAMP doesn't exist yet.

This just seems like an extremely risky change inadvertently caused by the listStatus change (which I think is generally ok). Why don't we consider adding a globStatus(Path, boolean) to control whether exceptions will be thrown?

Returning an empty array is the right behavior for glob when nothing matched.
A reasonable situation to consider is throwing an exception if a parent directory, that was not glob'ed, does not exist; if we want to do that it should be a separate jira. +1 for going back to the original behavior.

Can you please summarize your final solution.

Jason, between 1.0 and 0.23 there are incompatible changes, as marked in the jiras. The primary motivation of this jira should be correct behavior and no backward compatibility. So could you please update the description and the jira summary accordingly.

That said, the main question you are raising is, is the new behavior correct. I agree with Daryn that globStatus is different from getFileStatus. I also think your proposal seems reasonable to me.

> The thing that bugs me is that the documentation and the behavior do not match.
Yes, at the very least, the docs need fixed.

I'm not going to block it, but Sanjay codified the behavior in the test, so he should weigh in.

The thing that bugs me is that the documentation and the behavior do not match. It especially bugs me because it is on something that is marked public stable and is commonly used. If I had my wish the behavior would match that of 1.0, just because the exception may not be exceptional, but is somewhat rare. I fear it will catch users off guard when it does show up, and their code will not be able to handle it. Additionally most globbing libraries I have seen never throw an exception or return an error, most globbing is in c, unless something is seriously wrong.

However, a lot of this is selfishness on my part because most of my users are on the 1.0 line, and would expect that behavior. I realize that making another change to the APIs behavior is risky so I am fine with whatever we do so long as the documentation and the API's behavior match.

Now putting on my release manager hat I really would like to do a release of 0.23.3 soon, but this is one JIRA that I consider somewhat of a blocker because of the mismatch. However we fix it I would like a fix soon.

I am a +1 on the patch as is. I reviewed it and the code looks fine to me. but Jakob, I am not going to check it in right away. If you don't want this to happen feel free to give it a -1 or even a -0, and I will look into posting a patch that updates the documentation instead.

The general consensus of the discussion previously was that returning null should be avoided (as it requires an extra null check), specifically to indicate that the file you're looking for wasn't found, since there's a perfectly good exception to indicate that. The only thing that bothers me about throwing FNFE for globbing is that globbing is an exploratory operation and not finding anything, including the base path you were looking at, seems a reasonable, non-exceptional outcome. If we're going to treat the base path not existing as something exceptional, FNFE seems a good way to do it.

TestZKFailoverController failure is unrelated, see HADOOP-8591.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12541767/HADOOP-8709.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 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. The javadoc tool did not generate any warning messages.

+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 failed these unit tests in hadoop-common-project/hadoop-common:

org.apache.hadoop.ha.TestZKFailoverController

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1342//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1342//console

This message is automatically generated.

Yes, I saw the discussion, but it seemed focused on listStatus and not globStatus. I understand the desire to differentiate between a non-existent and empty directory for listStatus, but I'm not sure that applies to globStatus. Most callers are not going to expect to handle three types of behavior from globStatus when files are not found, as it currently can return null, an empty array, or throw FNFE depending upon the situation. Most callers simply care if there were files found or not. One might wonder why it isn't always throwing FNFE if nothing is found.

I think backwards compatibility is an important goal, so my preference would be to preserve the 1.x behavior. Attaching a patch to that effect. If there's enough demand for the FNFE behavior, we can add an alternate interface so new clients can access that functionality without breaking existing clients.

If we decide that it's more important for globStatus to throw FNFE than preserve compatibility with 1.x, I think we should clean it up so it's consistent about it – either always throw FNFE or at a minimum stop returning null so callers only have to check for an empty array or catch FNFE to realize no file was found.

You will notice that HADOOP-6201 was explicitly marked as an incompatible change. Please see the discussion that happened at the time.

I originally thought the change was unintentional until I ran across the explicit test cases for it in FSMainOperationsBaseTest and FileContextMainOperationsBaseTest. That indicates we intentionally want globStatus to throwing FNFE.

I agree that if we're serious about preserving backwards compatibility, we need to start creating alternative methods with the new behavior rather than breaking the contracts of established methods. And I, too, am skeptical on the merits of having globStatus break compatibility to throw FNFE.

While I'm inclined to agree that the glob handling should be better, this is a non-trivial change for users to add an instantiation of GlobFilter (which seems like a possible misuse of the class) to determine if listStatus or globStatus should be called. Then they also need to catch an unexpected exception where none was generated before.

This method is often used to poll if files exist yet (ex. /some-path/$TIMESTAMP/*.blah). That code will break when the $TIMESTAMP dir doesn't exist yet. There are also so many inconsistencies when the exception will be thrown. Ex. "/path/does-not-exist/*" throws, but "/path/

/*" will not... I'm also worried by the fact that existing code might now erroneously decide to create paths with metachars when a glob's parent dir doesn't exist.

I think this wasn't a conscious incompatibility, but was broken by the listStatus change. I think it's a bad change since it fundamentally changes the semantics of 1.x behavior. I'd say a new method should be used to introduce this new behavior and we should restore the previous behavior per the javadoc.

I don't think it's the job of globStatus to be used as a function to distinguish between globbed paths and non-globbed paths based on return code. There's a separate class, GlobFilter, which can be used to check for glob patterns within a path with the added benefit that it doesn't require fs operations.

Just discovered that FileContext has duplicated code and therefore the same issues, javadoc inconsistencies and all.

[...] it does provide the ability to distinguish between a non-existent expected directory level vs. a pattern matching nothing for some cases. I can see some merit there.

The big demerit is now it's impossible to distinguish "not a glob, doesn't exist" and "is a glob, but a parent dir doesn't exist" because FNF means either. Let's say I want to create a path that doesn't exist iff it's not a glob. How do I do that? I don't want to accidentally create a path with glob metachars in it...

I think FNF should only be thrown for non-existent non-glob paths. That alone is an incompatible change that requires a try block. Throwing FNF for any glob path breaks the semantics of the method and there is no workaround.

OK, I think I can see the reasoning behind throwing FNFE from globStatus. Basically it boils down to breaking the path into directory-level components and for each component:

• if the component doesn't contain patterns, treat it like listStatus and throw FNFE if it doesn't exist
• if the component contains patterns, treat it like a pattern match and return an empty array if it doesn't exist

So for example, if the pattern is /a/b/* but /a/b/ doesn't exist, then it will throw FNFE. However if the pattern is /a/b*/* for the same situation, then it will return an empty array even though /a/b* doesn't exist. It does match my expectation that matching shouldn't throw but at a directory component level rather than the top-level.

The new behavior seems a bit cumbersome, since callers now have to check for both an empty array and catch FNFE to handle the case of nothing found. However it does provide the ability to distinguish between a non-existent expected directory level vs. a pattern matching nothing for some cases. I can see some merit there.

That still leaves the issue of returning null for a non-globbed pattern, which I still assume is just broken if we're going to throw FNFE for some globbed paths.

FsShell for one needs to know if something is or isn't a glob to work correctly.

I'm a bit uneasy with the incompatibility. I think the listStatus change for java compat is fine. However, I agree with Jason that globStatus is different. It's like a grep. When you call it, you don't know if the path contains globs. You're just acknowledging that you are prepared to deal with globs. The previous semantics allow you know if it is indeed a glob, and whether it matches.

1. null: not a glob & doesn't exist
2. empty array: glob with no matches
3. non-empty array: either existing path or expanded glob

Maybe it would be ok to throw FNF for a non-glob pattern that doesn't exist, instead of returning null. However, a globbed pattern should always return an array whether it matches or not so you know if it's a glob.

The TestZKFailoverController, TestNameNodeMetrics, and TestPersistBlocks failures appear to be unrelated. The remaining failed tests are all failures complaining about globStatus not throwing FNFE, which gets back to the question of whether it really should.

If we do choose the route of changing the docs (and I assume changing the behavior of a path without globbing to also throw FNFE if not found), then I see that FileInputFormat.listStatus is still expecting the old behavior.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12541382/HADOOP-8709.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 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. The javadoc tool did not generate any warning messages.

+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 failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

org.apache.hadoop.fs.viewfs.TestFSMainOperationsLocalFileSystem
org.apache.hadoop.ha.TestZKFailoverController
org.apache.hadoop.fs.TestFSMainOperationsLocalFileSystem
org.apache.hadoop.hdfs.web.TestFSMainOperationsWebHdfs
org.apache.hadoop.hdfs.server.namenode.metrics.TestNameNodeMetrics
org.apache.hadoop.hdfs.TestPersistBlocks

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1323//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1323//console

This message is automatically generated.

Small patch to restore the original behavior, if we choose that route.

I wasn't proposing to revert HADOOP-6201, but even if we propose to just change the javadoc there's still an inconsistency in the code. Currently a path without a glob pattern returns null if nothing matches, while a path with a glob a pattern that doesn't match anything throws FNFE. Seems like either we should match the old behavior or at least throw FNFE in both cases, depending upon which way we want to go.

Personally I'd rather not break compatibility unless there's a very compelling reason to do so. The directory listing case was a clear reason to do so, and I can see HADOOP-6201's thinking that we should match that new behavior and also Java's convention. But I'm on the fence if that's enough reason to break compatibility here as well. I'm not aware of a Java convention for file globbing behavior. Globbing is like pattern matching, and I don't believe pattern matchers normally throw when they fail to match.

Are we talking of reverting HADOOP-6201? I think if listStatus throws FNFE (we had some JIRAs around this, linked on HADOOP-6201), globStatus should throw it too. Are we going to be changing javadocs or behavior here?