-
Notifications
You must be signed in to change notification settings - Fork 6.6k
Resuming and persisting subcompaction progress in CompactionJob #13983
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
base: main
Are you sure you want to change the base?
Conversation
214b20a
to
359598f
Compare
} | ||
|
||
// LIMITATION: Persisting compaction progress with timestamp | ||
// is not supported since the feature of persisting tiemstamp of the key in |
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.
nit: tiemstamp --> timestamp
assert(input_iter); | ||
|
||
Status status = | ||
MaybeResumeSubcompactionProgressOnInputIterator(sub_compact, input_iter); |
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.
Could we add an option to not use this feature?
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.
Hi - it's in MaybeResumeSubcompactionProgressOnInputIterator where there is no progress. The actual option is in OpenAndCompact option and if false, will pass in no progress in the third PR here c6d04f5#diff-17fbdec07244b1f07d1a4e5aed0a6feecf4474d20b3129818c10fc0ff9f3d547R1400
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 was more concerned about the scenario in case we find any issue when this PR makes to Prod before your third PR.
The only way to mitigate it would be reverting this PR. I was wondering if we could completely put from line 1725 to 1734 under a db or cf-wide check - resumable_compaction_enabled
or something like that.
And if the OpenAndCompactOption::resume_compaction
is true while this db or cf-wide check is false, we either ignore it or return Status::Unsupported()
if (status.IsIncomplete()) { | ||
input_iter->SeekToFirst(); | ||
} else if (!status.ok()) { | ||
sub_compact->status = status; |
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.
If we had any IO issue while resuming the compaction or even tmp output files are corrupted for any reason, I'm wondering if it's better to fail the current compaction with Corruption(), or still want to re-try from the beginning.
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.
Good question - how to set a good contract and specify in the OpenAndCompacition() API when not to retry. I haven't written that yet in my third PR and want to wait until we chat tmr. My take is we only resume when OpenAndCompact() crashes, returns Shutdown or ManualCompactionPaused but no other status. The Shutdown and ManualCompactionPaused after this #13891 should not override other non-ok status so should be good to rely on. These two error status are also rare to expand their use case within RocksDB.
359598f
to
815d7f3
Compare
Context/Summary:
This is stacked on top of #13928.
Flow of resuming: DB::OpenAndCompact() -> Compaction progress file -> SubcompactionProgress -> CompactionJob
Flow of persistence: CompactionJob -> SubcompactionProgress -> Compaction progress file -> DB that is called with OpenAndCompact()
This PR focuses on SubcompactionProgress -> CompactionJob and CompactionJob -> SubcompactionProgress -> Compaction progress file. For now only single subcompaction is supported as OpenAndCompact() does not partition compaction anyway.
The actual triggering of progress persistence and resuming (i.e, integration) is through DB::OpenAndCompact() in the upcoming PR.
Resume Flow
Persistence Strategy
Test plan: