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

Update the updater.rs code. #2894

Merged
merged 5 commits into from
Nov 15, 2024
Merged

Conversation

MathieuDutSik
Copy link
Contributor

@MathieuDutSik MathieuDutSik commented Nov 14, 2024

Motivation

The updater.rs is currently using the blob_last_used_by with the error case returning the missing entries which is a bd design:

  • It might get you a wrong index if the error is not not_found.
  • It does not group the data.

Proposal

The following was done:

  • Introduce the missing_blob_states that provides what we are after.
  • Remove the blobs_last_used_by that was previously introduced, but keep it in the db_storage.rs.
  • The replacement of stream and the attention to only getting whether a blob_state is present or not allows for grouping the operation efficiently.

Possible criticism of the design:

  • We could have kept the blobs_last_used_by but there was no use case and in particular, we could not use it for getting the missed blob ids.
  • The API is left in the db_storage.rs. Possibly we could remove it as well as other unused APIs.
  • The missing_blob_ids does a single operation. We could split it into blocks and then use a stream in a future PR. However, the size is quite small, only BlobIds.

There is no remaining read_certificate(s) that is done for extracting only small data.

Test Plan

The CI

Release Plan

This changes the binaries but does not change the storage. So, a switch

Links

@MathieuDutSik MathieuDutSik marked this pull request as ready for review November 14, 2024 14:42
Copy link
Contributor

@afck afck left a comment

Choose a reason for hiding this comment

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

Good catch! We actually are only interested in the missing blobs in this case.

linera-core/src/updater.rs Outdated Show resolved Hide resolved
linera-core/src/node.rs Outdated Show resolved Hide resolved
Comment on lines 400 to 391
Ok(missing_blobs)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

[Re: lines +385 to +401]

Like @afck mentioned, `missing_blob_states` should be semantically equivalent to this one (we'll only have a state in storage if we have a blob also). So we can probably just use this existing one instead and delete `missing_blob_states`

See this comment inline on Graphite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thank you.

@MathieuDutSik MathieuDutSik merged commit e3b4729 into linera-io:main Nov 15, 2024
21 checks passed
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