-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix runtime stats when cache hit #18503
Fix runtime stats when cache hit #18503
Conversation
6fe43b3
to
c69d301
Compare
@@ -927,10 +929,20 @@ private int getPage(PageInfo pageInfo, int pageOffset, int bytesToRead, | |||
// data read from page store is inconsistent from the metastore | |||
LOG.error("Failed to read page {}: supposed to read {} bytes, {} bytes actually read", | |||
pageInfo.getPageId(), bytesToRead, ret); | |||
target.offset(originOffset); //reset the offset | |||
//best efforts to delete the corrupted file without acquire the write lock | |||
deletePage(pageInfo, false); |
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.
Do we need to delete the metadata as well?
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.
Deleting the metadata here will acquire more locks, which might cause dead lock. So I just delete the page file, and page store will re-cache this page in the next visit.
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.
I think it's a good call, probably we can have a further improvement in the near future. But just for now, ,et's merge this pr and stop the bleeding of the data corruption issue
LOG.error("Data corrupted page {} from pageStore", pageInfo.getPageId(), e); | ||
target.offset(originOffset); //reset the offset | ||
//best efforts to delete the corrupted file without acquire the write lock | ||
deletePage(pageInfo, false); |
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.
Do we need to delete the metadata as well?
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. Just left a comment. If we read a corrupted page, do we need to delete the metadata as well?
Hi @beinan , this fix works from our testing. Would it be possible to make it into version 308? |
c69d301
to
637364c
Compare
alluxio-bot, merge this please. |
What changes are proposed in this pull request?
Fix runtime stats when cache hit
Why are the changes needed?
We already count the runtime stats in local cache manager, but we didn't pass in a proper cache context
Does this PR introduce any user facing changes?
Please list the user-facing changes introduced by your change, including