storage: round probe timestamps to the probe interval #31072
Conversation
teskje
force-pushed
the
probe-round-timestamps
branch
2 times, most recently
from
cd4f574 to
c5a5c10
Compare
This commit ensures that probe timestamps, and therefore timestamps used for minting new bindings, are rounded down to the probe interval (typically 1s). This is to reduce the amount of distinct timestamps flowing through a dataflow joining multiple sources. Each distinct timestamp induces some amount of overhead, so forcing the timestamps of individual sources to the same values is more efficient.
teskje
force-pushed
the
probe-round-timestamps
branch
from
c5a5c10 to
6856a22
Compare
| /// Return the desired time of the next tick. | ||
| fn next_tick_target(&self) -> EpochMillis { | ||
| let target = match self.last_tick { | ||
| Some(ms) => ms + self.interval, |
There was a problem hiding this comment.
Might this over time get out of sync with the NowFn/wall-clock time? If we only ever keep adding an interval to a timestamp we got initially.
There was a problem hiding this comment.
Ah, but we're not doing that! We call the NowFn in each tick and set the returned timestamp (rounded down) as last_tick to which the interval is then added.
| @@ -1737,8 +1739,6 @@ fn spawn_metadata_thread<C: ConsumerContext>( | |||
| if tx.send((probe_ts, update)).is_err() { | |||
| break; | |||
| } | |||
|
|
|||
| thread::park_timeout(poll_interval); | |||
There was a problem hiding this comment.
Is the change from park_timeout to using sleep a behavior change? Or can sleep also be woken up the same way? Might be worth considering but I haven't looked into the behavior of either function.
There was a problem hiding this comment.
It is! The difference is that a parked thread can be woken up by another thread, a sleeping thread cannot. I think in this case we don't need the wakeup-by-other-thread behavior and so sleep is the right way, but lmk if I missed something!
There was a problem hiding this comment.
Park/unpark was introduced here: #10845, so it was meaningful. Not sure where things are currently, I think the thread now works somewhat differently. But still might be worth it to keep the immediate-cleanup-on-drop behavior.
There was a problem hiding this comment.
Ah, good to know! Just changing the sleep to a park_timeout doesn't help much though. The sleep is now in the Ticker which only returns from a tick when the interval is reached, regardless of whether it's woken up before or not. The unpark-on-drop code also doesn't exist anymore.
We could change both of these things, but it doesn't seem worthwhile to pile on complexity just to shut down the thread slightly faster. With RLU the tick interval will be 1s, which is quite different from the 5m interval that existed when the unpark_timeout was added.
There was a problem hiding this comment.
fine for me! as I said, none of my comments were meant to block merging 🕊️
| > ALTER CLUSTER test SET (REPLICATION FACTOR 0) | ||
| > ALTER CLUSTER test SET (REPLICATION FACTOR 1) | ||
|
|
||
| > SELECT name, write_frontier::text::uint8 % 1000 = 1 |
There was a problem hiding this comment.
Are the write frontiers now, say 1, 1001, 2001? My I-like-rounded-numbers brain would prefer 0, 1000, 2000. I could be totally misunderstanding what's happening though.
There was a problem hiding this comment.
Indeed. I think what's happening here is that the source pipeline mints a binding for time, e.g. 1000, writes out all the data it has for that binding and then steps forward the frontier to indicate that all future updates will be at some later time. We could maybe use our knowledge of the tick interval instead of just stepping forward, but idk what the implications of that would be.
Looks like that got accidentally removed in MaterializeInc#30817.
|
TFTRs! |
This PR ensures that probe timestamps, and therefore timestamps used for minting new bindings, are rounded down to the probe interval (typically 1s). This is to reduce the amount of distinct timestamps flowing through a dataflow joining multiple sources. Each distinct timestamp induces some amount of overhead, so forcing the timestamps of individual sources to the same values is more efficient.
Motivation
Closes https://github.com/MaterializeInc/database-issues/issues/8885
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.