Optimize the cold startup of RaftEngine when enable-log-recycle == true

[LykxSassinator]

Discription

This commit optimize the startup of Engine when enable-log-recycle == true, by preparing a bunch of logs named with special suffix ".fakelog", so-called "Fake logs", to curtail the side effect when there is no enough stale files for log recycling.

Related Issue: close #277

Signed-off-by: Lucasliang [email protected]

[codecov]

Codecov Report

Base: 97.74% // Head: 97.73% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (459c127) compared to base (3353011).
Patch coverage: 97.88% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #278      +/-   ##
==========================================
- Coverage   97.74%   97.73%   -0.01%     
==========================================
  Files          30       30              
  Lines       11287    11411     +124     
==========================================
+ Hits        11032    11153     +121     
- Misses        255      258       +3     

Impacted Files	Coverage Δ
src/file_pipe_log/mod.rs	98.46% <ø> (ø)
src/pipe_log.rs	95.45% <ø> (ø)
tests/failpoints/mod.rs	100.00% <ø> (ø)
tests/failpoints/util.rs	98.86% <ø> (-0.09%)	⬇️
src/file_pipe_log/format.rs	99.10% <94.44%> (-0.41%)	⬇️
src/file_pipe_log/pipe_builder.rs	95.49% <96.12%> (-0.12%)	⬇️
src/file_pipe_log/pipe.rs	98.48% <96.81%> (-1.01%)	⬇️
src/config.rs	97.57% <100.00%> (+0.24%)	⬆️
src/engine.rs	98.29% <100.00%> (+0.09%)	⬆️
src/env/default.rs	93.40% <100.00%> (+0.58%)	⬆️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[LykxSassinator]

@tabokie PTAL, thx

[tabokie]

Mixing multiple log queues is too ugly. I'm thinking something like this:

// builder.rs
let suffix = "append.reserved";
let reserved_files = ReservedFiles::scan_from_path(fs, path, suffix)?;
if capacity > active_files.len() + reserved_files.len() {
  reserved_files.generate_some(..);
}
let pipe = SinglePipe::build(reserved_files, active_files);
// pipe.rs
fn purge() {
  for f in purged_files {
    if f.format == V2 {
      self.reserved_files.add(f.handle);
    }
  }
  self.reserved_files.truncate(self.capacity - self.active_files.len());
}

[LykxSassinator]

@tabokie PTAL, thx

[tabokie]

As I mentioned before, keeping obsolete files across restart (i.e. do not delete them when Pipe is dropped) is of higher priority. Which means all obsolete files (not just "fake" logs) will be managed by reserved_files. They are renamed (this detail is preferably put inside ReservedFiles) before retired so that we won't waste time recovering them later.

[LykxSassinator]

@tabokie PTAL, thx.

[tabokie]

I don't know why this change is so difficult for me to review... Maybe part of it has to do with the interactions between structures are not properly encapsulated. E.g. prepare_dummy_logs_for_recycle with so many arguments, concat that accepts files without checking if they have the proper names.

To improve, I think you can just keep thing even simpler, make FileCollection as simple as possible (as simple as a Vec):

pub struct FileList { seq, files };
impl FileList {
  fn append(&mut self, rhs: &mut FileList ) { ... }
  // Different than `VecDeque`, this function takes seqno as splitting point, and returns the first half.
  fn split_front(&mut self, seq: u64) -> Self {}
  // Caller must guarantee the file has the right seqno.
  fn push_back(&mut self, file) {..}
  fn pop_front(&mut self) -> Option<FileWithFormat> {...};
  fn span(&self) -> (usize, usize);
  fn len(&self) -> usize;
  fn get(&self, seq: usize) -> Option<&FileWithFormat> {...}
}

Then all the complicated logic can be put in-place, or wrapped into a bigger struct:

pub struct FileCollection { capacity, active_files: Mutex<FileList>, stale_files: Mutex<FileList>, fs }

impl FileCollection {
  fn rotate(&self) -> FileHandle {
    let f = self.stale_files.pop_front().or_else(|| fs.create(self.active_files.span().1 + 1));
    self.active_files.lock().push_back(f);
    f.handle
  }

