[
https://issues.apache.org/jira/browse/HDFS-3246?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16784761#comment-16784761
]
Sahil Takiar commented on HDFS-3246:
------------------------------------
[~jojochuang] yes, the implementation of {{decrypt}} was the trickiest part of
this patch for me, mainly because I'm not familiar with how
{{CryptoInputStream}} works. If there are other HDFS committers more familiar
with this code, feedback is welcome. The new {{decrypt}} method is meant to be
a merge of {{decrypt(long position, byte[] buffer, int offset, int length)}}
and {{decrypt(ByteBuffer buf, int n, int start)}}. I made sure to add plenty of
unit tests to make sure the changes in {{CryptoInputStream}} work properly.
As for your specific comments:
{quote}buf.position(start + len); --> not needed?
{quote}
The call to {{inBuffer.put(buf)}} on line 4 increments the position of {{buf}}
which is why it is necessary to reset the position.
{quote}buf.limit(limit); --> not needed?
{quote}
The limit of the buffer is changed from its original limit on line 3; this call
just resets the limit of the {{buf}} back to its original value, which is
necessary because the method is not suppose to change the limit of the buffer.
{quote}len += outBuffer.remaining(); --> len += Math.min(n - len,
inBuffer.remaining())?
{quote}
The next line calls {{buf.put(outBuffer)}} which will add all remaining bytes
in {{outBuffer}} to {{buf}} (up until the limit of {{buf}} is reached). So
adding {{outBuffer.remaining()}} should be fine. Technically, its not an
accurate representation of how much data has been decrypted because the {{buf}}
limit can be reached before all {{outBuffer}} bytes are transferred, but given
how the method is structured it seems to be fine. The logic here is similar to
what {{decrypt(ByteBuffer buf, int n, int start)}} does.
Essentially, this method decrypts {{buf}} chunk by chunk. It loads a chunk of
the {{buf}} into a {{localInBuffer}} (which is borrowed from a buffer pool) and
writes the decrypted data into a {{localOutBuffer}} (also borrowed from a
buffer pool). Then it overwrites the chunk of {{buf}} that was originally
loaded into the {{localInBuffer}}.
Overall, I agree the way this method is structured is a bit confusing, but its
inline with how the other decrypt methods work, which is why I wrote it this
way.
I can add some more code comments if that will help as well.
> pRead equivalent for direct read path
> -------------------------------------
>
> Key: HDFS-3246
> URL: https://issues.apache.org/jira/browse/HDFS-3246
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: hdfs-client, performance
> Affects Versions: 3.0.0-alpha1
> Reporter: Henry Robinson
> Assignee: Sahil Takiar
> Priority: Major
> Attachments: HDFS-3246.001.patch, HDFS-3246.002.patch,
> HDFS-3246.003.patch, HDFS-3246.004.patch, HDFS-3246.005.patch,
> HDFS-3246.006.patch
>
>
> There is no pread equivalent in ByteBufferReadable. We should consider adding
> one. It would be relatively easy to implement for the distributed case
> (certainly compared to HDFS-2834), since DFSInputStream does most of the
> heavy lifting.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
