xline-kv / Xline

A geo-distributed KV store for metadata management
https://xline.cloud
Apache License 2.0
565 stars 71 forks source link

fix: prevent batch_index overflow in raw_curp #704

Closed GFX9 closed 3 weeks ago

GFX9 commented 3 months ago
codecov[bot] commented 3 months ago

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 :warning:
crates/curp/src/server/raw_curp/mod.rs 66.66% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bsbds commented 3 months ago

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]
    }
}
mergify[bot] commented 2 months ago

@GFX9 Convert your pr to draft since CI failed

mergify[bot] commented 2 months ago

@GFX9 Convert your pr to draft since CI failed

mergify[bot] commented 2 months ago

@GFX9 Convert your pr to draft since CI failed

GFX9 commented 2 months ago

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 commented 2 months ago

@Mergifyio rebase

mergify[bot] commented 2 months ago

rebase

✅ Branch has been successfully rebased

mergify[bot] commented 1 month ago

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