Skip to content

Check the real memory circuit breaker when building global ordinals#102462

Merged
iverase merged 5 commits intoelastic:mainfrom
iverase:cb-global-ordinals
Nov 22, 2023
Merged

Check the real memory circuit breaker when building global ordinals#102462
iverase merged 5 commits intoelastic:mainfrom
iverase:cb-global-ordinals

Conversation

@iverase
Copy link
Copy Markdown
Contributor

@iverase iverase commented Nov 22, 2023

Currently global ordinals are built without any protection against out of memory errors, even though they can use quite a bit of heap. This PR improves the way we build global ordinals by checking the parent circuit breaker every 8192 65536 calls to TermsEnum#next.

@iverase iverase requested review from jpountz and martijnvg November 22, 2023 10:47
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Nov 22, 2023
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Hi @iverase, I've created a changelog YAML for you.

Copy link
Copy Markdown
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this, this is the kind of approach I had in mind. From a quick look, checking the circuit breaker is not so cheap, but doing it infrequently, every 8192 terms, should help amortize this cost. I wonder if we should do it even more infrequently, e.g. every 64k terms, since only adding 8k terms should not allocate lots of heap

@iverase
Copy link
Copy Markdown
Contributor Author

iverase commented Nov 22, 2023

I upddated it so we do the check every 65536 calls.

@iverase iverase merged commit 5516579 into elastic:main Nov 22, 2023
@iverase iverase deleted the cb-global-ordinals branch November 22, 2023 12:55
Copy link
Copy Markdown
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/Aggregations Aggregations >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.12.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants