-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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
[#8859] Fix duplicate mapped file in mutil commitlog store path mode. #8897
base: develop
Are you sure you want to change the base?
Conversation
@@ -76,13 +82,14 @@ public MappedFile putRequestAndReturnMappedFile(String nextFilePath, String next | |||
canSubmitRequests--; | |||
} | |||
|
|||
String nextNextFileName = nextNextFilePath.substring(nextNextFilePath.lastIndexOf("/") + 1); |
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.
It is necessary to use File.separator, as the file path separator on other OS may not be '/'.
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.
Okay, fixed 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.
The UTs also need to be modified accordingly.
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.
Sorry, it was my oversight. I have modified it as required. Please review it again. Thank you.
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.
Sorry, it was my oversight. I have modified it as required. Please review it again. Thank you.
By the way, is there any compatibility issue here? From the code, it seems that there isn't.
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 you mean file path separator in test case? I also have this doubts but the other use cases I refer to in this file are like this.
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.
Sorry, there are indeed some compatibility issues in my UTs, I have located the problem in the workflows. I will provide a new patch after completing the test in my repository later.
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.
a new patch after completing the test in my repository
Fixed compatibility issue.
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.
It seems like some errors blocked the workflow checks, but I don't have permission to stop or rerun it.
This pr may need someone review it again, thanks ~ |
Which Issue(s) This PR Fixes
Fixes #8859
Brief Description
I changed the key of the requestTable in the AllocateMappedFileService class from the FilePath to the FileName(this must be the only one), and then added a getCachedMappedFilePath method to provide to the MultiPathMappedFileQueue to check whether the MappedFile of a specified offset MappedFile create request has been submitted, and if so, obtain the cached FilePath, otherwise a new path is generated based on the previous rules.
How Did You Test This Change?
I make a unit test to recurrence this case in testUniqueNextNextMappedFile().
In the previous implementation, the results obtained were as shown in the picture below, the MappedFile 00000000000000001024 repeatedly appear in directory a and directory b.
This problem was solved in the new implementation and the result is as follows: