-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HADOOP-16547. make sure that s3guard prune sets up the FS #1402
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-16547. make sure that s3guard prune sets up the FS #1402
Conversation
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java
Outdated
Show resolved
Hide resolved
e023027
to
a199b5c
Compare
a199b5c
to
195f8fc
Compare
Change-Id: Iaf71561cef6c797a3c66fed110faf08da6cac361
…. Still not gone through a full test Change-Id: I5fd525b21eb67d1241b6af8d135862211dfda5b1
… FS if a path was provided and region calculation didn't force this in I can't think of a good way to test this with a unit test, except maybe one involving delegation tokens Change-Id: I914f2c2b4d3691c5b7c5bdbb98b84ce2dcc3cba0
195f8fc
to
15fa0a0
Compare
manually verified this patch works through DT games, as well as ITest run. All good |
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java
Show resolved
Hide resolved
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 we want this to be stable, then we need a test that actually we will have a filesystem after running maybeInitFilesystem
. Please add a test so we don't remove this fix by accident.
I'm adding the test with the caveat that because only certain configs could trigger it (no fs.s3a.ddb.region) even if there was a regression you can't be confident it is valid. Though if it fails for someone, we can be happy that they have found that regression. |
… FS. Even before the fix, this new probe would only fail in certain test configurations -but at least now we have it. Change-Id: I1290ccacedd155f6fa876de5de2eea9799d9494a
tested: s3 ireland; didnt rerun the manual tests this time. |
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.
Thanks @steveloughran for extending the test for prune, but it would be better to have the method itself tested: the protected boolean maybeInitFilesystem
, so if you could add a new test asserting that this method creates a filesystem.
That way it would be more stable, and we would test if the change it that method works.
will do |
Change-Id: I156e563ce64d1d031706075463d693b965c4a201
added tests; tested s3 ireland |
🎊 +1 overall
This message was automatically generated. |
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.
Thanks for the tests @steveloughran, looks good to me.
Tested against ireland. Results: one known flaky testConcurrentTableCreations failure (not related) and testMR failures (not related, known).
+1
…pache#1402) Contributed by Steve Loughran. Change-Id: Iaf71561cef6c797a3c66fed110faf08da6cac361
…. Contributed by Steve Loughran. Change-Id: Iaf71561cef6c797a3c66fed110faf08da6cac361
…. Contributed by Steve Loughran. Change-Id: Iaf71561cef6c797a3c66fed110faf08da6cac361
initial patch; not done the full testing yet
Change-Id: Iaf71561cef6c797a3c66fed110faf08da6cac361