GiraffaFS / giraffa

Giraffa FileSystem (Slack: giraffa-fs.slack.com)
https://giraffa.ci.cloudbees.com
Apache License 2.0
17 stars 6 forks source link

Refactor INode into INodeFile and INodeDirectory #188

Closed milandesai closed 8 years ago

milandesai commented 8 years ago

I think it will be useful and cleaner to make INode an abstract class and subclass it into INodeFile and INodeDirectory, similar to HDFS. That will make it easier to add additional INode fields and to construct INodes by reducing the number of required parameters, especially for directories. It also makes the code less prone to errors related to properties meant only for files or directories. I noticed this when experimenting with INodeId-based RowKeys.

milandesai commented 8 years ago

Created a pull request that demonstrates what the end result would look like. Note that the INode class was entirely rewritten so its best to look at the new version than the diff. The consequential changes are constrained to INodeManager and NamespaceProcessor, with minimal leakage into tests and the web server. All tests are passing.

shvachko commented 8 years ago

The refactoring looks good. Couple of things

  1. I see that you removed fileId field of INode. Which you will need for the id-based RowKey. Should we bring it back, even though current impl. always sets it to GRANDFATHER_INODE_ID? We may want to call it inodeId as it is common for files and directories.
  2. Could you add JavaDoc descriptions all three classes.
  3. Hadoop inodes have two casting techniques: one is asFile() / asDirectory() another is valueOf(). We should choose one and stick to it. I do not have preferences, but valueOf() seems to be more "scalable" if you had more INode types. Although it is more chatty, as you need to use the class name INodeFile.valueOf(). But this is cleaner as you explicitly name the class to which you cast. Yet another way is to parametrize static <T> T valueOf(). Seems like the least amount of code?
milandesai commented 8 years ago
  1. Didn't mean to do that, added it back. I named the field simply "id"; "inodeId" seemed redundant since its already in the class INode.
  2. Done. Let me know if they look good to you.
  3. Makes sense to use valueOf. I didn't use the parameterized version though because I want the methods to do a sanity check and throw an IOException if the conversion can't be done. Also avoids casting warnings.

I also added a shortcut method INode.getPath() to replace the lengthy getKey().getPath() used everywhere. All tests are passing.

shvachko commented 8 years ago

There is one unused import of IOException in INode. I removed it and will commit. Looks good.

shvachko commented 8 years ago

I just committed this. Thank you Milan.