[Remote Compaction] Load latest options from OPTIONS file in Remote host #13025
Conversation
|
@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@jaykorean has updated the pull request. You must reimport the pull request before landing. |
|
@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| @@ -28,6 +29,13 @@ CompactionJob::ProcessKeyValueCompactionWithCompactionService( | |||
|
|
|||
| const Compaction* compaction = sub_compact->compaction; | |||
| CompactionServiceInput compaction_input; | |||
| Status s = GetLatestOptionsFileName(dbname_, db_options_.env, | |||
| &compaction_input.options_file); | |||
| if (!s.ok()) { | |||
There was a problem hiding this comment.
Can the file be deleted before remote instance reads it?
There was a problem hiding this comment.
Ah good question. Yes, if options change between this line and remote worker trying to read the file, the file can be missing and we will have failure.
(Looks like we are purging the obsolete options file immediately after renaming the temp file.)
Lines 5443 to 5450 in 71e38db
Once the file is read from the remote worker, we don't need the file to exist anymore while the job is running. I'm wondering how often we could run into this issue in production and if this is something that we can tolerate. To even lower the chance, I think we can keep the last N OPTIONS file, which can be controlled behind an option. (for the users who change options often)
There was a problem hiding this comment.
Could you simply retry? The OPTIONS file should only change when SetDBOptions is called, which should be pretty rare.
There was a problem hiding this comment.
Could you simply retry?
Which one? I am not sure if retry on either of them would actually be helpful.
- For
GetLatestOptionsFileName()in the primary host, I believe that it only fails if no OPTIONS file is found at all. - For
LoadOptionsFromFile(file_name_from_the_payload)in the remote worker, retry won't succeed if the file is gone.
Maybe I'm wondering if we should just call LoadLatestOptions() from the remote worker and always uses the latest options instead of passing the options file name over the payload.
There was a problem hiding this comment.
Just updated the PR to always load the latest options file instead of passing the file name in the payload. With a single retry, compaction failure due to missing OPTION file should be extremely rare.
|
@jaykorean has updated the pull request. You must reimport the pull request before landing. |
jaykorean
changed the title
|
@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| // In a very rare scenario, loading options may fail if the options changed by | ||
| // the primary host at the same time. Just retry once for now. | ||
| if (!s.ok()) { | ||
| s = LoadLatestOptions(config_options, name, &db_options, |
There was a problem hiding this comment.
Actually Logger is not available at this point before the options is loaded, so I didn't bother creating a tmp logger just for this. We can consider adding it later if really needed.
|
@jaykorean has updated the pull request. You must reimport the pull request before landing. |
|
@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@jaykorean merged this pull request in 5f4a8c3. |
Summary
We've been serializing and deserializing DBOptions and CFOptions (and other CF into) as part of
CompactionServiceInput. These are all readily available in the OPTIONS file and the remote worker can read the OPTIONS file to obtain the same information. This helps reducing the size of payload significantly.In a very rare scenario if the OPTIONS file is purged due to options change by primary host at the same time while the remote host is loading the latest options, it may fail. In this case, we just retry once.
This also solves the problem where we had to open the default CF with the CFOption from another CF if the remote compaction is for a non-default column family. (TODO comment in /db_impl_secondary.cc)
Test Plan
Unit Tests
Also tested with Meta's internal Offload Infra