Skip to content

Break on FieldData when building global ordinals #108875

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

iverase
Copy link
Contributor

@iverase iverase commented May 21, 2024

Currently when building global ordinals, we add to the breaker the size of the final object without breaking. I think the reason for not breaking is that it makes little sense to break at that point if we already build the object. The side effect is that we don't honour the breaker limit which is equally bad.

Therefore this commit proposes to add the bytes and break if we get over the limit of the breaker. the side effect is that working cluster might be hitting this breaker now. Still not honouring a limit should be considered a bug.

closes #97075

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 21, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine
Copy link
Collaborator

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

@iverase iverase requested a review from jpountz May 21, 2024 17:46
@nik9000
Copy link
Member

nik9000 commented May 21, 2024

I think this is the right thing to do, but:

  1. I'm sick. Don't trust me.
  2. I thought we already did this.
  3. I can see why we might not want to change it.

@benwtrent
Copy link
Member

This seems like the safe thing to do. Why wouldn't we want to check the CB on ordinal creation to protect users?

@iverase
Copy link
Contributor Author

iverase commented Jul 10, 2024

@elasticmachine update branch

@iverase
Copy link
Contributor Author

iverase commented Jul 10, 2024

@elasticmachine update branch

@wchaparro
Copy link
Member

@iverase Did we want to pursue this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fielddata circuit breaker doesn't seem to limit cache size
7 participants