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

Add Physical lock file so that multiple processes can't operate on the same db directory simultaneously #102

Merged
merged 5 commits into from
Sep 25, 2021

Conversation

yuqi1129
Copy link
Contributor

Description: see #101 and #99

…me db directory simultaneously tikv#101

Signed-off-by: yuqi1129 <yuqi4733@gmail.com>
src/engine.rs Outdated
@@ -307,6 +308,8 @@ where

listeners: Vec<Arc<dyn EventListener>>,

engine_file: File,
Copy link
Member

Choose a reason for hiding this comment

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

Put this inside FilePipeLog?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tabokie I have try to put it in FilePipeLog , but FilePipeLog has marked as Clone and File is not clonable and had no idea to solve this problem, so i put it there, do you have any idea about this?

Copy link
Member

Choose a reason for hiding this comment

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

You can remove the #[derive(Clone)] for FilePipeLog, it's not needed now. As a side note, you can make any type T cloneable by wrapping it into Arc<T>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it

src/file_pipe_log.rs Outdated Show resolved Hide resolved
@@ -25,6 +25,8 @@ use crate::pipe_log::{FileId, LogQueue, PipeLog, SequentialReplayMachine};
use crate::reader::LogItemBatchFileReader;
use crate::util::InstantExt;
use crate::{Error, Result};
use std::fs::File;
use fs2::FileExt;
Copy link
Member

Choose a reason for hiding this comment

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

ditto, belong to third_party::xx

src/file_pipe_log.rs Outdated Show resolved Hide resolved
Comment on lines 824 to 828
let r1 = FilePipeLog::open::<BlackholeSequentialReplayMachine>(
&cfg,
Arc::new(DefaultFileBuilder {}),
vec![],
);
Copy link
Member

Choose a reason for hiding this comment

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

In tests, we normally unwrap the result rather than asserting is_ok().

Suggested change
let r1 = FilePipeLog::open::<BlackholeSequentialReplayMachine>(
&cfg,
Arc::new(DefaultFileBuilder {}),
vec![],
);
let r1 = FilePipeLog::open::<BlackholeSequentialReplayMachine>(
&cfg,
Arc::new(DefaultFileBuilder {}),
vec![],
).unwrap();

src/file_pipe_log.rs Outdated Show resolved Hide resolved
@tabokie
Copy link
Member

tabokie commented Sep 23, 2021

BTW, you can run cargo clippy --all before committing, it can detect lint and format issues.

…me db directory simultaneously tikv#101

Signed-off-by: yuqi1129 <yuqi4733@gmail.com>
…me db directory simultaneously tikv#101

Signed-off-by: yuqi1129 <yuqi4733@gmail.com>
…me db directory simultaneously tikv#101

Signed-off-by: yuqi1129 <yuqi4733@gmail.com>
@yuqi1129
Copy link
Contributor Author

@tabokie , Can you take time to review a again

Copy link
Member

@tabokie tabokie left a comment

Choose a reason for hiding this comment

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

Rest LGTM

Comment on lines 835 to 836
assert_eq!(r2.is_err(), true);
assert_eq!(format!("{}", r2.err().unwrap()).contains("maybe another instance is using this directory"), true);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert_eq!(r2.is_err(), true);
assert_eq!(format!("{}", r2.err().unwrap()).contains("maybe another instance is using this directory"), true);
r2.expect_err("maybe another instance is using this directory");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it

…me db directory simultaneously tikv#101

Signed-off-by: yuqi1129 <yuqi4733@gmail.com>
@tabokie tabokie merged commit ed93866 into tikv:master Sep 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants