NamespaceProcessor methods reset INode block and location fields

[shvachko]

Original issue 92 created by shvachko on 2014-10-17T07:56:37.000Z:

Methods like setPermissions and setReplication use the single-argument getINode method to retrieve the INode in question. This method does not set the block and location fields in the INode. The INode is later updated in the Namespace table causing those fields to be deleted permanently, effectively making those files unreadable. Instead, methods that intend to later call updateINode should call getINode(<path>, true) to ensure the block and location fields are set. Patch attached.

[shvachko]

Comment #1 originally posted by shvachko on 2014-10-17T07:58:59.000Z:

Alternatively, we could have getINode return a read-only INode subclass that is rejected by updateINode to prevent these errors in the future. Not sure how important is though.

[shvachko]

Comment #2 originally posted by shvachko on 2014-10-17T08:00:41.000Z:

<empty>

[shvachko]

Comment #3 originally posted by shvachko on 2014-10-17T17:00:26.000Z:

About this change -- this is interesting.

There is basically no need for the 'needLocation' boolean anymore. We should just make 'getINode' always fetch blocks and locations.

Another idea is to change how we do updateINode to ensure we DON'T delete fields just because we aren't updating them.

Maybe we should have a INode booleans to indicate whether we are updating Blocks and Locations at all.

[shvachko]

Comment #4 originally posted by shvachko on 2014-10-17T18:30:11.000Z:

I guess it depends on how much of a performance impact deserializing the blocks and locations from the row bytes will have. Currently: getFileInfo, setQuota, create, delete, getContentSummary, mkdirs getPreferredBlockSize, and checkCanRename all use the getINode call without getting block locations. But it's mostly for checking parent dirs, which don't have blocks and locations anyway. The only ones that call getINode on a file are getFileInfo, getContentSummary, and getPreferredBlockSize. getFileInfo is called pretty often.

[shvachko]

Comment #5 originally posted by shvachko on 2014-10-18T02:52:25.000Z:

I want to spend some time thinking about this one.

I am unsure if we want to read in all fields and update all fields every time or only update the fields we are modifying.

I am strongly inclined to think we should be only modifying that which we update. However, we can make snapshots very easy to implement by updating all fields at the same time, no matter how little the change.

If we go about updating all fields at the same time then we should ALWAYS grab blocks and locations (ALL fields, really) -- regardless of whether it is a directory or file.

Let me know what you guys think.

[shvachko]

Comment #6 originally posted by shvachko on 2014-10-18T20:24:38.000Z:

I think we should do a combination of the two; read in only the fields we need to and update only the fields we need to. We can do this by making the INode class store a copy of the row bytes, and then we deserialize and cache on the getters. Simultaneously do what you said and store flags for which fields were updated, via booleans or bits. Besides better throughput, it would have the added benefit of simplifying the NamespaceProcessor code. Renames would also be more efficient since we can just copy the row bytes over to the new INode.

Also Plamen, can you explain how updating all fields at the same time would make snapshots easy to implement? I'm confused about that one.

Regardless, I think that should be a separate issue from this one, as this issue is just about fixing the bug introduced by calling getINode(src) instead of getINode(src, true).

[shvachko]

Comment #7 originally posted by shvachko on 2014-10-18T20:39:34.000Z:

If we update all the fields at the same time, then we can say that all fields of VERSION 1 is snapshot 1, and all fields of VERSION 2 is snapshot 2.

If we update only the fields we modify, then snapshot 1 and snapshot 2 require some calculation. Basically snapshot 2 is all fields of VERSION 1 and any fields which have a VERSION 2.

It is not that a big a deal I think, just less computation.

[shvachko]

Comment #8 originally posted by shvachko on 2014-10-24T21:59:16.000Z:

I think we should just update all the fields for now. We can work on optimization another time.

[shvachko]

Comment #9 originally posted by shvachko on 2014-10-31T20:28:55.000Z:

New patch in which getINode and newINode always obtain blocks/locations.

[shvachko]

Comment #10 originally posted by shvachko on 2014-10-31T20:43:01.000Z:

+1ed by Plamen and committed to trunk.