Skip to content
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

fix(duplication): deal with plog concurrent problem #2068

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ninsmiracle
Copy link
Contributor

What problem does this PR solve?

#2014

What is changed and how does it work?

Old logic: If some file open with err ERR_FILE_OPERATION_FAILED, the others file could as a paramater to build a log_file object. However, plog GC thread can remove the file between those two actiion.(Just as the condition following...)
image

New logic: Add some judgement logic in find_log_file_to_start to prevent those plog (which has possible already be removed) from open_read.

Tests
  • Unit test
  • Manual test (add detailed scripts or steps below)

@github-actions github-actions bot added the cpp label Jul 9, 2024
@ninsmiracle
Copy link
Contributor Author

Here are some picture can make this problem clearly:

  • Firstly, we get _current plog file in find_log_file_to_start
    image

This process is shown in the schematic diagram:
image

This time, current will be set to plog file 1.

  • Then , in plog GC logic mutation_log::garbage_collection
    image

Traverse from new to old, find a file whose max_decree is less than cleanable, delete this plog file and all previous files.
image

In extreme cases, cleanable is _start_decree - 1 (it can never be greater than start_decree).
So no matter what, when _current is specified as 1, file 2 will not be judged to need GC. (So,switch_to_next_log_file opens the potential file 2 in the figure above, and there will be no concurrency issues.)

for (const auto &pr : file_map) {
decree cleanable_decree = _private_log->get_cleanable_decree();
decree max_decree_gpid = _private_log->max_decree(get_gpid());
if (max_decree_gpid <= cleanable_decree) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible? CHECK it if it's impossible.

return;
}

for (auto it = file_map.rbegin(); it != file_map.rend(); ++it) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use const reference?

new_file_map.emplace(pr.first, file);
new_file_map.emplace(it->first, file);

// next file map may can not open
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upper the first letter and add a dot at the end. Other places are the same.

And which code line does this comment for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. And this comment is the same meaning with line 181. So I deleted it.

// next file map may can not open
gpid pid = get_gpid();
decree previous_log_max_decree = file->previous_log_max_decree(pid);
// these plog_file has possible be deleted do not open_read next plog_file , otherwise it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is plog_file? Why you add a underline?

@ninsmiracle ninsmiracle force-pushed the deal_with_plog_concurrent_problem branch from 3db3bd1 to f030a9e Compare September 25, 2024 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants