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

Enable percolation of nested documents #5082

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
4 participants
@brwe
Copy link
Contributor

brwe commented Feb 11, 2014

No description provided.

@@ -0,0 +1,12 @@
package org.elasticsearch.percolator;

This comment has been minimized.

Copy link
@martijnvg

martijnvg Feb 12, 2014

Member

The licence header is missing here. Maybe also add class level jdocs here?

This comment has been minimized.

Copy link
@brwe

brwe Feb 12, 2014

Author Contributor

added, see new commit

@martijnvg

View changes

src/main/java/org/elasticsearch/percolator/MultiDocumentPercolatorIndex.java Outdated

MemoryIndex getMemoryIndex() {
final long maxReuseBytes = settings.getAsBytesSize("indices.memory.memory_index.size_per_thread", new ByteSizeValue(1, ByteSizeUnit.MB)).bytes();
return new ExtendedMemoryIndex(true, maxReuseBytes);

This comment has been minimized.

Copy link
@martijnvg

martijnvg Feb 12, 2014

Member

Should we cache the MemoryIndex instances in a thread local (with a list or something like that)? I lean towards not, since we would then have multiple instances per thread and then then the indices.memory.memory_index.size_per_thread setting doesn't make sense. In that case we can just instantiate MemoryIndex directly (not use the custom impl).

This comment has been minimized.

Copy link
@brwe

brwe Feb 12, 2014

Author Contributor

I am unsure if I understand the size per thread setting. This will only be used if the MemoryIndex is reset, so if I do not have it as a thread local then I can remove this setting?
See c10e099
I am also not sure how we would benefit from having this as a thread local.

@martijnvg

This comment has been minimized.

Copy link
Member

martijnvg commented Feb 12, 2014

+1 Looks great @brwe

I think we can pull this in.

@martijnvg

This comment has been minimized.

Copy link
Member

martijnvg commented Feb 14, 2014

+1 The last commit looks good to me

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.