HADOOP-17156. Purging the buffers associated with input streams during close() #3285

mukund-thakur · 2021-08-10T09:14:41Z

Ran all abfs tests. Don't see any failures.

Please treat this as initial patch to get a basic understanding for the thought process.

mukund-thakur · 2021-08-10T09:15:03Z

CC @steveloughran @mehakmeet @snvijaya

...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManager.java

steveloughran

LGTM, some clarification over concurrent access for tests, some minor nits (import etc)

steveloughran · 2021-08-10T10:10:09Z

...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManager.java

afraid if we need concurrency we should use Collections.synchronizedList()

...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManager.java

...adoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/ITestReadBufferManager.java

steveloughran · 2021-08-10T10:15:08Z

...adoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/ITestReadBufferManager.java

if you can parallelize this you can get a faster test setup, I think ITestS3AAssumedRole does that.

steveloughran · 2021-08-10T10:16:11Z

...adoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/ITestReadBufferManager.java

assertThat(list).hasSize(), as you do below.
could consider an AsserListEmpty(text, list) with that operation

steveloughran · 2021-08-10T10:18:45Z

...adoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/ITestReadBufferManager.java

can you close these always in a try/finally, just so if an assert fails things still get cleaned up

steveloughran · 2021-08-10T10:23:24Z

...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManager.java

nit, just make simple list. Now, small risk of that copy happening while something else changes. Now it is only test code, but could lead to intermittent test fails.

so: make these all synchronized; return List() and use ArrayList inside

mukund-thakur · 2021-08-11T11:46:25Z

Re-ran all tests including scale one using us-east-1 bucket. Don't see any failures.
Though there was an instance where new test failed. I think the reason for that tests running in parallel. Since ReadBufferManager is shared across InputStreams, there maybe a scenario where freeList, completedList may not be empty even after close calls are made in the new test.
Should we move this as a sequential tests?

steveloughran · 2021-08-19T14:13:51Z

parallel test runs execute in different processes; that is not the direct cause of test failure

Possible

state of remote store (including paths)
things just being slow on an overloaded machines, so cleanup happening earlier
state of recycled FS being such that this test case fails

…g close()

…ances in tests

mukund-thakur · 2021-08-20T06:19:53Z

Forced push since I had to do rebase.

hadoop-yetus · 2021-08-20T07:33:36Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 39s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 1s		codespell was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 1 new or modified test files.
			_ trunk Compile Tests _
+1 💚	mvninstall	31m 9s		trunk passed
+1 💚	compile	0m 41s		trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	compile	0m 37s		trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	checkstyle	0m 28s		trunk passed
+1 💚	mvnsite	0m 41s		trunk passed
+1 💚	javadoc	0m 34s		trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	0m 31s		trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	spotbugs	1m 1s		trunk passed
+1 💚	shadedclient	14m 11s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	0m 31s		the patch passed
+1 💚	compile	0m 31s		the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javac	0m 31s		the patch passed
+1 💚	compile	0m 27s		the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	javac	0m 27s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 18s		the patch passed
+1 💚	mvnsite	0m 31s		the patch passed
+1 💚	xml	0m 1s		The patch has no ill-formed XML file.
+1 💚	javadoc	0m 24s		the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	0m 21s		the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	spotbugs	1m 1s		the patch passed
+1 💚	shadedclient	14m 17s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	2m 12s		hadoop-azure in the patch passed.
+1 💚	asflicense	0m 34s		The patch does not generate ASF License warnings.
		72m 43s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3285/4/artifact/out/Dockerfile
GITHUB PR	#3285
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient codespell xml spotbugs checkstyle
uname	Linux edccda6a4b7a 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	dev-support/bin/hadoop.sh
git revision	trunk / `84179a3`
Default Java	Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3285/4/testReport/
Max. process+thread count	665 (vs. ulimit of 5500)
modules	C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3285/4/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

steveloughran

looks good; just add that little import nit and I'm happy,

+1 pending.

...adoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/ITestReadBufferManager.java

steveloughran

+1 from me

hadoop-yetus · 2021-09-07T06:56:25Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 56s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 1 new or modified test files.
			_ trunk Compile Tests _
