enlarge append recycle log threshold #331
Conversation
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
|
@LykxSassinator PTAL thanks~ |
Codecov ReportPatch coverage is
📢 Thoughts on this report? Let us know!. |
| @@ -241,7 +241,7 @@ impl Config { | |||
| if self.prefill_for_recycle && prefill_limit >= self.target_file_size.0 { | |||
There was a problem hiding this comment.
When prefill_limit is None, use recycle_capacity.
src/file_pipe_log/pipe_builder.rs
Outdated
| ); | ||
| let to_create = target.saturating_sub(self.recycled_files.len()); | ||
| let prefill_target = | ||
| std::cmp::min(self.cfg.prefill_capacity(), self.cfg.recycle_capacity()) |
There was a problem hiding this comment.
This is not the same as old code.
There was a problem hiding this comment.
Because the old code is not fit anymore after the recycle threshold is enlarged. From @LykxSassinator, the old logic is that prefill_capacity is the number of recycle files that should be pre-created even if there are already some append files; but the new logic changes the semantic to let prefill_capacity the total files number (recycle + append), I think this is more intuitive.
In the general case, we expect the total files size is around the size of purge threshold but not the recycle threshold ( 1.5 * purge_threshold), so it's better not to pre-create too many redundant files.
There was a problem hiding this comment.
we expect the total files size is around the size of purge threshold but not the recycle threshold ( 1.5 * purge_threshold)
I don't think so. We need to prepare the same amount of files as recycle-threshold. The condition where a large recycle-threshold is needed can also occur when raft-engine has just started.
There was a problem hiding this comment.
Ok, I changed the default prefill threshold to recycle-threshold.
There was a problem hiding this comment.
I think there is a bug in the old code that it only count the recycle file to create prefill files count. While with the default config there is no problem because it is equal to the recycle threshold. But if the prefill threshold is smaller than recycle threshold, I can cause raft-engine to create more files after restarting which is unexpacted. One of the log recycle test case failed after I exlarged the recycle threshold.
Signed-off-by: glorv <glorvs@163.com>
…e into recycle-threshold
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
src/engine.rs
Outdated
| @@ -2197,11 +2197,11 @@ pub(crate) mod tests { | |||
| assert_eq!(reserved_start_2, reserved_start_3); | |||
|
|
|||
| // Reuse all of reserved files. | |||
| for rid in 1..=50 { | |||
| for rid in 1..=75 { | |||
There was a problem hiding this comment.
| for rid in 1..=75 { | |
| for rid in 1..=cfg.recycled_capacity() { |
src/file_pipe_log/pipe_builder.rs
Outdated
| ); | ||
| let to_create = target.saturating_sub(self.recycled_files.len()); | ||
| let prefill_target = | ||
| std::cmp::min(self.cfg.prefill_capacity(), self.cfg.recycle_capacity()) |
There was a problem hiding this comment.
the old logic is that prefill_capacity is the number of recycle files that should be pre-created even if there are already some append files; but the new logic changes the semantic to let prefill_capacity the total files number (recycle + append), I think this is more intuitive.
This doesn't seem more intuitive to me. The prefill_limit stands for the amount of data that user can tolerate to write before startup. It should not be subtracted with append files.
There was a problem hiding this comment.
ok. I reverted this change as the default purge_threshold and recycle_threshold is still the same.
Signed-off-by: glorv <glorvs@163.com>
(purge_threshold / target_file_size) + 2to(purge_threshold / target_file_size)*1.5to avoid repeatedly creating and purging log files when tikv's log compaction has non-trivial latency.prefill_limit's semantic meaning, after this change, it will means the total log files threshold instead of recycle files threshold.