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: prevent batch_index overflow in raw_curp #704

Merged
merged 3 commits into from
Jun 13, 2024

Conversation

GFX9
Copy link
Contributor

@GFX9 GFX9 commented Mar 18, 2024

  • what problem are you trying to solve? (or if there's no problem, what's the motivation for this change?)
    -> Fixes Bug: batch_index in raw_curp will overflow eventually #368

  • what changes does this pull request make?
    -> Use a scrolling array to replace the previous prefix array to prevent batch_index overflow, which will be truncated when compact. Besides, the scrolling array reduces the operation time complexity of obtaining batch log to O(1).

  • are there any non-obvious implications of these changes? (does it break compatibility with previous versions, etc)
    -> No

@GFX9 GFX9 force-pushed the issue-368 branch 2 times, most recently from 0d73ba7 to a80897e Compare March 18, 2024 03:24
@GFX9 GFX9 changed the title fix: prevent batch_index overflow in raw_curp #368 fix: prevent batch_index overflow in raw_curp Mar 18, 2024
@GFX9 GFX9 force-pushed the issue-368 branch 2 times, most recently from 8b788c1 to a7b56d8 Compare March 27, 2024 13:54
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 89.53975% with 25 lines in your changes missing coverage. Please review.

Project coverage is 75.68%. Comparing base (e35b35a) to head (cf8c026).
Report is 109 commits behind head on master.

Files Patch % Lines
crates/curp/src/server/raw_curp/log.rs 89.83% 16 Missing and 8 partials ⚠️
crates/curp/src/server/raw_curp/mod.rs 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #704      +/-   ##
==========================================
+ Coverage   75.55%   75.68%   +0.13%     
==========================================
  Files         180      187       +7     
  Lines       26938    27788     +850     
  Branches    26938    27788     +850     
==========================================
+ Hits        20353    21032     +679     
- Misses       5366     5467     +101     
- Partials     1219     1289      +70     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

crates/curp/src/server/raw_curp/log.rs Outdated Show resolved Hide resolved
crates/curp/src/server/raw_curp/log.rs Outdated Show resolved Hide resolved
crates/curp/src/server/raw_curp/log.rs Outdated Show resolved Hide resolved
crates/curp/src/server/raw_curp/log.rs Outdated Show resolved Hide resolved
crates/curp/src/server/raw_curp/log.rs Outdated Show resolved Hide resolved
@bsbds
Copy link
Collaborator

bsbds commented Apr 3, 2024

This implementation is too complex compare to the original prefix sum version. I suggest making some modifications to the original algorithm. Here's the pseudocode. The interval representation is [left, right).

#[derive(Default)]
struct Log {
    right: Vec<usize>,
    sizes: Vec<u64>,
    limit: u64,
    p: usize,
    sum: u64,
}

impl Log {
    fn push(&mut self, size: u64) {
        self.sizes.push(size);
        self.right.push(0);
        self.sum += size;
        while self.sum > self.limit {
            self.right[self.p] = self.sizes.len() - 1;
            self.sum -= self.sizes[self.p];
            self.p += 1;
        }
    }

    fn get(&self, left: usize) -> usize {
        self.right[left]
    }
}

@GFX9 GFX9 force-pushed the issue-368 branch 3 times, most recently from 1574f83 to 7a2046d Compare April 9, 2024 07:06
crates/curp/src/server/raw_curp/log.rs Outdated Show resolved Hide resolved
crates/curp/src/server/raw_curp/log.rs Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team April 19, 2024 04:37
Copy link

mergify bot commented Apr 19, 2024

@GFX9 Convert your pr to draft since CI failed

@mergify mergify bot marked this pull request as draft April 19, 2024 13:42
@Phoenix500526 Phoenix500526 marked this pull request as ready for review April 23, 2024 15:24
@mergify mergify bot requested a review from a team April 24, 2024 04:12
crates/curp/src/server/raw_curp/log.rs Outdated Show resolved Hide resolved
crates/curp/src/server/raw_curp/log.rs Outdated Show resolved Hide resolved
crates/curp/src/server/raw_curp/log.rs Outdated Show resolved Hide resolved
crates/curp/src/server/raw_curp/log.rs Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team April 24, 2024 04:51
Copy link

mergify bot commented Apr 26, 2024

@GFX9 Convert your pr to draft since CI failed

@mergify mergify bot marked this pull request as draft April 26, 2024 08:08
Copy link

mergify bot commented Apr 26, 2024

@GFX9 Convert your pr to draft since CI failed

@GFX9
Copy link
Contributor Author

GFX9 commented Apr 26, 2024

The current implementation uses relevant offset to handle the right index of the range. But when Log calls methods of LogEntryVecDeque, there already exists the process of transferring the logical index to physical index. So the current implementation is kind of redundant, which should be improved in the future.

@Phoenix500526
Copy link
Collaborator

@Mergifyio rebase

Phoenix500526
Phoenix500526 previously approved these changes May 9, 2024
Copy link
Collaborator

@Phoenix500526 Phoenix500526 left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot requested a review from a team May 9, 2024 06:06
@mergify mergify bot requested a review from a team May 11, 2024 01:46
@mergify mergify bot requested a review from a team May 11, 2024 01:49
@Phoenix500526 Phoenix500526 self-assigned this May 28, 2024
@Phoenix500526 Phoenix500526 linked an issue May 28, 2024 that may be closed by this pull request
@Phoenix500526 Phoenix500526 force-pushed the issue-368 branch 2 times, most recently from f5ff5aa to 1451a42 Compare May 28, 2024 13:33
Copy link

mergify bot commented Jun 6, 2024

@GFX9 Your PR is in conflict and cannot be merged.

crates/curp/src/log_entry.rs Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team June 11, 2024 07:28
@mergify mergify bot requested a review from a team June 11, 2024 07:56
@mergify mergify bot requested a review from a team June 12, 2024 03:46
@mergify mergify bot merged commit e54d83f into xline-kv:master Jun 13, 2024
14 checks passed
@mergify mergify bot requested a review from a team June 13, 2024 01:58
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.

Refactor the LogEntryVecDeque in the log.rs Bug: batch_index in raw_curp will overflow eventually
4 participants