[dbsp] Limit checkpoint fingerprints to the JS-safe 53-bit range - #6844
[dbsp] Limit checkpoint fingerprints to the JS-safe 53-bit range#6844blp wants to merge 2 commits into
Conversation
fda's REST client generates the CheckpointMetadata.fingerprint field as i64 from the OpenAPI schema, so a checkpoint whose 64-bit fingerprint has the high bit set breaks `fda pipelines`/`fda status` for the whole instance (#6841). Add a Fingerprint newtype that masks to 0..2**53 both when a circuit computes a fresh fingerprint and when one is deserialized, so checkpoints already on disk get corrected on load instead of only new ones. Also add a migration to mask any checkpoint fingerprint already cached in a pipeline's storage_status_details: a stopped pipeline has no running worker to send a fresh status report, so its last-cached value would otherwise stay out of range indefinitely. Fixes #6841 Signed-off-by: Ben Pfaff <blp@feldera.com>
Signed-off-by: feldera-bot <feldera-bot@feldera.com>
mihaibudiu
left a comment
There was a problem hiding this comment.
The only thing I don't understand is exactly how migration will happen for older checkpoints, but I don't know that part of the code.
| #[derive(Debug, Copy, Clone, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| pub struct Fingerprint(u64); | ||
|
|
||
| /// The derive can't express `maximum` for a tuple struct (utoipa only allows |
There was a problem hiding this comment.
This comment could be just the last line
| -- gets a fresh status report to pick up that fix, so its last-cached | ||
| -- `storage_status_details` can still hold an out-of-range value. Mask any | ||
| -- checkpoint fingerprint already stored here the same way. | ||
| UPDATE pipeline |
There was a problem hiding this comment.
it makes sense, just wonder if it's even worth to run an explicit migration for it
There was a problem hiding this comment.
Well, we don't have to, if you don't care whether fda pipelines works with older pipelines.
| "type": "integer", | ||
| "format": "int64", | ||
| "description": "Fingerprint of the circuit at the time of the checkpoint.", | ||
| "description": "Fingerprint of the circuit at the time of the checkpoint, limited to 0..2**53 to stay within JavaScript's safe integer range.", |
There was a problem hiding this comment.
isn't javascript a great language
There was a problem hiding this comment.
I wonder why this is a javascript problem since it's the fda rust? binding that fails
There was a problem hiding this comment.
Depends on whether we might want to use the fingerprints from the web console someday. If we don't care, then we don't have to do this.
| ); | ||
| } | ||
|
|
||
| #[cfg(feature = "testing")] |
There was a problem hiding this comment.
is this supposed to be behind a testing gate?
| @@ -0,0 +1,37 @@ | |||
| -- `storage_status_details` embeds each checkpoint's fingerprint as a plain | |||
There was a problem hiding this comment.
it seems we already have a V36_ in the repo (i believe they need to be unique and incrementing for things to work)
|
@gz There's a much simpler fix if we only care about |
|
Closing in favor of #6850 |
fda's REST client generates the CheckpointMetadata.fingerprint field as i64 from the OpenAPI schema, so a checkpoint whose 64-bit fingerprint has the high bit set breaks
fda pipelines/fda statusfor the whole instance (#6841). Add a Fingerprint newtype that masks to 0..2**53 both when a circuit computes a fresh fingerprint and when one is deserialized, so checkpoints already on disk get corrected on load instead of only new ones.Also add a migration to mask any checkpoint fingerprint already cached in a pipeline's storage_status_details: a stopped pipeline has no running worker to send a fresh status report, so its last-cached value would otherwise stay out of range indefinitely.
Fixes #6841
Describe Manual Test Plan
Tested a sample migration
Checklist