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