Oops. I botched that, but it's never actually called (currently). The method is intended to return a string of the text for a FileNotFoundException since every command's text seems slightly different. The above base class was supposed to throw "not implemented", or return the string that got thrown. I munged it, but it's not called, so I'll fix it in the next refactored command review.

The end goal is "throw PathNotFoundException(path)" which will enforce a standard text message, and have a getter for the path. I intend to first refactor all FsShell commands. I can then universally change them all to use the new exception with just 2 LOC, and another jira to update all the hdfs tests.

I can see the wisdom of using a branch for larger features. Unfortunately in this case, code is being moved from a file into new files, which would make merges completely manual and error-prone. I also have someone waiting on my changes so they can add symlink support. Hence my hesitancy to use a branch...

Daryn Sharp
added a comment - 21/Apr/11 19:19 Oops. I botched that, but it's never actually called (currently). The method is intended to return a string of the text for a FileNotFoundException since every command's text seems slightly different. The above base class was supposed to throw "not implemented", or return the string that got thrown. I munged it, but it's not called, so I'll fix it in the next refactored command review.
The end goal is "throw PathNotFoundException(path)" which will enforce a standard text message, and have a getter for the path. I intend to first refactor all FsShell commands. I can then universally change them all to use the new exception with just 2 LOC, and another jira to update all the hdfs tests.
I can see the wisdom of using a branch for larger features. Unfortunately in this case, code is being moved from a file into new files, which would make merges completely manual and error-prone. I also have someone waiting on my changes so they can add symlink support. Hence my hesitancy to use a branch...

This was the specific code which led me to suggest that this could be better done in a branch:

/**
* TODO: A crutch until the text is standardized across commands...
* Eventually an exception that takes the path as an argument will
* replace custom text
* @param path the thing that doesn't exist
* @returns String in printf format
*/
protectedString getFnfText(Path path) {
thrownew RuntimeException(path + ": No such file or directory");
}

Which obviously should be changed sooner rather than later, and arguably should never be in trunk. If the intention is that this will never actually return a String, but rather always throw an exception, then you should change the return type to be vode, and the name of the method to "throwFnfError". It should probably also throw an IOException, as we do elsewhere in the code.

Note that, in the Hadoop projects, if you do work on a branch it's OK to do "commit-then-review" of the individual issues to speed up the development time. You'd then need to get a full review/+1 at branch merge time. I don't feel strongly about moving this work to a branch, but others might. Just a suggestion.

Aaron T. Myers
added a comment - 21/Apr/11 18:48 This was the specific code which led me to suggest that this could be better done in a branch:
/**
* TODO: A crutch until the text is standardized across commands...
* Eventually an exception that takes the path as an argument will
* replace custom text
* @param path the thing that doesn't exist
* @returns String in printf format
*/
protected String getFnfText(Path path) {
throw new RuntimeException(path + ": No such file or directory" );
}
Which obviously should be changed sooner rather than later, and arguably should never be in trunk. If the intention is that this will never actually return a String, but rather always throw an exception, then you should change the return type to be vode , and the name of the method to " throwFnfError ". It should probably also throw an IOException, as we do elsewhere in the code.
Note that, in the Hadoop projects, if you do work on a branch it's OK to do "commit-then-review" of the individual issues to speed up the development time. You'd then need to get a full review/+1 at branch merge time. I don't feel strongly about moving this work to a branch, but others might. Just a suggestion.

My concern with making the changes on a separate branch is the time it will take to review and commit the changes. I'm trying to avoid a "big bang" integration that will be harder for a committer, and require me to perform manual merges of any changes that occur in common and/or hdfs until it's committed. If others feel strongly, I can pursue a branch.

Regarding the TODOs, I have added 9 in the code I have touched. They are more of a wish list than truly transitional detritus. I do intend to address most of them, but the overall task is neither at risk, nor left in a bad shape if not completed (which I am committed to completing). The breakdown is:

1 is that ls should be iterative (technically out of scope for my work)

1 is about how the descriptive text should be auto-wrapped (nice to have)

5 are about standardizing the existing text of exceptions (nice to have)

1 is about better exit codes which already has another jira (nice to have)

1 is about less than ideal instantiation of the command factory, which is only an issue if dfsadmin and fsshell share a common base class instead of dfsadmin deriving from fsshell (nice to have)

Daryn Sharp
added a comment - 21/Apr/11 15:17 Thanks for the quick review!
My concern with making the changes on a separate branch is the time it will take to review and commit the changes. I'm trying to avoid a "big bang" integration that will be harder for a committer, and require me to perform manual merges of any changes that occur in common and/or hdfs until it's committed. If others feel strongly, I can pursue a branch.
Regarding the TODOs, I have added 9 in the code I have touched. They are more of a wish list than truly transitional detritus. I do intend to address most of them, but the overall task is neither at risk, nor left in a bad shape if not completed (which I am committed to completing). The breakdown is:
1 is that ls should be iterative (technically out of scope for my work)
1 is about how the descriptive text should be auto-wrapped (nice to have)
5 are about standardizing the existing text of exceptions (nice to have)
1 is about better exit codes which already has another jira (nice to have)
1 is about less than ideal instantiation of the command factory, which is only an issue if dfsadmin and fsshell share a common base class instead of dfsadmin deriving from fsshell (nice to have)

One high-level comment: this refactor of FsShell is making a lot of changes and leaving a lot of TODOs in the code, all of which will be removed once the total refactor is completed. Since you're proceeding by piecemeal switching the commands over to the new system, might it make sense to do all this work in a branch, and then let us review the final product? It seems unfortunate to have transitional code in trunk over the course of this whole project, and there's the obvious question of what happens to the TODOs if the total refactor is never completed.

Aaron T. Myers
added a comment - 21/Apr/11 01:25 +1, The code looks good to me.
One high-level comment: this refactor of FsShell is making a lot of changes and leaving a lot of TODOs in the code, all of which will be removed once the total refactor is completed. Since you're proceeding by piecemeal switching the commands over to the new system, might it make sense to do all this work in a branch, and then let us review the final product? It seems unfortunate to have transitional code in trunk over the course of this whole project, and there's the obvious question of what happens to the TODOs if the total refactor is never completed.

Fix the usage text for ls.
Scrub out ls code and conditionals from FsShell, move to new subclass of FsCommand.
Add temporary "hack" to accomodate commands that don't throw the same text for a FileNotFoundException...
Add getter/setter for the recursive flag in Command class.

Daryn Sharp
added a comment - 20/Apr/11 21:29 Fix the usage text for ls.
Scrub out ls code and conditionals from FsShell, move to new subclass of FsCommand.
Add temporary "hack" to accomodate commands that don't throw the same text for a FileNotFoundException...
Add getter/setter for the recursive flag in Command class.