Skip to content
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.

Sign up for GitHub

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

Jump to bottom

HADOOP-17301. ABFS: Fix bug introduced in HADOOP-16852 which reports read-ahead error back #2369

Merged
merged 3 commits into from
Oct 13, 2020
Merged

HADOOP-17301. ABFS: Fix bug introduced in HADOOP-16852 which reports read-ahead error back #2369

merged 3 commits into from
Oct 13, 2020

Conversation

snvijaya
Copy link
Contributor

@snvijaya snvijaya commented Oct 8, 2020

When reads done by readahead buffers failed, the exceptions where dropped and the failure was not getting reported to the calling app.
Jira HADOOP-16852: Report read-ahead error back
tried to handle the scenario by reporting the error back to calling app. But the commit has introduced a bug which can lead to ReadBuffer being injected into read completed queue twice when it has finished the store operation.

Additionally, in a scenario where all readahead buffers are exhausted and the buffer chosen to evict is one which is failed read, there is no buffer returned for other reads to use. But successful eviction leads the queuing logic to determine there is a free buffer and while fetching the buffer index from free list, can lead to EmptyStack exceptions.

This PR fixes both these issues and also has added test checks for both scenarios.

@snvijaya
Copy link
Contributor Author

snvijaya commented Oct 8, 2020

Tests were run on accounts on East US 2 region.

HNS-OAuth

	[INFO] Results:
	[INFO] 
	[INFO] Tests run: 88, Failures: 0, Errors: 0, Skipped: 0
	[INFO] Results:
	[INFO] 
	[WARNING] Tests run: 457, Failures: 0, Errors: 0, Skipped: 66
	[INFO] Results:
	[INFO] 
	[WARNING] Tests run: 208, Failures: 0, Errors: 0, Skipped: 141

HNS-SharedKey

	[INFO] Results:
	[INFO] 
	[INFO] Tests run: 88, Failures: 0, Errors: 0, Skipped: 0
	[INFO] Results:
	[INFO] 
	[WARNING] Tests run: 457, Failures: 0, Errors: 0, Skipped: 24
	[INFO] Results:
	[INFO] 
	[WARNING] Tests run: 208, Failures: 0, Errors: 0, Skipped: 24

NonHNS-SharedKey

	[INFO] Results:
	[INFO] 
	[INFO] Tests run: 88, Failures: 0, Errors: 0, Skipped: 0
	[INFO] Results:
	[INFO] 
	[WARNING] Tests run: 457, Failures: 0, Errors: 0, Skipped: 245
	[INFO] Results:
	[INFO] 
	[WARNING] Tests run: 208, Failures: 0, Errors: 0, Skipped: 24

@apache apache deleted a comment from hadoop-yetus Oct 8, 2020
steveloughran
steveloughran requested changes Oct 8, 2020
View reviewed changes
Copy link
Contributor

@steveloughran steveloughran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some minor tuning but otherwise good to go in

...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManager.java Outdated Show resolved Hide resolved
...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManager.java Show resolved Hide resolved
...s/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsInputStream.java Outdated
@@ -49,6 +49,7 @@
private static final int TWO_KB = 2 * 1024;
private static final int THREE_KB = 3 * 1024;
private static final int REDUCED_READ_BUFFER_AGE_THRESHOLD = 3000; // 3 sec
private static final int INCREASED_READ_BUFFER_AGE_THRESHOLD = 30000; // 30 sec
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use 30_000 style integer

...s/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsInputStream.java Outdated Show resolved Hide resolved
...s/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsInputStream.java Outdated Show resolved Hide resolved
@hadoop-yetus
Copy link

