Skip to content

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