hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Wang (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-6987) Move CipherSuite xattr information up to the encryption zone root
Date Wed, 17 Sep 2014 23:49:34 GMT

    [ 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)

Mime
View raw message