Adding new heartbeat_agg #615
Conversation
WireBaron
requested review from
rtwalker,
syvb,
thatzopoulos,
davidkohn88 and
epgts
| #[pg_extern] | ||
| pub fn dead_ranges( | ||
| agg: HeartbeatAgg<'static>, | ||
| ) -> TableIterator<'static, (name!(start, TimestampTz), name!(end, TimestampTz))> { |
There was a problem hiding this comment.
wondering if we should just return a tstzrange? Mostly because this leaves it unclear whether it's inclusive on both sides or exactly how that works. We should be clear about the behavior either way, will need to document...
There was a problem hiding this comment.
I would think that time ranges (really any ranges over continuous data) are almost always left inclusive right exclusive, and really only feel a need to explain if the behavior varies from this. We can be explicit about this in the documentation if you feel it's important, but honestly I don't think any other approach would make sense.
extension/src/heartbeat_agg.rs
Outdated
| } | ||
|
|
||
| #[pg_extern] | ||
| pub fn duration_dead(agg: HeartbeatAgg<'static>) -> Interval { |
There was a problem hiding this comment.
Might want to change these names to downtime / uptime
Though it does mean our nomenclature changes. I think live_at / dead_at are slightly better than up_at / down_at and live_ranges / dead_ranges are much better than up_ranges / down_ranges
If we stick with this formulation though, I think we should definitely go with dead_duration / live_duration as it feels like switching the ordering of words ie leading with duration rather than the state is worse?
There was a problem hiding this comment.
I think just changing these accessors to uptime/downtime is alright. Having both uptime and live_ranges together doesn't strike me as too confusing.
|
|
||
| const BUFFER_SIZE: usize = 1000; // How many values to absorb before consolidating | ||
|
|
||
| // Given the lack of a good range map class, or efficient predecessor operation on btrees, |
There was a problem hiding this comment.
https://docs.rs/rangetree/latest/rangetree/ doesn't work? or just not good enough / just another dependency we don't need? Anything we can learn from it?
There was a problem hiding this comment.
I looked at several different options here, none of them actually behaved as I needed. The example you linked actually uses a balanced tree, so avoids the most common issue I had, but it also assumes a space of discrete values which can be taken one at a time rather than letting a caller take continuous subranges of the overall space.
extension/src/raw.rs
Outdated
| let ptr = | ||
| pg_sys::palloc(std::mem::size_of::<pg_sys::Interval>()) as *mut pg_sys::Interval; | ||
| *ptr = interval; | ||
| Interval(pgx::Datum::from(ptr)) |
There was a problem hiding this comment.
pgx::Datum won't work in pgx 0.6.0 (see #618)
| Interval(pgx::Datum::from(ptr)) | |
| Interval(pg_sys::Datum::from(ptr)) |
extension/src/heartbeat_agg.rs
Outdated
| interval_len: i64, | ||
| buffer: Vec<i64>, | ||
| liveness: Vec<(i64, i64)>, // sorted array of non-overlapping (start_time, end_time) | ||
| } | ||
|
|
||
| impl HeartbeatTransState { | ||
| pub fn new(start: i64, end: i64, interval: i64) -> Self { | ||
| assert!(end - start > interval); |
There was a problem hiding this comment.
Can this be triggered by user input? If so, is the error message comprehensible and can we test it?
Same for another assertion below.
There was a problem hiding this comment.
I've added a more explicit error message here and to insert that should make it clear to users what went wrong in this case.
This adds a new postgres aggregate to our toolkit that can be use to measure the liveness of a system given regular heartbeats.
WireBaron
force-pushed
the
br/heartbeat_agg
branch
from
4bb6dc4
to
d08aa61
Compare
|
bors r+ |
|
Build succeeded:
|
This adds a new postgres aggregate to our toolkit that can be use to measure the liveness of a system given regular heartbeats.
Fixes #578