HADOOP-17131. Refactor S3A Listing code for better isolation. #2148

mukund-thakur · 2020-07-16T12:32:06Z

Ran tests in ap-south-1 bucket using mvn clean verify -Dparallel-tests and mvn clean verify -Ds3guard -Ddynamo -Dparallel-tests
All good.

mukund-thakur · 2020-07-17T14:26:41Z

Test failures:
[ERROR] Failures: [ERROR] ITestS3GuardOutOfBandOperations.testRootTombstones:619->Assert.assertNotEquals:161->Assert.failEquals:185->Assert.fail:88 Listing was not supposed to include s3a://mthakur-data/test/testRootTombstones[auth=false]-nonauth0da8099f-ff80-4bf1-87f4-6a1326388907/test.file. Actual: s3a://mthakur-data/test/testRootTombstones[auth=false]-nonauth0da8099f-ff80-4bf1-87f4-6a1326388907/test.file [ERROR] ITestS3GuardOutOfBandOperations.testTombstoneExpiryGuardedDeleteRawCreate:358->Assert.assertNotEquals:161->Assert.failEquals:185->Assert.fail:88 Listing was not supposed to include s3a://mthakur-data/test/TEGDRC-13fba05d-ff3c-4a15-bb96-75942ac8f458/file. Actual: s3a://mthakur-data/test/TEGDRC-13fba05d-ff3c-4a15-bb96-75942ac8f458/file

The reason test fails is we are mocking the ITtlTimeProvider after the fileSystem is initialised thus not propagating the updated ITtlTimeProvider instance to StoreContext.

One solution I have in my mind is to add one method to fetch the update TtlProvider in OperationCallbacks.

mukund-thakur · 2020-07-20T18:01:17Z

Tested raw and guarded config in ap-south-1 bucket. All good.

bgaborg

LGTM overall, I'll run the tests against ireland and give it another look.
I like the fact that we factor out logic from S3AFs.

bgaborg

tested against ireland without errors
+1 from me, let's wait if @steveloughran has any comments

steveloughran

Like the overall design. I'd like to keep the OperationCallbacks separate and exclusive to rename/delete, with the new ListingCallbacks having all operations and accessors needed for listing. And both to stay out of StoreContext.

IMO StoreContext should be a minimal view of the store, and operations manipulating the state should be given their own interface to do so. Rename and Delete share one because they are so close. (list, copy, delete). If we put everything into StoreContext we've just recreated S3AFileSystem again with all its interdependencies

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/OperationCallbacks.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/StoreContext.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Listing.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Listing.java

steveloughran

this is good. Just reinstate the class as an AbstractStoreOperation just so that if we ever add more to the base class, it will get picked up

steveloughran · 2020-08-03T20:06:40Z

yetus doesn't seem to be live right now, so we don't have a final check. But some of its earlier comments are still there by the look of things, especially

Lines that start with ????? in the ASF License  report indicate files that do not have an Apache license header:
 !????? /home/jenkins/jenkins-home/workspace/hadoop-multibranch_PR-2148/src/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/ListingOperationCallbacks.java

Can you fix that? All the checkstyles seem to have been dealt with.

+1 pending the license

hadoop-yetus · 2020-08-04T06:41:23Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 13s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 1s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 2 new or modified test files.
		_ trunk Compile Tests _
+1 💚	mvninstall	19m 35s	trunk passed
+1 💚	compile	0m 37s	trunk passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚	compile	0m 32s	trunk passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+1 💚	checkstyle	0m 26s	trunk passed
+1 💚	mvnsite	0m 41s	trunk passed
+1 💚	shadedclient	14m 34s	branch has no errors when building and testing our client artifacts.
+1 💚	javadoc	0m 20s	trunk passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚	javadoc	0m 27s	trunk passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+0 🆗	spotbugs	1m 0s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	0m 58s	trunk passed
		_ Patch Compile Tests _
+1 💚	mvninstall	0m 33s	the patch passed
+1 💚	compile	0m 35s	the patch passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚	javac	0m 35s	the patch passed
+1 💚	compile	0m 28s	the patch passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+1 💚	javac	0m 28s	the patch passed
-0 ⚠️	checkstyle	0m 19s	hadoop-tools/hadoop-aws: The patch generated 2 new + 13 unchanged - 0 fixed = 15 total (was 13)
+1 💚	mvnsite	0m 33s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	shadedclient	13m 52s	patch has no errors when building and testing our client artifacts.
+1 💚	javadoc	0m 17s	the patch passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
-1 ❌	javadoc	0m 23s	hadoop-tools_hadoop-aws-jdkPrivateBuild-1.8.0_252-8u252-b09-1~~18.04-b09 with JDK Private Build-1.8.0_252-8u252-b09-1~~18.04-b09 generated 2 new + 4 unchanged - 0 fixed = 6 total (was 4)
-1 ❌	findbugs	1m 6s	hadoop-tools/hadoop-aws generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0)
		_ Other Tests _
