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

Fielddata: Remove soft/resident caches. #7443

Closed

Conversation

Projects
None yet
5 participants
@jpountz
Copy link
Contributor

jpountz commented Aug 25, 2014

These caches have no advantage compared to the default node cache. Additionally,
the soft cache makes use of soft references which make fielddata loading quite
unpredictable in addition to pushing more pressure on the garbage collector.

The none cache is still there because of tests. There is no other good
reason to use it.

LongFieldDataBenchmark has been removed because the refactoring exposed a
compilation error in this class, which seems to not having been working for a
long time. In addition it's not as much useful now that we are progressively
moving more fields to doc values.

Fielddata: Remove soft/resident caches.
These caches have no advantage compared to the default node cache. Additionally,
the soft cache makes use of soft references which make fielddata loading quite
unpredictable in addition to pushing more pressure on the garbage collector.

The `none` cache is still there because of tests. There is not other good
reason to use it.

LongFieldDataBenchmark has been removed because the refactoring exposed a
compilation error in this class, which seems to not having been working for a
long time. In addition it's not as much useful now that we are progressively
moving more fields to doc values.

@jpountz jpountz added review labels Aug 25, 2014

@jpountz jpountz self-assigned this Aug 25, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Aug 26, 2014

I agree we should drop them at least in master. I wonder what if we should / need to keep them around in 1.x for a while an deprecate them first so folks know about it? On the other hand this is really bad if you use it today so I am all for dropping?

@jpountz

This comment has been minimized.

Copy link
Contributor Author

jpountz commented Aug 26, 2014

At first I only planned to propose this to master but as you noted using eg. the soft cache today can be pretty bad so maybe it's more helpful to drop them now.

@martijnvg

This comment has been minimized.

Copy link
Member

martijnvg commented Aug 26, 2014

I'm +1 for removing these caches on 1.x as well. I think we should mark this issue as breaking?

Also if someone has an index that is configured to use either soft or resident cache, then after upgrading this indices will fail to load? (maybe not immediately but after close and open or when a shard gets moved to an node that didn't have this index before) So maybe we should still support these settings, but under the hood we will be using the nodes field data cache?

@dakrone

This comment has been minimized.

Copy link
Member

dakrone commented Aug 26, 2014

+1 on removing these from 1.x and master and marking the issue as breaking. Doc values should be the alternative rather than soft caches. Either removing from 1.4 or 1.5 (with deprecation) sounds good to me.

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Aug 26, 2014

sounds good to me /cc @kimchy

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Aug 26, 2014

one questions I have here is should we maybe log if such a cache is configured on an old index instead of silently falling back to the node cache. I also think we should reject these caches if they are configured on an index created on 1.4 or greater?

@s1monw s1monw removed the review label Aug 26, 2014

@jpountz jpountz added breaking and removed v1.4.0 labels Aug 27, 2014

@jpountz jpountz added the v1.4.0 label Aug 27, 2014

@jpountz

This comment has been minimized.

Copy link
Contributor Author

jpountz commented Aug 27, 2014

@s1monw I pushed a new commit. This is basically what the diff would look like on 1.x, on master these caches will just be removed.

// we default to node level cache, which in turn defaults to be unbounded
// this means changing the node level settings is simple, just set the bounds there
String cacheType = type.getSettings().get("cache", indexSettings.get(FIELDDATA_CACHE_KEY, FIELDDATA_CACHE_VALUE_NODE));
if (FIELDDATA_CACHE_VALUE_RESIDENT.equals(cacheType)) {
if (pre14 && FIELDDATA_CACHE_VALUE_RESIDENT.equals(cacheType)) {

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 27, 2014

Contributor

hmm should we log this too?

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 27, 2014

Contributor

I think we should have a bwc test for this that it can be set on indices that are created with < 1.4

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 27, 2014

Author Contributor

I think it is logged?

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 27, 2014

Contributor

nevermind I misread the diff

jpountz added some commits Aug 27, 2014

@jpountz

This comment has been minimized.

Copy link
Contributor Author

jpountz commented Aug 27, 2014

@s1monw updated

@jpountz jpountz added the review label Aug 27, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Aug 27, 2014

LGTM

@jpountz jpountz closed this in b745b01 Aug 27, 2014

jpountz added a commit that referenced this pull request Aug 27, 2014

Fielddata: Deprecate soft/resident caches.
These caches have no advantage compared to the default node cache. Additionally,
the soft cache makes use of soft references which make fielddata loading quite
unpredictable in addition to pushing more pressure on the garbage collector.

The `none` cache is still there because of tests. There is not other good
reason to use it.

LongFieldDataBenchmark has been removed because the refactoring exposed a
compilation error in this class, which seems to not having been working for a
long time. In addition it's not as much useful now that we are progressively
moving more fields to doc values.

Close #7443

@jpountz jpountz removed the review label Aug 27, 2014

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

Fielddata: Remove soft/resident caches.
These caches have no advantage compared to the default node cache. Additionally,
the soft cache makes use of soft references which make fielddata loading quite
unpredictable in addition to pushing more pressure on the garbage collector.

The `none` cache is still there because of tests. There is no other good
reason to use it.

LongFieldDataBenchmark has been removed because the refactoring exposed a
compilation error in this class, which seems to not having been working for a
long time. In addition it's not as much useful now that we are progressively
moving more fields to doc values.

Close #7443
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.