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 several new tests #1

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

gloriallluo
Copy link

This PR includes some new tests and a tool to check linearizability.

@wangrunji0408 wangrunji0408 self-requested a review May 22, 2023 09:25
Copy link

@ErvinXie ErvinXie left a comment

Choose a reason for hiding this comment

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

Maybe a few lines of code should be corrected

src/raft/tests.rs Outdated Show resolved Hide resolved
src/raft/tests.rs Outdated Show resolved Hide resolved
src/raft/tests.rs Outdated Show resolved Hide resolved
src/porcupine/mod.rs Outdated Show resolved Hide resolved
src/porcupine/checker.rs Outdated Show resolved Hide resolved
src/porcupine/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Is this module translated from Golang code? It seems like a double linked list.
There is too much unsafe code here. Maybe we should figure out how to write in safe code if possible. 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this algorithm was translated from Golang. Most of the unsafe codes were introduced by NonNull, so I wrapped them in a new type. 🤔

Copy link
Author

Choose a reason for hiding this comment

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

This linked list is totally safe now. 🤔

Comment on lines 219 to 221
lazy_static! {
static ref T0: Instant = Instant::now();
}
Copy link
Member

@wangrunji0408 wangrunji0408 May 24, 2023

Choose a reason for hiding this comment

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

This looks weird, although I understand it's a workaround for getting current timestamp.
In madsim v0.2 we could use std::time::SystemTime directly to get timestamp of the simulated time.
But it's another long story to upgrade madsim to v0.2...

Copy link
Author

Choose a reason for hiding this comment

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

Seems there's a lot of work upgrading madsim...🥲 Now I removed the lazy_static stuff and used t0 in Tester as a start timestamp.

@gloriallluo gloriallluo requested a review from wangrunji0408 June 5, 2023 14:18
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.

3 participants