[
https://issues.apache.org/jira/browse/HADOOP-12994?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15224736#comment-15224736
]
Chris Nauroth commented on HADOOP-12994:
----------------------------------------
Hello [[email protected]]. This looks good overall. Thank you for expanding
the test coverage.
I ran live Azure testing with the new tests, and one of them failed, as
described below. I'm in the middle of a full test run now, which might take up
to an hour to complete. I'm not expecting any other impacts, but I'll confirm
later after the test run finishes.
Here are a few comments.
# Regarding this {{ChecksumFileSystem}} change:
{code}
- public ChecksumFileSystem(FileSystem fs) {
+ protected ChecksumFileSystem(FileSystem fs) {
super(fs);
}
{code}
I think this is backwards-compatible, but it took me some time to convince
myself. I think the reasoning is that since the class is abstract, it cannot
be instantiated, so therefore a {{public}} constructor doesn't make sense.
Anonymous inner classes are not a concern, because they rely on the default
no-args constructor. Do I have it right? I'm not asking for any changes here,
just trying to confirm my own thinking and calling it out in case others want
to comment.
# I don't see anything that checks if the {{supports-positioned-readable}}
contract option is enabled. There should be corresponding
{{skipIfUnsupported}} calls, right?
# {{FSDataInputStream}} has a typo in the JavaDocs:
{code}
* @param buffer buffer into which data is readT
{code}
# After I applied the patch, {{TestAzureNativeContractSeek#testReadSmallFile}}
failed with an unexpected {{EOFException}}. I think the problem is that it
doesn't override {{NativeAzureFileSystem#NativeAzureFsInputStream#read}} to map
{{EOFException}} to -1. I tried applying a similar change to what you did for
{{S3AInputStream}}, and then the test passed.
> Specify PositionedReadable, add contract tests, fix problems
> ------------------------------------------------------------
>
> Key: HADOOP-12994
> URL: https://issues.apache.org/jira/browse/HADOOP-12994
> Project: Hadoop Common
> Issue Type: Improvement
> Components: fs
> Affects Versions: 2.8.0
> Reporter: Steve Loughran
> Assignee: Steve Loughran
> Attachments: HADOOP-12994-001.patch, HADOOP-12994-002.patch
>
>
> Some work on S3a has shown up that there aren't tests catching regressions in
> readFully, reviewing the documentation shows that its specification could be
> improved.
> # review the spec
> # review the implementations
> # add tests (proposed: to the seek contract; streams which support seek
> should support positioned readable)
> # fix code, where it differs significantly from HDFS or LocalFS
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
