[
https://issues.apache.org/jira/browse/HDFS-6474?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14057844#comment-14057844
]
Charles Lamb commented on HDFS-6474:
------------------------------------
[~andrew.wang],
In general, this looks good. The FSN->FSD->EZM locking hierarchy is
straightforward and the obviously right thing to do.
I had a handful of relatively minor things that I noted, but I don't really
have strong feelings about most of them:
FileEncryptionInfo.java:
Are we not considering the DEK and EDEK to be proper nouns? Since we use DEK
and EDEK as abbreviations in a bunch of places should we make it clearer in our
comments that's what we're referring to and consider them as proper nouns?
DFSConfigKeys.java:
Is it typical to use MS time intervals for config options in HDFS? If so, then
no problem. If not, then does it make sense to have a 5 min default for a MS
granularity config param? I see why you're doing this since the executor wants
ms. If it's not likely that the user will actually care about ms, should we
just do the conversion from secs (in the config param) to ms (for the executor)?
EncryptionZoneManager.java:
Is it by design that you use LOG.debug and LOG.trace in the run() method
instead of just one or the other?
updateLatestKeyVersion(): I realize that the declaration of ezi has no
underlying instructions assoc'd with it, but visually, (I know this is a nit),
it would be nice to put the readLock() call right before the try (so just swap
the readLock() line and the EncryptionZoneInt ezi line).
{code}
// Release the lock while doing KeyProvider operation
{code}
Since the lock was already released in the previous line it might be good to
change this comment to something like
{code}
// Do the KeyProvider operation when the lock is not held
{code}
getFullPathname(): would it be useful to assert that the FSD lock is held or is
that a waste of time? If you believe we should then there are a few other
methods where it would be useful to add asserts of the same.
isValidKeyVersion: Should the javadoc read something like this:
@return true if (1) iip refers to a path in an encryption zone,
and (2) keyVersionName is a valid KeyVersion for that encryption
zone.
checkMoveValidity:
This comment is a little bit incorrect, or at least misleading:
{code}
* Throws an exception if the provided inode cannot be renamed into the
* destination because of differing encryption zones.
{code}
I realize it's an iip (and I also realize you weren't the author of this
comment), but perhaps s/inode/path/?
In general, is there a reason why you use KeyProvider.KeyVersion instead of
importing KeyProvider.KeyVersion directly?
FSNamesystem.java:
startFileInt. You are setting shouldContinue=false in two places and I think
that only one is necessary, right?
TestEncryptionZonesAPI:
"raw file" will have its own connotations soon enough. Perhaps change that to
"unencrypted file"?
> Namenode needs to get the actual keys and iv from the KeyProvider
> -----------------------------------------------------------------
>
> Key: HDFS-6474
> URL: https://issues.apache.org/jira/browse/HDFS-6474
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: namenode, security
> Reporter: Charles Lamb
> Assignee: Andrew Wang
> Attachments: hdfs-6474.001.patch
>
>
> The Namenode has code to connect to the KeyProvider, but it needs to actually
> get the keys and return them to the client at the right time.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
