Skip to content

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

Merged
merged 5 commits into from
Sep 18, 2019

Conversation

steveloughran
Copy link
Contributor

initial patch; not done the full testing yet

Change-Id: Iaf71561cef6c797a3c66fed110faf08da6cac361

@steveloughran steveloughran force-pushed the s3/HADOOP-16547-prune-auth branch from e023027 to a199b5c Compare September 10, 2019 09:18
@apache apache deleted a comment from hadoop-yetus Sep 12, 2019
@apache apache deleted a comment from hadoop-yetus Sep 12, 2019
@steveloughran steveloughran force-pushed the s3/HADOOP-16547-prune-auth branch from a199b5c to 195f8fc Compare September 12, 2019 09:45
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
@steveloughran steveloughran force-pushed the s3/HADOOP-16547-prune-auth branch from 195f8fc to 15fa0a0 Compare September 13, 2019 14:15
@steveloughran
Copy link
Contributor Author

manually verified this patch works through DT games, as well as ITest run. All good

@steveloughran steveloughran added bug fs/s3 changes related to hadoop-aws; submitter must declare test endpoint labels Sep 13, 2019
@apache apache deleted a comment from hadoop-yetus Sep 13, 2019
Copy link

@bgaborg bgaborg left a 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.

@steveloughran
Copy link
Contributor Author

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
@steveloughran
Copy link
Contributor Author

tested: s3 ireland; didnt rerun the manual tests this time.

Copy link

@bgaborg bgaborg left a 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.

@steveloughran
Copy link
Contributor Author

will do

Change-Id: I156e563ce64d1d031706075463d693b965c4a201
@steveloughran
Copy link
Contributor Author

added tests; tested s3 ireland

@hadoop-yetus
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
0 reexec 78 Docker mode activated.
_ Prechecks _
+1 dupname 0 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 1 new or modified test files.
_ trunk Compile Tests _
+1 mvninstall 1200 trunk passed
+1 compile 32 trunk passed
+1 checkstyle 24 trunk passed
+1 mvnsite 36 trunk passed
+1 shadedclient 784 branch has no errors when building and testing our client artifacts.
+1 javadoc 27 trunk passed
0 spotbugs 59 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 56 trunk passed
_ Patch Compile Tests _
+1 mvninstall 32 the patch passed
+1 compile 26 the patch passed
+1 javac 26 the patch passed
+1 checkstyle 18 the patch passed
+1 mvnsite 31 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 844 patch has no errors when building and testing our client artifacts.
+1 javadoc 23 the patch passed
+1 findbugs 61 the patch passed
_ Other Tests _
+1 unit 70 hadoop-aws in the patch passed.
+1 asflicense 29 The patch does not generate ASF License warnings.
3465
Subsystem Report/Notes
Docker Client=19.03.0 Server=19.03.0 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1402/6/artifact/out/Dockerfile
GITHUB PR #1402
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux f402829dea9e 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 419dd0f
Default Java 1.8.0_222
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1402/6/testReport/
Max. process+thread count 320 (vs. ulimit of 5500)
modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1402/6/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

@apache apache deleted a comment from hadoop-yetus Sep 18, 2019
@apache apache deleted a comment from hadoop-yetus Sep 18, 2019
Copy link

@bgaborg bgaborg left a 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

@bgaborg bgaborg merged commit 5db32b8 into apache:trunk Sep 18, 2019
smengcl pushed a commit to smengcl/hadoop that referenced this pull request Oct 8, 2019
…pache#1402)

Contributed by Steve Loughran.

Change-Id: Iaf71561cef6c797a3c66fed110faf08da6cac361
amahussein pushed a commit to amahussein/hadoop that referenced this pull request Oct 29, 2019
…. Contributed by Steve Loughran.

Change-Id: Iaf71561cef6c797a3c66fed110faf08da6cac361
RogPodge pushed a commit to RogPodge/hadoop that referenced this pull request Mar 25, 2020
…. Contributed by Steve Loughran.

Change-Id: Iaf71561cef6c797a3c66fed110faf08da6cac361
@steveloughran steveloughran deleted the s3/HADOOP-16547-prune-auth branch October 15, 2021 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fs/s3 changes related to hadoop-aws; submitter must declare test endpoint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants