[dbsp] Include Builder::done in time to complete a merge in the spine. - #6804
Conversation
Finishing a merge can do a lot of I/O due to fsync(), but that cost wasn't being included. This fixes the problem. It also factors out the code for measuring elapsed time into a new helper ElapsedTime::record(). Signed-off-by: Ben Pfaff <blp@feldera.com>
Signed-off-by: feldera-bot <feldera-bot@feldera.com>
mythical-fred
left a comment
There was a problem hiding this comment.
Approve. Real fix — the Builder::done cost (fsync) was outside the timer, so slow storage would silently under-report real in merge stats and mask stalls. Moving it inside elapsed.record() closes that gap.
The ElapsedTime::record(F) helper is the right abstraction: same start-real / start-cpu / AddAssign pattern the old inlined code had, now in one place, and the async wrapper collapses to a single line without changing behavior. Tests are proportionate — return-value pass-through, accumulation across calls, and the sleep-vs-CPU signature that documents why real and cpu are tracked separately (that third test doubles as executable documentation for anyone who wonders which counter to look at when triaging a stall). Nice.
Finishing a merge can do a lot of I/O due to fsync(), but that cost wasn't being included. This fixes the problem. It also factors out the code for measuring elapsed time into a new helper ElapsedTime::record().
Describe Manual Test Plan
Ran the unit tests with and without temporary changes that break them.
Checklist