Fix CreateFlag handling

[milandesai]

We don't correctly handle the logic for Create with respect to the CreateFlags. We should implement this logic by correctly following the documentation for CreateFlag. First steps are to create unit tests that reproduce the problem. I have assigned this issue to Wei-Lin.

[weilintsaiWand]

Now I've finished some test cases at git-link and the results are summarized below: google doc As you can see above. All APPEND related test cases will throw

    java.io.IOException: java.io.IOException: Append is not supported.

Which is as expected. However, for CREATE only flag (Row 6), the result is    1. FileAlreadyExistsException when file is existed    2. AlreadyBeingCreatedException when file is created before and not closed yet which I am not sure is correct or not since the comments in CreateFlag.java are

// CREATE - to create a file if it does not exist, else throw FileAlreadyExists

I think the current result (throw AlreadyBeingCreatedException) is reasonable. However, I am not sure if there's better approach. I would appreciate any ideas.

[milandesai]

AlreadyBeingCreatedException is correct. The "Ideal Result" column is correct. The spreadsheet will be easier to analyze and comment on if you removed all the Append related cases and instead made the third column specify whether or not the file already exists. So Column 1 is for CREATE flag, Column 2 is for OVERWRITE flag, and Column 3 is "file_exists?". It is pretty clear that an Append flag will throw the correct Exception so we don't need to test it.

Additionally, a note for HadoopIllegalArgumentExceptions. It is good that you are getting those Exceptions when the flags are not properly set. But note that this is because of DFSClient, so the illegal arguments are caught before they command is sent to the server and handled by NamespaceProcessor. However, we still want to make NamespaceProcessor.create() robust. So you will need to add logic there to make sure an incorrect combination of flags throws an IOException.

When you are done writing the tests, before working on any code changes create a Pull request and link it here so I can review them.

[weilintsaiWand]

The sheet is refined as request. Will finish remaining tests and create a Pull request for unit-test code review later

[shvachko]

Just wanted to emphasize two things:

1. The correct behaviour is the one defined by hdfs, that is, if hdfs throws ABCE, then we should too, etc.
2. We should implement this logic both on the client and on the server as much as possible. The reason is that we plan to have other APIs and clients for Giraffa, so server side implementation will ensure consistent behaviour for them too.

[weilintsaiWand]

Pull request is created for 1st version unit tests code review

[weilintsaiWand]

Add a new version to fix the bug and let all unit tests pass in the last commit.

Please note all special cases (createFlag setting which may trigger exception) are all handled on server side (NamespaceProcessor) no matter it's APPEND or not. That is to say, the client side does not verify createFlag, it just forward flag to server side and let NamespaceProcessor handle the flag.

However, there's one exception: The client side DFSClient will throw IllegalArgumentException when the input flag is empty because it uses input flag to new EnumSetWritable

[milandesai]

Good work @weilintsaiWand. Few things:

1. You can use else if statement in NamespaceProcessor
2. Notice how in NamespaceProcessor, if none of the flags are specified and the file doesn't exist, we would get a NullPointerException. Since we expect to have at least one flag set, however, let's add an assert statement that one of the flags is true. This will remove the potential NullPointerException warning that I'm seeing in my IDE.
3. For tests, you can use annotations to state that you expect the method to throw an Exception. See TestRename for how to do this.
4. Let's remove the tests not related to checking flags (the first three tests). We already have other tests that check normal create functionality.

[weilintsaiWand]

All revisions done as request

[shvachko]

I just committed this. Thank you Wei-Lin