Fix lease extension, and make it recursive#9956
Conversation
stuhood
force-pushed
the
stuhood/lease-extension-fixes
branch
from
12caed5 to
40e1f71
Compare
|
Commits are useful to review independently. Thanks! |
… a different key. Centralize, and add a test. [ci skip-jvm-tests]
…ecursive more parallel and more async/awaity. [ci skip-jvm-tests]
[ci skip-jvm-tests]
# Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]
stuhood
force-pushed
the
stuhood/lease-extension-fixes
branch
from
40e1f71 to
e8c6a7a
Compare
| let f_dbs = self.inner.file_dbs.clone()?; | ||
| let is_file = f_dbs.exists(fingerprint); | ||
|
|
||
| // TODO: Could technically use select to return slightly more quickly with the first |
There was a problem hiding this comment.
Is the intent to fix this TODO at some point?
There was a problem hiding this comment.
Possibly? It's preeeetty micro-optimizey, but might be worth doing the next time someone is in there.
There was a problem hiding this comment.
My concern is that this introduces a TODO that will never get done. Maybe just have the comment not say TODO if there is no intent to change later?
| let executor = local.executor().clone(); | ||
| let maybe_upload = local | ||
| .load_bytes_with(entry_type, digest, move |bytes| { | ||
| // NB: `load_bytes_with` runs on a spawned thread which we can safely block. |
There was a problem hiding this comment.
This seems like an assumption that could confuse a reader (if not for the comment). Should load_bytes_with be renamed to say it can block (e.g., load_bytes_with_blocking)? Have all other load_bytes_with call sites received a similar comment? Does Rust have a way to encode the "blockability" of a closure into the type? (probably not but I'm curious if it has)
There was a problem hiding this comment.
This seems like an assumption that could confuse a reader (if not for the comment). Should load_bytes_with be renamed to say it can block (e.g., load_bytes_with_blocking)?
This is existing code that was only moved: see #9793. The called method also has a comment.
Does Rust have a way to encode the "blockability" of a closure into the type? (probably not but I'm curious if it has)
Good question: whee, colored functions.
No, it does not. On the "bright side", tokio will panic to let you know that you've blocked in the wrong place: so you'll know if you've done it wrong, but you don't get compiler assistance.
As was misdiagnosed on #9942, extension of leases is currently broken due to `Store::lease_all` using a separate implementation of `fn lease` that is not aware of `VersionedFingerprint` and thus has a different key. This means that the leases being read in `Store::shrink` are different from the leases being extended in `Store::lease_all`. Additionally, lease extension is not currently recursive into all values held in memory in the `Graph`, which might mean that a value held in memory in `pantsd` can become invalidated. Fix the first issue by removing the duplicate implementation of `fn lease`, and making `ShardedLmdb` more directly responsible for lease management: add a test to cover the failing case. In separate commits, adjust our lease extension of in-memory content to be recursive into `Snapshots` and `ProcessResults`. Fixes #9942. [ci skip-jvm-tests]
3 participants
Problem
As was misdiagnosed on #9942, extension of leases is currently broken due to
Store::lease_allusing a separate implementation offn leasethat is not aware ofVersionedFingerprintand thus has a different key. This means that the leases being read inStore::shrinkare different from the leases being extended inStore::lease_all.Additionally, lease extension is not currently recursive into all values held in memory in the
Graph, which might mean that a value held in memory inpantsdcan become invalidated.Solution
Fix the first issue by removing the duplicate implementation of
fn lease, and makingShardedLmdbmore directly responsible for lease management: add a test to cover the failing case.In separate commits, adjust our lease extension of in-memory content to be recursive into
SnapshotsandProcessResults.Result
Fixes #9942.