state_agg API tweaks #699
state_agg API tweaks #699
Conversation
syvb
force-pushed
the
sv/duration_in-api-tweaks
branch
from
35413a5
to
7e83195
Compare
extension/src/state_aggregate.rs
Outdated
| @@ -671,6 +671,30 @@ fn state_agg_int_trans( | |||
| } | |||
| } | |||
|
|
|||
| /// Makes an interval from an `i64` representing the number of microseconds. | |||
| fn make_interval(time: i64) -> crate::raw::Interval { | |||
There was a problem hiding this comment.
We actually have a impl From<i64> for Interval already in raw.rs that does most of this already. It seems cleaner to just add a justify_interval there as well and use something like justify_interval(time.into()) instead of this function.
There was a problem hiding this comment.
I updated that impl to always justify the intervals it creates – I don't think we rely on it creating unjustified intervals elsewhere. Outside of state_agg, it's only used for uptime and downtime in heartbeat_agg, where I think those values should be justified as well?
There was a problem hiding this comment.
That works. I was a little reluctant to suggest this as I'm guessing the justification isn't sticky...if we add two justified intervals I don't think the result is guaranteed to be justified. However, I'm pretty sure we never actually do math on these after we convert them to intervals, so that shouldn't be a problem.
|
bors r+ |
699: state_agg API tweaks r=Smittyvb a=Smittyvb - `interpolated_duration_in`/`duration_in`/`interpolated_state_periods`/`state_periods` have the first two arguments swapped: now the aggregate is first and the state is second - `into_values`/`into_int_values` now returns a table with intervals instead of microseconds Co-authored-by: Smitty <[email protected]>
|
This PR was included in a batch that successfully built, but then failed to merge into main. It will not be retried. Additional information: Response status code: 422
{"message":"Required status check \"license/cla\" is expected.","documentation_url":"https://docs.github.com/articles/about-protected-branches"} |
|
I forced the CLA bot to recheck this PR by logging into https://cla-assistant.io/ and clicking "Recheck PRs". bors retry |
699: state_agg API tweaks r=Smittyvb a=Smittyvb - `interpolated_duration_in`/`duration_in`/`interpolated_state_periods`/`state_periods` have the first two arguments swapped: now the aggregate is first and the state is second - `into_values`/`into_int_values` now returns a table with intervals instead of microseconds Co-authored-by: Smitty <[email protected]>
|
Build failed: |
|
Looks like the flaky bors retry |
|
Smittyvb wrote:
Looks like the flaky |test_rollups| test failed again.
Hmmm, did we know that one to be flaky? I only remember
`stats_agg::tests::pg_stats_agg_fuzz`...
(not that it needs to block merging this, but we probably ought
to file an issue with a list of known flaky tests...)
|
|
Build succeeded: |
It's this one – we added a few days ago and it's been quite flakey. We should probably make it less flakey or ignore it before the release. |
interpolated_duration_in/duration_in/interpolated_state_periods/state_periodshave the first two arguments swapped: now the aggregate is first and the state is secondinto_values/into_int_valuesnow returns a table with intervals instead of microseconds