-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
HADOOP-18756. S3A prefetch - CachingBlockManager to use AtomicBoolean for closed flag #5718
Conversation
Requires rebase after PR #5675 |
💔 -1 overall
This message was automatically generated. |
@steveloughran i resolved merge conflicts but github has some incidents going on, will likely take some time |
💔 -1 overall
This message was automatically generated. |
This is now resolved |
|
if (numFilesDeleted > 0) { | ||
LOG.info("Deleted {} cache files", numFilesDeleted); | ||
if (numFilesDeleted > 0) { | ||
LOG.info("Deleted {} cache files", numFilesDeleted); |
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.
downgrade to debug and remove the if() test; we can log the zero count too
|
||
closed = true; | ||
if (closed.compareAndSet(false, true)) { | ||
LOG.info(getStats()); |
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.
could we log this at debug; prefetching is fairly noisy right now
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.
agree, it is noisy, let me downgrade it to debug
💔 -1 overall
This message was automatically generated. |
ok. looks good. final check: which s3 store did you run the final build against and what were the settings |
my sincere apologies, i thought the addendum was small enough so didn't re-test the whole suite (only tested against ITestS3APrefetchingInputStream and ITestS3APrefetchingCacheFiles). |
re-running this with the addendum change right now. will post here as soon as i get the full results. |
test run is complete against |
aah, but you didn't even say you'd tested those two... anyway, thanks for the full run, always good to keep that coverage up. |
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.
ok, comment on one more log. i just don't want prefetching to be excessively noisy here
prefetchingStatistics.blockRemovedFromFileCache(); | ||
numFilesDeleted++; | ||
} catch (IOException e) { | ||
LOG.error("Failed to delete cache file {}", entry.path, 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.
ok, so this is moving from debug to error. why so?
if we do want to print this, it's not an error. at worst a warn.
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.
sure let me make it at least warn, this is likely going to be rare event and highlighting it would be helpful debugging stale file records in future.
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.
with this new addendum (log level change: ERROR to WARN), i re-ran the test suite again (without scale profile, since the addendum change is trivial)
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.
re-tested the latest state of the PR with mvn clean verify -Dparallel-tests -DtestsThreadCount=8 -Dscale -Dprefetch
against us-west-2
💔 -1 overall
This message was automatically generated. |
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
(if I do ever see a log with too many of the warnings about deletion problems, we will have to revisit the log level)
… for closed flag (#5718) Contributed by Viraj Jasani
Jira: HADOOP-18756
Requires rebase after PR #5675