+1 💚	mvninstall	33m 27s		trunk passed
+1 💚	compile	0m 38s		trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	compile	0m 31s		trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	checkstyle	0m 24s		trunk passed
+1 💚	mvnsite	0m 36s		trunk passed
+1 💚	javadoc	0m 28s		trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	0m 26s		trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	spotbugs	0m 59s		trunk passed
+1 💚	shadedclient	16m 52s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	0m 29s		the patch passed
+1 💚	compile	0m 31s		the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javac	0m 30s		the patch passed
+1 💚	compile	0m 25s		the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	javac	0m 25s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 16s		the patch passed
+1 💚	mvnsite	0m 29s		the patch passed
+1 💚	xml	0m 1s		The patch has no ill-formed XML file.
+1 💚	javadoc	0m 22s		the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	0m 20s		the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	spotbugs	1m 3s		the patch passed
+1 💚	shadedclient	16m 22s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	1m 55s		hadoop-azure in the patch passed.
+1 💚	asflicense	0m 29s		The patch does not generate ASF License warnings.
		78m 59s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3285/5/artifact/out/Dockerfile
GITHUB PR	#3285
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient codespell xml spotbugs checkstyle
uname	Linux a695f4c08547 4.15.0-143-generic #147-Ubuntu SMP Wed Apr 14 16:10:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `5066471`
Default Java	Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3285/5/testReport/
Max. process+thread count	522 (vs. ulimit of 5500)
modules	C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3285/5/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

… close() (#3285) Contributed By: Mukund Thakur

… close() (apache#3285) Contributed By: Mukund Thakur

…t streams in close() (apache#3285) Contributed By: Mukund Thakur Change-Id: Id50f1aac113f89c3957301266f3ef7ce3f09b698

steveloughran requested changes Aug 10, 2021

View reviewed changes

...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManager.java Outdated Show resolved Hide resolved

steveloughran requested changes Aug 10, 2021

View reviewed changes

mukund-thakur mentioned this pull request Aug 11, 2021

HADOOP-16852: Report read-ahead error back #1898

Merged

apache deleted a comment from hadoop-yetus Aug 19, 2021

mukund-thakur added 3 commits August 20, 2021 11:48

HADOOP-17156. Purging the buffers associated with input streams durin…

152dca0

…g close()

HADOOP-17156. Review comments

21bcb98

Moving new test to run in seperate jvm and creating new Azure FS inst…

84179a3

…ances in tests

mukund-thakur force-pushed the HADOOP-17156 branch from 94ae9dc to 84179a3 Compare August 20, 2021 06:19

mukund-thakur requested a review from steveloughran August 20, 2021 07:38

steveloughran requested changes Aug 24, 2021

View reviewed changes

...adoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/ITestReadBufferManager.java Outdated Show resolved Hide resolved

apache deleted a comment from hadoop-yetus Sep 6, 2021

steveloughran approved these changes Sep 6, 2021

View reviewed changes

HADOOP-17156 import new line

5066471

mukund-thakur force-pushed the HADOOP-17156 branch from 7c5b65e to 5066471 Compare September 7, 2021 05:28

mukund-thakur merged commit 9b8f81a into apache:trunk Sep 7, 2021

mukund-thakur added a commit that referenced this pull request Sep 7, 2021

HADOOP-17156. ABFS: Release the byte buffers held by input streams in…

3b1c594

… close() (#3285) Contributed By: Mukund Thakur

kiran-maturi pushed a commit to kiran-maturi/hadoop that referenced this pull request Nov 24, 2021

HADOOP-17156. ABFS: Release the byte buffers held by input streams in…

bfd42c4

… close() (apache#3285) Contributed By: Mukund Thakur

HADOOP-17156. Purging the buffers associated with input streams during close() #3285

HADOOP-17156. Purging the buffers associated with input streams during close() #3285

Uh oh!

Conversation

mukund-thakur commented Aug 10, 2021

Uh oh!

mukund-thakur commented Aug 10, 2021

Uh oh!

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

steveloughran Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steveloughran Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

steveloughran Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

steveloughran Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

steveloughran Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

mukund-thakur commented Aug 11, 2021

Uh oh!

steveloughran commented Aug 19, 2021

Uh oh!

mukund-thakur commented Aug 20, 2021

Uh oh!

hadoop-yetus commented Aug 20, 2021

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

hadoop-yetus commented Sep 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants