-
Notifications
You must be signed in to change notification settings - Fork 119
Add downsample to ILMs #538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There is a parameter for the hot, warm and cold phases called downsample. According to the description: "Roll up documents within a fixed interval to a single summary document. Reduces the index footprint by storing time series data at reduced granularity." As of 0.11.0 there is no support for this parameter in the proivider, this PR adds it.
💚 CLA has been signed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this, small question on the schema.
@xaviervolvo are you able to add an entry to the CHANGELOG.md file for this one too? |
Only one downsample section per resource should be allowed.
42d6262
to
476f354
Compare
I think I addressed all the questions, thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM, just was regenerated docs.
I see that acceptance tests are failing, is there anything I can do to solve that? |
@tobio When briefly discussing the current status of the PR with Volvo Cars, they shared the feedback on the failed checks that this might have to do with downsample not being implemented in version prior to 8.10. Could this maybe be the cause and if so how can we move this PR forward? |
It looks like there's three cases here:
We don't usually restrict provider functionality based on the target cluster version (i.e there's no provider level version checks, the resource will just fail). That's not ideal, nicer errors would of course be nicer, I'm just stating the status quo. If a nice error message was added here then that would be lovely, but not necessary. Instead, we usually filter the acceptance tests, the ILM test already does this for the shards per node case. It looks like we should do the same here, i.e create a new test (or two given the 8.5-8.9 case above) case explicitly validating the downscale attributes work, and skipping them on unsupported versions. |
I understand. I haven't familiarised myself with the test setup to the point to see how to filter out tests depending on version? Can you take care of it or would you give me some pointers? Thank you. |
If you have some time to wrap this one up then it'll get merged and released faster :) This test case shows how we're skipping test cases based on the target stack version. Let me know if you've got any questions for that, but we've just been defining a minimum version and using this skip mechanism elsewhere. |
I created skip versions functions and added the test cases for the different versions as mentioned in the comment above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@xaviervolvo thanks for making the time for this addition! |
There is a parameter for the hot, warm and cold phases called downsample.
According to the description:
"Roll up documents within a fixed interval to a single summary document. Reduces the index footprint by storing time series data at reduced granularity."
As of 0.11.0 there is no support for this parameter in the proivider, this PR adds it.