+1 💚	unit	1m 29s	hadoop-aws in the patch passed.
-1 ❌	asflicense	0m 29s	The patch generated 1 ASF License warnings.
		61m 25s

Reason	Tests
FindBugs	module:hadoop-tools/hadoop-aws
	Unused field:StoreContextBuilder.java
	Unused field:StoreContextBuilder.java

Subsystem	Report/Notes
Docker	ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2148/7/artifact/out/Dockerfile
GITHUB PR	#2148
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname	Linux 7d5540d756f6 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/hadoop.sh
git revision	trunk / `ab2b3df`
Default Java	Private Build-1.8.0_252-8u252-b09-1~18.04-b09
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_252-8u252-b09-1~18.04-b09
checkstyle	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2148/7/artifact/out/diff-checkstyle-hadoop-tools_hadoop-aws.txt
javadoc	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2148/7/artifact/out/diff-javadoc-javadoc-hadoop-tools_hadoop-aws-jdkPrivateBuild-1.8.0_252-8u252-b09-1~18.04-b09.txt
findbugs	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2148/7/artifact/out/new-findbugs-hadoop-tools_hadoop-aws.html
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2148/7/testReport/
asflicense	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2148/7/artifact/out/patch-asflicense-problems.txt
Max. process+thread count	414 (vs. ulimit of 5500)
modules	C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2148/7/console
versions	git=2.17.1 maven=3.6.0 findbugs=4.0.6
Powered by	Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

mukund-thakur · 2020-08-04T07:30:41Z

Not fixing first checkstyle here https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2148/7/artifact/out/patch-asflicense-problems.txt as the line length is only 82. Cutting it will make the side by side review difficult.

hadoop-yetus · 2020-08-04T08:29:17Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 3s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 2 new or modified test files.
		_ trunk Compile Tests _
-1 ❌	mvninstall	0m 15s	root in trunk failed.
+1 💚	compile	0m 34s	trunk passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚	compile	0m 28s	trunk passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+1 💚	checkstyle	0m 18s	trunk passed
+1 💚	mvnsite	0m 30s	trunk passed
+1 💚	shadedclient	16m 27s	branch has no errors when building and testing our client artifacts.
+1 💚	javadoc	0m 15s	trunk passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚	javadoc	0m 22s	trunk passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+0 🆗	spotbugs	0m 56s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	0m 55s	trunk passed
		_ Patch Compile Tests _
+1 💚	mvninstall	0m 33s	the patch passed
+1 💚	compile	0m 33s	the patch passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚	javac	0m 33s	the patch passed
+1 💚	compile	0m 26s	the patch passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+1 💚	javac	0m 26s	the patch passed
-0 ⚠️	checkstyle	0m 17s	hadoop-tools/hadoop-aws: The patch generated 1 new + 13 unchanged - 0 fixed = 14 total (was 13)
+1 💚	mvnsite	0m 30s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	shadedclient	15m 36s	patch has no errors when building and testing our client artifacts.
+1 💚	javadoc	0m 15s	the patch passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
-1 ❌	javadoc	0m 21s	hadoop-tools_hadoop-aws-jdkPrivateBuild-1.8.0_252-8u252-b09-1~~18.04-b09 with JDK Private Build-1.8.0_252-8u252-b09-1~~18.04-b09 generated 2 new + 4 unchanged - 0 fixed = 6 total (was 4)
+1 💚	findbugs	1m 2s	the patch passed
		_ Other Tests _
+1 💚	unit	1m 10s	hadoop-aws in the patch passed.
+1 💚	asflicense	0m 27s	The patch does not generate ASF License warnings.
		44m 18s

Subsystem	Report/Notes
Docker	ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2148/8/artifact/out/Dockerfile
GITHUB PR	#2148
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname	Linux 5d4718ffb6e4 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/hadoop.sh
git revision	trunk / `ab2b3df`
Default Java	Private Build-1.8.0_252-8u252-b09-1~18.04-b09
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_252-8u252-b09-1~18.04-b09
mvninstall	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2148/8/artifact/out/branch-mvninstall-root.txt
checkstyle	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2148/8/artifact/out/diff-checkstyle-hadoop-tools_hadoop-aws.txt
javadoc	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2148/8/artifact/out/diff-javadoc-javadoc-hadoop-tools_hadoop-aws-jdkPrivateBuild-1.8.0_252-8u252-b09-1~18.04-b09.txt
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2148/8/testReport/
Max. process+thread count	428 (vs. ulimit of 5500)
modules	C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2148/8/console
versions	git=2.17.1 maven=3.6.0 findbugs=4.0.6
Powered by	Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