  fn purge(&self, seq: usize) {
    let stale_files = self.active_files.split_front(seq);
    for f in stale_files {
      if f.version > 1 {
        self.fs.reuse(f, new_stale_file_name(self.stale_files.span().1 + 1));
        self.stale_files.lock().push_back(f);
      }
    }
  }

  fn initialize(&mut self) {
    for _ in self.stale_files.len()..capacity {
      let f = self.fs.create(new_stale_file_name(self.stale_files.span().1 + 1));
      self.stale_files.push_back(f);
    }
  }

  fn get(&self, seq: usize) -> Option<FileHandle> {
    self.active_files.lock().get(seq)
  }
}

[LykxSassinator]

To improve, I think you can just keep thing even simpler, make FileCollection as simple as possible (as simple as a Vec):

Thx for your advices.
I've also take this design in previous draft, but I dropped it finally. For the reason why I didn't adopt the structure as u said, just like:

struct FileCollection {
    stale_files: FileList<F>,
    active_files: FileList<F>,
    ...
}

is that:
In previous work, SinglePipe is the operator and manager of FileCollection. IMO, the Stale files and Active files, represent by StaleFileCollection and ActiveFileCollection respectively, are the minimal operating unit of FileList. So, each one of them should be designed as an individual object because the related operations has been encapsulated in the API of SinglePipe. So, from my views, SinglePipe is just the subject with same trait as the new FileCollection u mentioned above. For example, SinglePipe.get_fd(...) just called the ActiveFileCollection.get_fd(...) to return the expected fd to callers.

And the confusing parts, such as prepare_dummy_logs_for_recycle, will be polished.

[tabokie]

@LykxSassinator It doesn't contradict with that I said.

Then all the complicated logic can be put in-place, or wrapped into a bigger struct:

The code snippet I wrote refers to the way to "wrapped into a bigger struct". It can be done as "put in-place" as well.

[LykxSassinator]

@LykxSassinator It doesn't contradict with that I said.

Then all the complicated logic can be put in-place, or wrapped into a bigger struct:

The code snippet I wrote refers to the way to "wrapped into a bigger struct". It can be done as "put in-place" as well.

Yes, i got what u meaned. I will polish the current codes.

[LykxSassinator]

ping @tabokie PTAL, thx

[LykxSassinator]

ping @tabokie PTAL, thx.

[tabokie]

I took the liberty of cleaning up the abstraction myself, it grows to be, somewhat superfluous IMO.

And by the way, #278 (comment) is not addressed. When you use &mut self, there's no need to lock again, because mutable reference guarantees exclusive access.

[LykxSassinator]

I took the liberty of cleaning up the abstraction myself, it grows to be, somewhat superfluous IMO.

And by the way, #278 (comment) is not addressed. When you use &mut self, there's no need to lock again, because mutable reference guarantees exclusive access.

Thx for your suggestions. I'll take some time to comprehend the newly introduced refactoring works.

[tabokie]

Return early if !prefill_for_recycle.

[LykxSassinator]

Not recommended.
When !prefill_for_recycle, it should check whether there still exists remained recycled_files and clear them if exists. And by the way, the later processing when !prefill_for_recycle just do several checking jobs, not costing much on CPU or other resources.

[LykxSassinator]

ping @tabokie PTAL, thx.

[tabokie]

file_seq < files[0].seq?

[tabokie]

Is there a test?

[tabokie]

Ping @LykxSassinator

[tabokie]

Ping again.

[LykxSassinator]

This branch has already been tested in test_pipe_log:

        ...
        // cannot purge active file
        assert!(pipe_log.purge_to(FileId { queue, seq: 4 }).is_err());
        ...

And more boundary test cases have been supplemented into test_pipe_log_with_recycle.

[tabokie]

Let's keep this in mind. Line coverage can only be useful when there's something not covered. What's covered is far from enough and cannot be used as a proof of software quality. (MC/DC coverage, on the other hand, is a much better criterion.) Whenever there's a bug, it's vital to keep the condition covered before fixing it.

[tabokie]

Needs changelog under "New Features", rest looks good.

[LykxSassinator]

ping @tabokie, thx.

[tabokie]

//

[tabokie]

Ping again.