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: manifest delta num may overflow #1616

Merged
merged 14 commits into from
Dec 23, 2024
Merged

Conversation

jiacai2050
Copy link
Contributor

@jiacai2050 jiacai2050 commented Dec 20, 2024

Rationale

Fix bugs found in local write bench.

Detailed Changes

  • When manifest starts up, the delta num may overflow since it's initialized to 0.
  • When manifest merge_update, since the deltas is unsorted, so we may first delete a non-existing file.

Test Plan

CI

@github-actions github-actions bot added the chore label Dec 20, 2024
@jiacai2050 jiacai2050 requested a review from baojinri December 21, 2024 13:53
@jiacai2050 jiacai2050 marked this pull request as ready for review December 21, 2024 13:53
@jiacai2050 jiacai2050 changed the title chore: refactor bench write fix: manifest delta num may overflow Dec 21, 2024
@github-actions github-actions bot added the bug Something isn't working label Dec 21, 2024
}

pub fn into_bytes(self) -> Result<Bytes> {
let buf = Vec::with_capacity(self.header.length as usize + SnapshotHeader::LENGTH);
let buf = Vec::with_capacity(self.header.length as usize * SnapshotHeader::LENGTH);
Copy link
Contributor

Choose a reason for hiding this comment

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

self.header.length as usize + SnapshotHeader::LENGTH is right ?

self.header.length is the total length of the payload of Snapshot in bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

@zealchen
Copy link
Contributor

"When manifest merge_update, since the deltas is unsorted, so we may first delete a non-existing file."

Why is the failure to delete related to the sorting of the deltas? Intuitively they are mutually irrelevent with each other.

@@ -52,6 +62,16 @@ async fn hello() -> impl Responder {
HttpResponse::Ok().body("Hello world!")
}

#[get("/toggle")]
async fn toggle(data: web::Data<AppState>) -> impl Responder {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reasons for choosing Actix over Axum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific reason. GPT says actix is more feature-rich.

@jiacai2050
Copy link
Contributor Author

jiacai2050 commented Dec 23, 2024

Why is the failure to delete related to the sorting of the deltas? Intuitively they are mutually irrelevent with each other.

This is a tricky bug. For example, we have two delta files to merge:

  1. to_add: 100
  2. to_add: 101, to_delete: 100

If the delta files are unsorted, when we first merge 2, 100 is not in snapshot yet, so it will delete nothing.

Copy link
Contributor

@baojinri baojinri left a comment

Choose a reason for hiding this comment

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

LGTM

@baojinri baojinri merged commit f8bb726 into apache:main Dec 23, 2024
6 checks passed
@jiacai2050 jiacai2050 removed the chore label Dec 23, 2024
@jiacai2050 jiacai2050 deleted the fix-bench-write branch December 23, 2024 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants