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 Fri, 19 Sep 2014 09:12:34 GMT

    [ https://issues.apache.org/jira/browse/HDFS-6987?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14140216#comment-14140216
] 

Andrew Wang commented on HDFS-6987:
-----------------------------------

Hi Zhe, thanks for revving this. The new version looks a lot closer, but still have some things
to address:

Nits:
* Still some lines longer than 80 chars. You can typically set your IDE to autowrap lines
for you.
* Charles' comment about using NameNode.LOG rather than FSNamesystem.LOG in FSDirectory is
a good one. We're also still missing some isDebugEnabled if wrappers (i.e. FSDir#getFileEncryptionInfo).

Method argument ordering
* This is a good opportunity to make argument ordering more consistent
* cipherSuite is also before or after keyName; I'd prefer to always put it first so it's like
FileEncryptionInfo. The following is not an exhaustive list, but these need to be updated:
EncryptionZone, EncryptionZoneInt, addEncryptionZone, unprotectedAddEncryptionZone,
* KeyVersion does name then versionName, so let's do that in FileEncryptionInfo as well.
* EncryptionZone's constructor is also a bit sloppy, it should really put the {{id}} field
should first so it's more like EncryptionZoneInt, i.e. id, path, suite, keyName.

Log messages:
* getFileEncryptionInfo, let's expand those debug logs with more context. i.e. they should
say "Could not get any xattrs for file <x> in encryption zone <y>", "xattr <the
xattr name> not found on file <x> in encryption zone <y>", etc.

The rest:
* Still doing an additional KeyProvider call to getMetadata, see my previous review comment
about this.
* Can we slap an explanatory TODO on getINodes() hack? Considering we do it in so many places,
it seems worth breaking it out into a single function at least. This is probably something
we should try to clean up later; follow-on JIRA?
* getFileEncryptionInfo could use javadoc explaining the params. Namely, that iip can be null,
and what happens if so.
* I don't find the name IndivFileEncryptionInfo any more illuminating than IndFEInfo. What
we want here is an adjective signifying that it's partial, or only the info stored per-file.
Maybe PerFileEncryptionInfo? Adding some method javadoc to getFileEncryptionInfo about how
we combine file and EZ info would also be good.
* addToInodeMap, we should include the xattr name in the WARN.
* getFileEncryptionInfo, using a foreach and then indexOf inside is probably {{O(n^2)}}. Why
not just have something like:

{code}
XAttr feXAttr = null;
for (XAttr x : fileXAttrs) {
    if (...) {
        feXAttr = x;
    }
}
{code}

Tucu comments above:
* We need to do these :)
* DFSClient has a call to EncryptedKeyVersion that was temporarily hardcoded to null, we need
to pass the acutal keyName.
* MiniKMS is hacked right now to set KEY_AUTHORIZATION_ENABLE to false, let's remove this
lineNits:
* Still some lines longer than 80 chars. You can typically set your IDE to autowrap lines
for you.
* Charles' comment about using NameNode.LOG rather than FSNamesystem.LOG in FSDirectory is
a good one. We're also still missing some isDebugEnabled if wrappers (i.e. FSDir#getFileEncryptionInfo).

> 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, HDFS-6987-20140918-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