Improve the local cache fallback behavior for corrupted pages #18498
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
beinan
force-pushed
the
implement_fallback_for_data_corruption
branch
3 times, most recently
from
0d4b82f to
c715238
Compare
beinan
force-pushed
the
implement_fallback_for_data_corruption
branch
from
c715238 to
9b39619
Compare
JiamingMai
reviewed
Feb 4, 2024
| @@ -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.
Do we need to delete the metadata as well?
JiamingMai
reviewed
Feb 4, 2024
| 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.
Do we need to delete the metadata as well?
JiamingMai
reviewed
Feb 4, 2024
Comment on lines
+586
to
+601
| @Test | ||
| public void testPageDataFileCorrupted() throws Exception | ||
| { | ||
| int pages = 10; | ||
| int fileSize = mPageSize * pages; | ||
| byte[] testData = BufferUtils.getIncreasingByteArray(fileSize); | ||
| ByteArrayCacheManager manager = new ByteArrayCacheManager(); | ||
| //by default local cache fallback is not enabled, the read should fail for any error | ||
| LocalCacheFileInStream streamWithOutFallback = setupWithSingleFile(testData, manager); | ||
|
|
||
| sConf.set(PropertyKey.USER_CLIENT_CACHE_FALLBACK_ENABLED, true); | ||
| LocalCacheFileInStream streamWithFallback = setupWithSingleFile(testData, manager); | ||
| Assert.assertEquals(100, streamWithFallback.positionedRead(0, new byte[10], 100, 100)); | ||
| Assert.assertEquals(1, | ||
| MetricsSystem.counter(MetricKey.CLIENT_CACHE_POSITION_READ_FALLBACK.getName()).getCount()); | ||
| } |
There was a problem hiding this comment.
Is this method duplicated with the method testPositionReadFallBack()?
There was a problem hiding this comment.
Ahhhh, I see, I got your point, yes, this test is covered by testPositionReadFallBack. I will remove this test case. Thanks!
JiamingMai
approved these changes
Feb 4, 2024
|
alluxio-bot, merge this please. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Throw a PageCorruptedException when the length of page inconsistent with the metadata
Do our best efforts to delete the corrupted page file
Reset the offset of buffer when we found the data has been corrupted to avoid ArrayOutOfBound exception.
Why are the changes needed?
We found the cache make presto keep failing when some of the page file got corrupted
Does this PR introduce any user facing changes?
No