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

Disable bloom filters #8572

Closed
wants to merge 2 commits into from
Closed

Disable bloom filters #8572

wants to merge 2 commits into from

Conversation

rmuir
Copy link
Contributor

@rmuir rmuir commented Nov 20, 2014

See #8571 and #8564

I make the "es090" postings format read-only, just to support old segments. There is a test version that subclasses it with write-capability for testing.

@s1monw
Copy link
Contributor

s1monw commented Nov 20, 2014

it looks awesome - I know that we have StoreDirectory#codecService() that is only used by the bloom stuff, I also think we should at least note this in migration doc for 2.0 and I wonder what happens to fields that have bloom configured as their postings format - should we auto-upgrade those fields?

@rmuir
Copy link
Contributor Author

rmuir commented Nov 20, 2014

oh, yes this is no longer used, lemme see if i can clean it up more.

to answer your question, es090 fields will read with the old code, but we never load blooms. we just read the delegate PF name from the .blm file.

as far as "auto-upgrade", they will be converted by normal merging and by the upgrade api in 2.0. For 1.x we would need additional logic to force the upgrade API to target these, since the lucene codec major version will not have changed, it will think they are up to date.

@rmuir
Copy link
Contributor Author

rmuir commented Nov 20, 2014

pushed commit removing StoreDirectory.codecService(). DirectoryUtils.getStoreDirectory is still used by other code.

As far as migration doc, I am unsure what, if any text we should add. We will be able to read the old segments so there is not much for the user to do?

@s1monw
Copy link
Contributor

s1monw commented Nov 20, 2014

As far as migration doc, I am unsure what, if any text we should add. We will be able to read the old segments so there is not much for the user to do?

All I'd add is something like this:

The `bloom` postingsformat has been removed in 2.0. This postingsformat can't be used in mappings anymore.

as far as "auto-upgrade", they will be converted by normal merging and by the upgrade api in 2.0. For 1.x we would need additional logic to force the upgrade API to target these, since the lucene codec major version will not have changed, it will think they are up to date.

sorry I was unclear... I meant if somebody has bloom in it's mapping we need to replace it with whatever is default since we can't write it anymore. The upgrade API doesn't do that so I think we need to add some code to the mapping that logs a warning and replaces "bloom" with whatever is our default.

@rmuir
Copy link
Contributor Author

rmuir commented Nov 20, 2014

I'm confused, where is this 'bloom' option in the mappings? there is no reference to it in the documentation.

@s1monw
Copy link
Contributor

s1monw commented Nov 20, 2014

bq. I'm confused, where is this 'bloom' option in the mappings? there is no reference to it in the documentation.

yeah nevermind I was talking about the SPI loader etc...

@mikemccand
Copy link
Contributor

We could maybe remove StoreDirectory entirely? Does it have a purpose beyond passing that bloom setting?

@s1monw
Copy link
Contributor

s1monw commented Nov 20, 2014

We could maybe remove StoreDirectory entirely? Does it have a purpose beyond passing that bloom setting?

yes it does I already spoke to rob about how to remove it I think we can but it might requrie some modification to lucene too to allow associating a shard ID with and index reader / leaf reader

@rmuir
Copy link
Contributor Author

rmuir commented Nov 20, 2014

Mike there is other code using this stuff (unfortunately). The reason is, some code wants to know what shard a segment reader belongs to. we should think about a better way to do this, but i think its out of scope here.

@mikemccand
Copy link
Contributor

OK let's not do it here.

@mikemccand
Copy link
Contributor

LGTM

1 similar comment
@s1monw
Copy link
Contributor

s1monw commented Nov 20, 2014

LGTM

@s1monw
Copy link
Contributor

s1monw commented Nov 23, 2014

@rmuir did you merge this? I think it can be closed?

@rmuir
Copy link
Contributor Author

rmuir commented Nov 23, 2014

yes, i only closed the issue, or the pull request, or whichever this one isnt. Github is a mess

@rmuir rmuir closed this Nov 23, 2014
@clintongormley clintongormley changed the title disable bloom filters Disable bloom filters Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants