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

Take two triples in atomic way #64

Merged
merged 5 commits into from
Dec 13, 2024

Conversation

volovyks
Copy link
Contributor

@volovyks volovyks commented Dec 12, 2024

Making all operations atomic prevents race conditions, especially in multithreaded and multi-node design. It also decreases the number of Redis calls and simplifies the code, although we lose some error types.

I plan to adopt this approach in other places if we stick with it.

@volovyks volovyks changed the title Take two rtiples in atomic way Take two triples in atomic way Dec 12, 2024
@ChaoticTempest
Copy link
Contributor

Why not use redis::transaction or redis::pipeline for this? A lua script would imply running a lua runtime and all the overhead that brings

@volovyks
Copy link
Contributor Author

@ChaoticTempest atomic and transaction do not allow us to have conditional logic, return errors, etc.

@volovyks
Copy link
Contributor Author

From what I see, Lua scripts are the only thing we can use to make all these operations in a completely isolated way.
Redis should be able to execute tens of thousands of such Lua scripts per second. We are very far from it to be a bottleneck.

@ChaoticTempest
Copy link
Contributor

I took a look at benchmarks and Lua scripts actually don't have a huge performance impact (around +4-5%) but that's fine in the grand scheme of things

ChaoticTempest
ChaoticTempest previously approved these changes Dec 13, 2024
Comment on lines 179 to 180
id0: &TripleId,
id1: &TripleId,
Copy link
Contributor

Choose a reason for hiding this comment

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

TripleId implements Copy so no need to pass by reference. It's also cheaper to pass it by value than reference here too since pointers are fat (i.e. 8 bytes in 64bit systems)

@@ -54,20 +54,88 @@ impl TripleStorage {
Ok(result)
}

pub async fn take(&self, id: &TripleId) -> StoreResult<Triple> {
pub async fn take_two(&self, id1: &TripleId, id2: &TripleId) -> StoreResult<(Triple, Triple)> {
Copy link
Contributor

Choose a reason for hiding this comment

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

same with this one

Copy link
Contributor

@ailisp ailisp left a comment

Choose a reason for hiding this comment

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

Looks good!

@volovyks volovyks merged commit 44cb919 into develop Dec 13, 2024
2 of 3 checks passed
@volovyks volovyks deleted the serhii/prevent-triple-and-presignature-reusage branch December 13, 2024 12:59
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