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