Conversation
Signed-off-by: Ben Pfaff <blp@feldera.com>
gz
left a comment
There was a problem hiding this comment.
the 2nd commit message seems to end abruptly at:
Since the /stats``
|
|
||
| // Write an empty checkpoint file to save the cost of listing | ||
| // all the files next time. | ||
| backend.write_json(&file_name, &VecDeque::<CheckpointMetadata>::new())?; |
There was a problem hiding this comment.
i hope this is "safe", it seems to be because we only do this when we couldnt recover any checkpoints anyways(?)
There was a problem hiding this comment.
It should be? There's a theoretical race where something external writes a nonempty file just before we write it. But AFAIK nothing should be doing that.
When there is no checkpoint catalog file, Checkpointer::read_checkpoints scans all of storage looking for checkpoint candidates. Since the `/status` and `/coordination/status` APIs call this a lot, it's expensive. Signed-off-by: Ben Pfaff <blp@feldera.com>
This isn't needed and it's expensive to do I/O at least once a second to implement `/coordination/status`. Signed-off-by: Ben Pfaff <blp@feldera.com>
Oops, fixed. |
mythical-fred
left a comment
There was a problem hiding this comment.
Three nice perf/quality wins in one PR — happy to see them all land together.
get_status(with_storage_status_details)correctly cuts theCheckpointer::read_checkpointscall out of the SSE polling loop incoordination_status. That listing was previously running on every notify iteration, which is exactly what you'd want to avoid on a hot streaming endpoint.- Exchange connection retry: hoisting the failure counter and logging "connected after N failures" turns a repeated
info!spam into a single actionable signal. Also nice thatbackoff_time()is now module-level instead of a nestedfn— easier to grep. - Checkpointer memoization is the right call for the empty-directory startup path.
One non-blocking nit on that last one:
Checkpointer::read_checkpoints now writes to the backend on the NotFound path. The name still suggests a pure read, so a caller (or future maintainer) reading read_checkpoints(&*backend) won't expect it to mutate storage. Worth either (a) a one-line doc comment on the fn saying "materializes an empty checkpoint file on first call if none exists, to short-circuit the listing on subsequent calls," or (b) renaming to load_checkpoints / factoring the write into a separate ensure_checkpoint_file_exists helper. The race gz asked about is theoretical as blp noted, so no blocker there.
Describe Manual Test Plan
These are primarily performance optimizations. I'll try them out with some workloads before merging.