Support for time bucketed fixed size #968
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
inahga
force-pushed
the
inahga/fixed-size-time-bucketed
branch
from
f3ea55f
to
69daf64
Compare
inahga
force-pushed
the
inahga/fixed-size-time-bucketed
branch
5 times, most recently
from
1b8c84d
to
8c08570
Compare
There was a problem hiding this comment.
I was able to successfully smoke test this in a kind cluster with a Janus leader with this PR divviup/janus#3001, and a Janus helper on 0.7.4.
I'm going to write an in_cluster test that fully exercises this query type, but for now the divviup-api plumbing is complete.
| ValidationError::new("time-bucketed-fixed-size-unsupported"), | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Notice that we're only checking the leader for support here, as noted in #291.
We don't withhold the batch_time_window_size parameter when submitting to the helper. If the helper doesn't directly support the parameter, I'm assuming that it's going to ignore it, i.e. the default behavior of serde.
Note that even without helper direct support, the query_type JSON blob is stored verbatim in the Janus database. So if for some reason the helper needs to become aware that it's running TimeBucketedFixedSize, the knowledge is there.
inahga
force-pushed
the
inahga/fixed-size-time-bucketed
branch
from
ba8e981
to
2493c2d
Compare
|
Hmm, recalling that #874 is a problem, I probably should have gone with the approach of storing the query type explicitly in the database rather than having it continue to be signaled by nullable columns. Let me pull this back out into draft to explore this. |
This turns out to be a relatively large and risky yak-shave. We need to migrate existing query type definitions into a new column, possibly keeping both sets of columns in sync for a bit. I don't think it will be meaningfully more difficult to migrate the additional |
| match value { | ||
| QueryType::TimeInterval => None, | ||
| QueryType::FixedSize { max_batch_size } => Some(max_batch_size), | ||
| impl From<NewTask> for QueryType { |
There was a problem hiding this comment.
I think this would be better suited as a new method on NewTask that takes &self, rather than a From conversion. Each place we use it, we clone the entire task first.
* UI display support for time bucketed fixed size * Review feedback
inahga
force-pushed
the
inahga/fixed-size-time-bucketed
branch
from
dc5f82f
to
d09c2e6
Compare
Supports #291.
I've not plumbed this through the UI, yet.