-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HADOOP-18351. Reduce excess logging of errors during S3A prefetching reads #5274
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
Conversation
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Hey @steveloughran ! Can you please review it. Tested the logging of Errors by throwing errors in case of both prefetch and read. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, some minor nits to deal with and then it is good to go in.
It'll be interesting to see what happens in real use when we encounter errors here. With the prefetching things may surface sooner, but transient ones may now be recovered from better.
readBlock(data, false, BufferData.State.BLANK); | ||
try { | ||
readBlock(data, false, BufferData.State.BLANK); | ||
} catch(Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a space between catch and "(", and only catch IOException
readBlock(data, false, BufferData.State.BLANK); | ||
} catch(Exception e) { | ||
String message = String.format("error reading block %s", data.getBlockNumber()); | ||
LOG.error(message, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the string formatting can go into the LOG.error() just like the ones below
Hey @steveloughran ! Made the small changes recommended by you and ran the tests again. Thanks. Please review it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
+1
will merge as soon as the yetus build is happy |
💔 -1 overall
This message was automatically generated. |
For test4tests, |
…reads (#5274) Contributed by Ankit Saurabh
…reads (#5274) Contributed by Ankit Saurabh
…reads (apache#5274) Contributed by Ankit Saurabh
Description of PR
Logged the errors at the info level with just the message. Added the stack at the Debug level. Not made any changes where error stack was not present and had only the message and parameters at Warn/INFO level.
Also, removed the case of multiple logging in case of prefetch. Moved the log one method above ReadBlock() (which was common for sync read thread and async prefetch thread) and logged it at ERROR level for the case of read.
How was this patch tested?
Logging levels of Error/info was tested locally by throwing errors in case of both cases of async prefetch and sync get calls for read separately. Then the unit test of ITestS3APrefetchingInputStream integration test ( testReadLargeFileFully) was used.
Finally done the integration test by
mvn -Dparallel-tests clean verify
in theeu-west-1
region.Following is the result of the tests -
[INFO] Results:
[INFO]
[WARNING] Tests run: 1154, Failures: 0, Errors: 0, Skipped: 182
[INFO]
[INFO]
[INFO] --- maven-failsafe-plugin:3.0.0-M1:integration-test (sequential-integration-tests) @ hadoop-aws ---
[INFO] Results:
[INFO]
[WARNING] Tests run: 124, Failures: 0, Errors: 0, Skipped: 84
[INFO]
[INFO]
[INFO] --- maven-enforcer-plugin:3.0.0:enforce (depcheck) @ hadoop-aws ---
[INFO]
[INFO] --- maven-failsafe-plugin:3.0.0-M1:verify (default-integration-test) @ hadoop-aws ---
[INFO]
[INFO] --- maven-failsafe-plugin:3.0.0-M1:verify (sequential-integration-tests) @ hadoop-aws ---
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 14:05 min
[INFO] Finished at: 2023-02-14T11:25:12Z
[INFO] Final Memory: 76M/1674M
[INFO] ------------------------------------------------------------------------
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?