[
https://issues.apache.org/jira/browse/HDFS-6987?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14138220#comment-14138220
]
Andrew Wang commented on HDFS-6987:
-----------------------------------
Hi Zhe, thanks for the patch. I took a look, had a few comments:
Nitty / logging / minor:
* Some lines longer than 80chars
* Rather than "IndFileEncryptionInfo", maybe "INodeFileEncryptionInfo"? "Ind"
isn't very descriptive.
* Need to wrap debug log calls in {{LOG.isDebugEnabled()}} checks, this avoids
string construction overhead. Consider removing or lowering these to TRACE also.
* The WARNs are only really important when we find one encryption attr, but not
the other. For an unencrypted file that has xattrs, we shouldn't print anything.
* Typo: "eninfo"?
* Meta comment, remember that log messages need to be useful to cluster admins
without much additional context. Normally you want to give them a hint to what
action to take in response, or at least help them understand the issue.
The rest:
* The callers of getFileEncryptionInfo already have an IIP, so we could pass
the IIP to getFEInfo to avoid getting it twice. Getting the IIP somewhat
expensive, and this is also a very popular function.
* Maybe we should add a helper function that gets an XAttr from an INode given
a name (or returns null if not found), seems to be a common idiom.
* Seems worth printing a debug if {{fileXAttrs == null ^ ezXAttrs == null}},
since either both or neither should be set. A mismatch means a screw up with
raw xattrs.
* I expected the CipherSuite to make it into EncryptionZoneInt and
EncryptionZone. Then in getFileEncryptionInfo we can just use those fields,
instead of re-pulling things from the zone xattr. Saves us some PB decoding.
* KeyProvider operations are rather expensive, since they involve an RPC. I
think what we want to do is, in createEncryptionZone, change the
{{provider.getCurrentKey}} to {{provider.getMetadata}}. It should still work to
test that the keyName is valid, and then we pass down the CipherSuite. Then we
aren't doing an additional RPC in createEncryptionZoneInt.
* tucu asked for the keyName to also be provided in FileEncryptionInfo, I don't
see it being added?
> Move CipherSuite xattr information up to the encryption zone root
> -----------------------------------------------------------------
>
> Key: HDFS-6987
> URL: https://issues.apache.org/jira/browse/HDFS-6987
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: encryption
> Reporter: Andrew Wang
> Assignee: Zhe Zhang
> Attachments: HDFS-6987-20140917-v1.patch
>
>
> All files within a single EZ need to be encrypted with the same CipherSuite.
> Because of this, I think we can store the CipherSuite once in the EZ rather
> than on each file.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
