Skip to content
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

Percolator doesn't reduce CircuitBreaker stats in every case. #5588

Closed

Conversation

martijnvg
Copy link
Member

In the case a filter / query is in a percolator query that uses FieldData, after percolating when the in-memory index gets cleared up the circuit breaker stats don't get reduced. Resulting in a discrepancy between what fielddata and the circuit breaker are reporting.

The fix makes sure that the circuit breaker stats gets reduced even for a segment no shard id can be found, which is the case for the in-memory index the percolator is used to percolate documents.

@s1monw
Copy link
Contributor

s1monw commented Mar 28, 2014

man good catch! I looked at the patch and I think we should rather introduce a special unload listener for this case and try to assert that the listener is never null. I'd also want to make sure that the key is never null - I think this only happens in tests today. you also need to make sure that size is positive otherwise you will increment the breaker!

@martijnvg
Copy link
Member Author

That makes sense! I'll update the PR.

@martijnvg
Copy link
Member Author

@s1monw I updated the PR.

@kimchy
Copy link
Member

kimchy commented Mar 30, 2014

I wonder if it won't be cleaner to have an "Indices" level field data cache listener, that will always reduce the circuit breaker, and will always be added as a listener to the key as well.

Then, we can remove the logic to reduce the circuit breaker in 2 places, and keep only the shard level stats at the shard level listener, and the indices level handling on the indices level (circuit breaker). This will also mean we won't have CirecuitBreaker in the ShardFieldData class. The class name can be cleaner as well, something like IndicesFieldDataCacheListener.

In the future, this will also allow other indices level services to register for this if they need to.

@martijnvg
Copy link
Member Author

@kimchy This makes sense, I'll update the PR.

@s1monw
Copy link
Contributor

s1monw commented Mar 31, 2014

+1 to move on an Indices level...

…oked and updates indices statistics and services about field data loading and unloading.

Moved the circuit breaker memory reducing logic to the IndicesFieldDataCacheListener, so it always reduces the memory used when field data gets unloaded,
this fixes a issue where the circuit breaker didn't get reduced when segments where no shardId could be resolved get cleared up.

Also made sure that exceptions in the percolator service are bubbled up properly.

Closes elastic#5588
@martijnvg
Copy link
Member Author

@kimchy @s1monw I updated the PR.

}

@Override
public void onUnload(FieldMapper.Names fieldNames, FieldDataType fieldDataType, boolean wasEvicted, long sizeInBytes, @Nullable AtomicFieldData fieldData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this assert that the size is positive?

Copy link
Member Author

Choose a reason for hiding this comment

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

The assert makes sense, let me add it

@s1monw
Copy link
Contributor

s1monw commented Apr 1, 2014

one small comment - LGTM otherwise

@martijnvg martijnvg closed this in b745153 Apr 2, 2014
martijnvg added a commit that referenced this pull request Apr 2, 2014
…oked and updates indices statistics and services about field data loading and unloading.

Moved the circuit breaker memory reducing logic to the IndicesFieldDataCacheListener, so it always reduces the memory used when field data gets unloaded,
this fixes a issue where the circuit breaker didn't get reduced when segments where no shardId could be resolved get cleared up.

Also made sure that exceptions in the percolator service are bubbled up properly.

Closes #5588
martijnvg added a commit that referenced this pull request Apr 2, 2014
…oked and updates indices statistics and services about field data loading and unloading.

Moved the circuit breaker memory reducing logic to the IndicesFieldDataCacheListener, so it always reduces the memory used when field data gets unloaded,
this fixes a issue where the circuit breaker didn't get reduced when segments where no shardId could be resolved get cleared up.

Also made sure that exceptions in the percolator service are bubbled up properly.

Closes #5588
martijnvg added a commit that referenced this pull request Apr 2, 2014
…oked and updates indices statistics and services about field data loading and unloading.

Moved the circuit breaker memory reducing logic to the IndicesFieldDataCacheListener, so it always reduces the memory used when field data gets unloaded,
this fixes a issue where the circuit breaker didn't get reduced when segments where no shardId could be resolved get cleared up.

Also made sure that exceptions in the percolator service are bubbled up properly.

Closes #5588
@martijnvg martijnvg deleted the bug/percolator_circuit_breaker branch May 18, 2015 23:32
@clintongormley clintongormley added the :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload label Jun 7, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
…oked and updates indices statistics and services about field data loading and unloading.

Moved the circuit breaker memory reducing logic to the IndicesFieldDataCacheListener, so it always reduces the memory used when field data gets unloaded,
this fixes a issue where the circuit breaker didn't get reduced when segments where no shardId could be resolved get cleared up.

Also made sure that exceptions in the percolator service are bubbled up properly.

Closes elastic#5588
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
…oked and updates indices statistics and services about field data loading and unloading.

Moved the circuit breaker memory reducing logic to the IndicesFieldDataCacheListener, so it always reduces the memory used when field data gets unloaded,
this fixes a issue where the circuit breaker didn't get reduced when segments where no shardId could be resolved get cleared up.

Also made sure that exceptions in the percolator service are bubbled up properly.

Closes elastic#5588
@justies
Copy link

justies commented Dec 7, 2015

incase if I have a diffferent types and having a common name of a field created by.When I querying with that field with respect to a particular type , will it load the data from all type to the heap memory or only from the type which i have refered. due to this frequent circuitbreaker exception occuring.. please answer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload v1.0.3 v1.1.1 v1.2.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants