-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HADOOP-16458 LocatedFileStatusFetcher.getFileStatuses failing intermittently with s3 #1160
HADOOP-16458 LocatedFileStatusFetcher.getFileStatuses failing intermittently with s3 #1160
Conversation
1fea0d3
to
5b41f7c
Compare
tested: s3 ireland with Also, check out cloudstore Release: 2019-07-29, whose |
* There's basic tests in ITestS3AFSMainOperations; this | ||
* is see if we can create better corner cases. | ||
*/ | ||
public class ITestLocatedFileStatusFetcher extends AbstractS3ATestBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either I implement a test here or I cut the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you already started to implement the test then it would be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cut the file; all tests live in the ITestRestrictedRead operation.
Reason: a basic scan isn't that interesting, trying to break it is, and the test bucket I was working with was giving me list access to the store but not read on files. I wanted to see if that was the problem. i..e. it was the reporting that was at fault. It isn't, but there's a good sequence of work there combining basic LocatedStatusFetcher operations which work with those that don't
* There's basic tests in ITestS3AFSMainOperations; this | ||
* is see if we can create better corner cases. | ||
*/ | ||
public class ITestLocatedFileStatusFetcher extends AbstractS3ATestBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you already started to implement the test then it would be useful.
...-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/auth/ITestRestrictedReadAccess.java
Outdated
Show resolved
Hide resolved
intercept(AccessDeniedException.class, () -> | ||
ContractTestUtils.readUTF8(roleFS, subdirFile, data.length)); | ||
if (authMode) { | ||
// auth mode doesn't just not check the store, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't just not -- maybe not the best wording
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed it
ContractTestUtils.readUTF8(roleFS, emptyFile, 0)); | ||
} | ||
|
||
// Fun with Glob |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this to another test. If something fails somebody (me) will need to debug and it could be harder for a test this long.
...-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/auth/ITestRestrictedReadAccess.java
Outdated
Show resolved
Hide resolved
afe8030
to
9067458
Compare
I've also tested this against ireland and got a new test error:
Note that this only fails after the parallel test run. This maybe related to this change, as I have not seen this fail yet. I will try to debug this using my WIP fsck. I created https://issues.apache.org/jira/browse/HADOOP-16476 for a failure I've seen already to fail intermittently, but that should be unrelated. |
Updated PR
No changes to production code. Tested, S3 Ireland. All well except for HADOOP-16477, which surface as I'd set the fs.s3a.encryption.key for the bucket. Unrelated |
Unless this test is confusing DDB so there's no cleanup, I'd point to those test failures less as a sign of a problem here and more that our test runs still seem somehow to cause confusion, even after the tombstone fixes.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @steveloughran, there are some requested changes pending from me at ITestRestrictedReadAccess.java. Otherwise, it's a +1 from me.
thanks. Think I've addressed your comments; pushing up for yetus then I'll commit. Tested: S3 ireland |
9067458
to
861a19d
Compare
checkstyle is about an 81 char line where splitting it would make things less readable
|
861a19d
to
0ece51b
Compare
🎊 +1 overall
This message was automatically generated. |
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Globber.java
Show resolved
Hide resolved
0ece51b
to
a58d260
Compare
a58d260
to
d43dce6
Compare
🎊 +1 overall
This message was automatically generated. |
d43dce6
to
fc56e9e
Compare
🎊 +1 overall
This message was automatically generated. |
This adds the ITestS3AFSMainOperations test so that we have some globber tests against the S3A FS. They didn't find problems there, only a bug in setWorkingDir(). Change-Id: Idbdc4b049fa70fa31aa701b2e1c570c5bd208521
Improving diagnostics of scanning * stack traces don't get lost as failures get wrapped/aggregated * debug level logging of what is up in Globber * Preparation for a test of LocatedFileStatus in S3A, though I've got some better ideas there (i.e. make it a scale test) Change-Id: Ie85eef17a52314d35c95defa4105d72b8c84dd20
this eliminates HEAD requests on scans where candidates are files Change-Id: I56971977e28a3adf09cae3df1c3d6e634cbc7295
… no-read paths key point: LIST always works, but HEAD and GET fail; in auth mode many such failures get skipped. Change-Id: I66fc17021d5f2b4015c9afbb446df6cb4df0b9ce
… Store Lots of tests (bad: all same test case; will fix), to invoke LocatedFileStatusFetcher against various S3 paths Key summary: you get the error seen in the HADOOP-16458 JIRA if * the path contains no pattern and does not exist * the path contains no pattern, exists but doesn't match the supplied pattern filter. Change-Id: Ib66f316a526982136e1345d10e9750aaec48cb95
LocatedFileStatusFetcher * Debug level logging of every callable invoked, thread count and other useful events. * Minor IDE-related cleanups as this is the first time this file has been looked at for a while. * fixup javadocs Globber * moved to builder pattern to the new (and still marked as private) API For creating a globber. If this really delivers speedups against stores, we'd benefit from it in our own stores and externally in GCS, so ultimately it may become public. For now, just design it ready for that. Invoker * log duration of operations @ debug Change-Id: I87ca7008ab787a848a35457c7a72d8f47e6d558f
… off Change-Id: Ibe7f502a92c67040dbdba464e13751fdbe0db4f3
… method into pieces Change-Id: Ic65c49bd512f4b8f33c211b9e4924b203165619f
Change-Id: I6937aa4c7de8adc135d9eb6bf99cca1b69773014
…ileStatusFetcher Change-Id: I592cff79fce8680daee55bae983465919734a8f1
Change-Id: I792c6187fdaaf6c981ac635ec9b527a925ad3d14
Change-Id: I3c1d64749107e6f69be76cdcb4eab724cdb1f9ba
-break up the ITestRestrictedReadAccess operations into more methods and change test* to check* as the prefix as the IDE was complaining about test- prefixed methods which weren't annotated. -javadoc and other reviews of the Globber changes Change-Id: Ib4f176926e7631d93829bab45e85857480521042
Change-Id: I80e5054ae2a5b7c7b616586f9882f07fcf0b8f92
This is the first time anyone has edited the file for a few years; most of the warnings are unrelated to the patch. Change-Id: I429bc5f3c28d33f0321890487faa7ac2fd7a5ddd
fc56e9e
to
b81aa7a
Compare
finally merged to trunk. Thx for review |
Improving diagnostics of scanning