steveloughran

+1, approved

mukund-thakur · 2020-08-04T15:03:55Z

Thanks

Contributed by Mukund Thakur. Change-Id: I79160b236a92fdd67565a4b4974f1862e600c210

aajisaka · 2020-08-05T08:35:46Z

Hi @steveloughran and @mukund-thakur

mvn javadoc:javadoc fails after this commit.
Filed https://issues.apache.org/jira/browse/HADOOP-17186

mukund-thakur · 2020-08-05T09:45:43Z

I was ignoring java doc issues because of https://issues.apache.org/jira/browse/HADOOP-17091. Looks like it got fixed recently. Sorry.
I will create a small patch and fix https://issues.apache.org/jira/browse/HADOOP-17186
Thanks for pointing out.

…#2148) Contributed by Mukund Thakur. Conflicts: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java Change-Id: Idc9ac4848b857362dee5574a912e4f36e586091e

mukund-thakur added 2 commits July 16, 2020 15:21

HADOOP-17131 Moving listing to use operation callback

315aa43

Moving listing methods to Listing class for better abstraction

3a70b65

Adding ListingOperationCallbacks interface for list ops

bda8f84

bgaborg self-requested a review July 28, 2020 10:17

bgaborg reviewed Jul 29, 2020

View reviewed changes

bgaborg approved these changes Jul 29, 2020

View reviewed changes

steveloughran requested changes Jul 29, 2020

View reviewed changes

Review comments

cd1b0a7

steveloughran reviewed Aug 1, 2020

View reviewed changes

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Listing.java Outdated Show resolved Hide resolved

steveloughran requested changes Aug 1, 2020

View reviewed changes

Final touch and checkstyle

2d5b331

mukund-thakur requested a review from steveloughran August 3, 2020 12:41

license and findbugs

d625e94

apache deleted a comment from hadoop-yetus Aug 4, 2020

steveloughran changed the title ~~HADOOP-17131 Moving listing to use operation callback~~ HADOOP-17131. Refactor S3A Listing code for better isolation. Aug 4, 2020

steveloughran approved these changes Aug 4, 2020

View reviewed changes

steveloughran merged commit 8fd4f54 into apache:trunk Aug 4, 2020

asfgit pushed a commit that referenced this pull request Aug 4, 2020

HADOOP-17131. Refactor S3A Listing code for better isolation. (#2148)

251d2d1

Contributed by Mukund Thakur. Change-Id: I79160b236a92fdd67565a4b4974f1862e600c210

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HADOOP-17131. Refactor S3A Listing code for better isolation. #2148

HADOOP-17131. Refactor S3A Listing code for better isolation. #2148

mukund-thakur commented Jul 16, 2020

mukund-thakur commented Jul 17, 2020

mukund-thakur commented Jul 20, 2020

bgaborg left a comment

bgaborg left a comment

steveloughran left a comment

steveloughran left a comment

steveloughran commented Aug 3, 2020

hadoop-yetus commented Aug 4, 2020

mukund-thakur commented Aug 4, 2020

hadoop-yetus commented Aug 4, 2020

steveloughran left a comment

mukund-thakur commented Aug 4, 2020

aajisaka commented Aug 5, 2020

mukund-thakur commented Aug 5, 2020

HADOOP-17131. Refactor S3A Listing code for better isolation. #2148

HADOOP-17131. Refactor S3A Listing code for better isolation. #2148

Conversation

mukund-thakur commented Jul 16, 2020

mukund-thakur commented Jul 17, 2020

mukund-thakur commented Jul 20, 2020

bgaborg left a comment

Choose a reason for hiding this comment

bgaborg left a comment

Choose a reason for hiding this comment

steveloughran left a comment

Choose a reason for hiding this comment

steveloughran left a comment

Choose a reason for hiding this comment

steveloughran commented Aug 3, 2020

hadoop-yetus commented Aug 4, 2020

mukund-thakur commented Aug 4, 2020

hadoop-yetus commented Aug 4, 2020

steveloughran left a comment

Choose a reason for hiding this comment

mukund-thakur commented Aug 4, 2020

aajisaka commented Aug 5, 2020

mukund-thakur commented Aug 5, 2020