hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yi Liu (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (HADOOP-10628) Javadoc and few code style improvement for Crypto input and output streams
Date Sun, 25 May 2014 13:15:04 GMT

     [ https://issues.apache.org/jira/browse/HADOOP-10628?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Yi Liu updated HADOOP-10628:
----------------------------

    Attachment: HADOOP-10628.patch

Thanks [~clamb] for the comments. The patch includes update for these comments.

Response to some comments:
{quote}
Do you have a test that verifies that seek with an exception unwinds
the seek position correctly? The contract is to not move the position
if an exception occurs.
{quote}
OK, let's add position check after seek with exception.

{quote}
Why create curOffset at all? It looks like it is only used to pass to
resetStreamOffset below.
{quote}

{{curOffset}} is to cache {{streamOffset}}, since {{streamOffset}} will be changed after 
the first {{resetStreamOffSet(position)}}

{quote}
+ // If target pos we have already read and decrypt.

This comment is confusing. Could you please reword it?
{quote}
Sure.

{quote}
For the code above, I wonder if we shouldn't maintain a reference to
the actual ByteBuffer once it is known to be ByteBufferReadable. If
the caller switches BBs, then it is possible that this could throw a
UnsupportedOperationException. So the check would be to see if the BB
was the same one that was already known to be BBReadable, and if not,
then check it again.
{quote}
{{ByteBufferReadable}} is decided by the underlying steam when {{CryptoInputStream}} is created,
and the {{inBuffer}} is owned by {{CryptoInputStream}}, the caller may not switch the underlying
stream or the {{inBuffer}}.

{quote}
+ @Override
+ public int read(ByteBuffer buf) throws IOException {
+ checkStream();
+ if (in instanceof ByteBufferReadable) {

It would be good if we didn't have to do this instanceof check here.
{quote}
It's necessary. We can also see this in {{FSDataInputStream}}



> Javadoc and few code style improvement for Crypto input and output streams
> --------------------------------------------------------------------------
>
>                 Key: HADOOP-10628
>                 URL: https://issues.apache.org/jira/browse/HADOOP-10628
>             Project: Hadoop Common
>          Issue Type: Sub-task
>          Components: security
>    Affects Versions: fs-encryption (HADOOP-10150 and HDFS-6134)
>            Reporter: Yi Liu
>            Assignee: Yi Liu
>             Fix For: fs-encryption (HADOOP-10150 and HDFS-6134)
>
>         Attachments: HADOOP-10628.patch
>
>
> There are some additional comments from [~clamb] related to javadoc and few code style
on HADOOP-10603, let's fix them in this follow-on JIRA.



--
This message was sent by Atlassian JIRA
(v6.2#6252)

Mime
View raw message