Rely on PurgeObsoleteFiles Only for Options file clean up when remote compaction is enabled #13139
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
jaykorean
changed the title
jaykorean
force-pushed
the
preserve_options_file_fix
branch
from
f1c4ae5
to
8df645f
Compare
|
@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
cbi42
approved these changes
Nov 15, 2024
|
@jaykorean merged this pull request in 3495c94. |
jaykorean
added a commit
that referenced
this pull request
Nov 18, 2024
… compaction is enabled (#13139) Summary: In PR #13074 , we added a logic to prevent stale OPTIONS file from getting deleted by `PurgeObsoleteFiles()` if the OPTIONS file is being referenced by any of the scheduled the remote compactions. `PurgeObsoleteFiles()` was not the only place that we were cleaning up the old OPTIONS file. We've been also directly cleaning up the old OPTIONS file as part of `SetOptions()`: `RenameTempFileToOptionsFile()` -> `DeleteObsoleteOptionsFiles()` unless FileDeletion is disabled. This was not caught by the UnitTest because we always preserve the last two OPTIONS file. A single call of `SetOptions()` was not enough to surface this issue in the previous PR. To keep things simple, we are just skipping the old OPTIONS file clean up in `RenameTempFileToOptionsFile()` if remote compaction is enabled. We let `PurgeObsoleteFiles()` clean up the old options file later after the compaction is done. Pull Request resolved: #13139 Test Plan: Updated UnitTest to reproduce the scenario. It's now passing with the fix. ``` ./compaction_service_test --gtest_filter="*PreservedOptionsRemoteCompaction*" ``` Reviewed By: cbi42 Differential Revision: D65974726 Pulled By: jaykorean fbshipit-source-id: 1907e8450d2ccbb42a93084f275e666648ef5b8c
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.
Summary
In PR #13074 , we added a logic to prevent stale OPTIONS file from getting deleted by
PurgeObsoleteFiles()if the OPTIONS file is being referenced by any of the scheduled the remote compactions.PurgeObsoleteFiles()was not the only place that we were cleaning up the old OPTIONS file. We've been also directly cleaning up the old OPTIONS file as part ofSetOptions():RenameTempFileToOptionsFile()->DeleteObsoleteOptionsFiles()unless FileDeletion is disabled.This was not caught by the UnitTest because we always preserve the last two OPTIONS file. A single call of
SetOptions()was not enough to surface this issue in the previous PR.To keep things simple, we are just skipping the old OPTIONS file clean up in
RenameTempFileToOptionsFile()if remote compaction is enabled. We letPurgeObsoleteFiles()clean up the old options file later after the compaction is done.Test Plan
Updated UnitTest to reproduce the scenario. It's now passing with the fix.