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 should cache index field data instances. #7081

Merged

Conversation

Projects
None yet
3 participants
@martijnvg
Copy link
Member

commented Jul 29, 2014

Before the index reader used by the percolator didn't allow to register a CoreCloseListener, but now it does, making it safe to cache index field data cache entries.

Creating field data structures is relatively expensive and caching them can save a lot of noise if many queries are evaluated in a percolator call.

Closes #6806

@@ -188,11 +188,7 @@ public Filter cacheFilter(Filter filter, @Nullable CacheKeyFilter.Key cacheKey)
}

public <IFD extends IndexFieldData<?>> IFD getForField(FieldMapper<?> mapper) {
if (disableCaching) {
return indexQueryParser.fieldDataService.getForFieldDirect(mapper);

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 30, 2014

Contributor

Can we remove this method now?

This comment has been minimized.

Copy link
@martijnvg

martijnvg Jul 31, 2014

Author Member

I think getForField can stay? Because it is shorter, otherwise all code needs to do context.fielddata() and then invoke getForField()

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 1, 2014

Contributor

Sorry, I wanted to remove getForFieldDirect, not getForField

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2014

The change looks good to me. Should we add assertions in the percolation tests to ensure that the field data cache is empty at the end of tests?

@martijnvg

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2014

@jpountz Good idea, but should this be a general check? (Like how we check the circuit breaker after each test)

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2014

@martijnvg Probably!

@jpountz jpountz removed the review label Jul 31, 2014

@martijnvg

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2014

@jpountz I updated the PR to check if the filter cache and field data cache is 0 before stopping the test cluster.

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2014

LGTM

Percolator should cache index field data instances.
Before the index reader used by the percolator didn't allow to register a CoreCloseListener, but now it does, making it safe to cache index field data cache entries.
Creating field data structures is relatively expensive and caching them can save a lot of noise if many queries are evaluated in a percolator call.

Closes #6806
Closes #7081

@martijnvg martijnvg merged commit c8cc59d into elastic:master Aug 4, 2014

martijnvg added a commit that referenced this pull request Aug 4, 2014

Percolator should cache index field data instances.
Before the index reader used by the percolator didn't allow to register a CoreCloseListener, but now it does, making it safe to cache index field data cache entries.
Creating field data structures is relatively expensive and caching them can save a lot of noise if many queries are evaluated in a percolator call.

Closes #6806
Closes #7081

martijnvg added a commit that referenced this pull request Aug 4, 2014

Percolator should cache index field data instances.
Before the index reader used by the percolator didn't allow to register a CoreCloseListener, but now it does, making it safe to cache index field data cache entries.
Creating field data structures is relatively expensive and caching them can save a lot of noise if many queries are evaluated in a percolator call.

Closes #6806
Closes #7081

martijnvg added a commit that referenced this pull request Sep 8, 2014

Percolator should cache index field data instances.
Before the index reader used by the percolator didn't allow to register a CoreCloseListener, but now it does, making it safe to cache index field data cache entries.
Creating field data structures is relatively expensive and caching them can save a lot of noise if many queries are evaluated in a percolator call.

Closes #6806
Closes #7081

@clintongormley clintongormley changed the title Percolator should cache index field data instances. Percolator: Percolator should cache index field data instances. Sep 8, 2014

@martijnvg martijnvg deleted the martijnvg:improvements/percolator_cache_fd branch May 18, 2015

@clintongormley clintongormley changed the title Percolator: Percolator should cache index field data instances. Percolator should cache index field data instances. Jun 7, 2015

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

Percolator should cache index field data instances.
Before the index reader used by the percolator didn't allow to register a CoreCloseListener, but now it does, making it safe to cache index field data cache entries.
Creating field data structures is relatively expensive and caching them can save a lot of noise if many queries are evaluated in a percolator call.

Closes elastic#6806
Closes elastic#7081
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.