🎊 +1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 29s 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 💚 0m 0s test4tests The patch appears to include 1 new or modified test files.
_ trunk Compile Tests _
+1 💚 mvninstall 29m 46s trunk passed
+1 💚 compile 0m 37s trunk passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 compile 0m 33s trunk passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+1 💚 checkstyle 0m 27s trunk passed
+1 💚 mvnsite 0m 38s trunk passed
+1 💚 shadedclient 15m 7s branch has no errors when building and testing our client artifacts.
+1 💚 javadoc 0m 30s trunk passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javadoc 0m 29s trunk passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+0 🆗 spotbugs 0m 58s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 0m 55s trunk passed
_ Patch Compile Tests _
+1 💚 mvninstall 0m 29s the patch passed
+1 💚 compile 0m 29s the patch passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javac 0m 29s the patch passed
+1 💚 compile 0m 26s the patch passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+1 💚 javac 0m 26s the patch passed
+1 💚 checkstyle 0m 16s the patch passed
+1 💚 mvnsite 0m 27s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 shadedclient 14m 4s patch has no errors when building and testing our client artifacts.
+1 💚 javadoc 0m 25s the patch passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javadoc 0m 24s the patch passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+1 💚 findbugs 0m 57s the patch passed
_ Other Tests _
+1 💚 unit 1m 27s hadoop-azure in the patch passed.
+1 💚 asflicense 0m 34s The patch does not generate ASF License warnings.
71m 41s
Subsystem Report/Notes
Docker ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2369/3/artifact/out/Dockerfile
GITHUB PR #2369
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux 61d0a03f4d37 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/bin/hadoop.sh
git revision trunk / b3786d6
Default Java Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
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_265-8u265-b01-0ubuntu2~18.04-b01
Test Results https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2369/3/testReport/
Max. process+thread count 430 (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-2369/3/console
versions git=2.17.1 maven=3.6.0 findbugs=4.1.3
Powered by Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

@snvijaya
Copy link
Contributor Author

Test results from accounts on East US 2 region:

NON-HNS:

SharedKey:
	[INFO] Results:
	[INFO] 
	[INFO] Tests run: 88, Failures: 0, Errors: 0, Skipped: 0
	[INFO] Results:
	[INFO] 
	[WARNING] Tests run: 457, Failures: 0, Errors: 0, Skipped: 245
	[INFO] Results:
	[INFO] 
	[WARNING] Tests run: 208, Failures: 0, Errors: 0, Skipped: 24

HNS:

SharedKey:
	[INFO] Results:
	[INFO] 
	[INFO] Tests run: 88, Failures: 0, Errors: 0, Skipped: 0
	[INFO] Results:
	[INFO] 
	[WARNING] Tests run: 457, Failures: 0, Errors: 0, Skipped: 24
	[INFO] Results:
	[INFO] 
	[WARNING] Tests run: 208, Failures: 0, Errors: 0, Skipped: 24

OAuth:
	[INFO] Results:
	[INFO] 
	[INFO] Tests run: 88, Failures: 0, Errors: 0, Skipped: 0
	[INFO] Results:
	[INFO] 
	[WARNING] Tests run: 457, Failures: 0, Errors: 0, Skipped: 66
	[INFO] Results:
	[INFO] 
	[WARNING] Tests run: 208, Failures: 0, Errors: 0, Skipped: 141

@snvijaya
Copy link
Contributor Author

@steveloughran - Thanks for your review. I have updated this PR with the suggestions.
Kindly request your review.

@apache apache deleted a comment from hadoop-yetus Oct 13, 2020
@steveloughran steveloughran merged commit c4fff74 into apache:trunk Oct 13, 2020
@steveloughran
Copy link
Contributor

+1, merged to trunk.

@snvijaya -before I backport it, can you do a cherrypick of this into branch-3.3 and verify all the tests are good? thanks

@steveloughran steveloughran mentioned this pull request Oct 14, 2020
Merged
snvijaya added a commit to snvijaya/hadoop that referenced this pull request Oct 14, 2020
@snvijaya
HADOOP-17301. ABFS: read-ahead error reporting breaks buffer manageme…
d65a694 
…nt (apache#2369)


Fixes read-ahead buffer management issues introduced by HADOOP-16852,
 "ABFS: Send error back to client for Read Ahead request failure".

Contributed by Sneha Vijayarajan
@snvijaya
Copy link
Contributor Author

+1, merged to trunk.

@snvijaya -before I backport it, can you do a cherrypick of this into branch-3.3 and verify all the tests are good? thanks

Thanks a lot @steveloughran . I have created a backport PR for this change. Tests went well, have pasted the result in the backport PR.
PR: #2384

asfgit pushed a commit that referenced this pull request Oct 14, 2020
@snvijaya @ThomasMarquardt
HADOOP-17301. ABFS: read-ahead error reporting breaks buffer manageme…
d5b4d04 
…nt (#2369)


Fixes read-ahead buffer management issues introduced by HADOOP-16852,
 "ABFS: Send error back to client for Read Ahead request failure".

Contributed by Sneha Vijayarajan
snvijaya added a commit to snvijaya/hadoop that referenced this pull request May 23, 2021
@snvijaya
HADOOP-17301. ABFS: read-ahead error reporting breaks buffer manageme…
cd3691e 
…nt (apache#2369)

Fixes read-ahead buffer management issues introduced by HADOOP-16852,
 "ABFS: Send error back to client for Read Ahead request failure".

Contributed by Sneha Vijayarajan
jojochuang pushed a commit to jojochuang/hadoop that referenced this pull request May 23, 2023
@snvijaya
CDPD-25024. HADOOP-17301. ABFS: read-ahead error reporting breaks buf…
d348ef1 
…fer management (apache#2369)

Fixes read-ahead buffer management issues introduced by HADOOP-16852,
 "ABFS: Send error back to client for Read Ahead request failure".

Contributed by Sneha Vijayarajan

Backport Notes:
  Mockito backport issues

Change-Id: I1a45c3c3ba5258ab61d495392a2821f35748d6a7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steveloughran steveloughran steveloughran requested changes

@bilaharith bilaharith bilaharith approved these changes

@sumangala-patki sumangala-patki sumangala-patki approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@snvijaya @hadoop-yetus @steveloughran @bilaharith @sumangala-patki