Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Init default point slice pools in models and expr #2004

Merged
merged 2 commits into from
Sep 15, 2021

Conversation

colega
Copy link
Contributor

@colega colega commented Sep 13, 2021

Sometimes those packages could be used outside of the application lifecycle and the point slices should be initialized in order to avoid panics.

Signed-off-by: Oleg Zaytsev mail@olegzaytsev.com

Sometimes those packages could be used outside of the application
lifecycle and the point slices should be initialized in order to avoid
panics.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@Dieterbe
Copy link
Contributor

From devdocs/expr.md:

Note that the expr library can be called by different clients. At this point only Metrictank uses it, but we intend this library to be easily embeddable in other programs.
It's up to the client to instantiate the pool, and set up the default allocation to return point slices of desired point capacity.
The client can then of course use this pool to store series, which it then feeds to expr.
expr library does the rest. It manages the series/pointslices and gets new ones as a basis for the COW.
Once the data is returned to the client, and the client is done using the returned data, it should call plan.Clean(),
which returns all data back to the pool (both input data or newly generated series, whether they made it into the final output or not).

I think the above makes sense, as any caller application will need to deal with point slices before calling expr, and also deal with them after calling expr, so it needs access to the pool, and may as well own it.

See the Pool() functions right under both of your changes.

@colega
Copy link
Contributor Author

colega commented Sep 14, 2021

I see the doc, but I also have seen other programs where usage of these packages just panicked because the slice pool wasn't set before using it. This can get even trickier if this package is a third party dependency.

Why can't we just avoid panics out of the box, leaving the Pool() function for callers that want to provide a custom pool?

@Dieterbe
Copy link
Contributor

I see. I suppose then we can initialize with a default pool, but change the wording in the docs to encourage callers to create their own pool to increase efficiency. (e.g. to leverage the pool during the entire series lifecycle, not just within expr)

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega
Copy link
Contributor Author

colega commented Sep 15, 2021

WDYT about the wording in 20bb93c ?

Copy link
Contributor

@Dieterbe Dieterbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Dieterbe Dieterbe merged commit 5f593b6 into master Sep 15, 2021
@Dieterbe Dieterbe deleted the init-default-point-slices-pools branch September 15, 2021 13:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants