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

Upgrade to Lucene 4.10 #7584

Closed
wants to merge 28 commits into from

Conversation

Projects
None yet
7 participants
@rmuir
Copy link
Contributor

rmuir commented Sep 4, 2014

No description provided.

rmuir and others added some commits Jun 30, 2014

Merge branch 'master' into enhancement/lucene_4_10_upgrade
Note:
src/main/java/org/elasticsearch/action/termvector/TermVectorFields.java
is still unresolved

Conflicts:
	src/main/java/org/elasticsearch/action/termvector/TermVectorFields.java
	src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentParser.java
	src/main/java/org/elasticsearch/index/fielddata/FieldData.java
	src/main/java/org/elasticsearch/search/facet/terms/strings/HashedScriptAggregator.java
	src/main/java/org/elasticsearch/search/facet/terms/strings/TermsStringOrdinalsFacetExecutor.java
	src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java

@rmuir rmuir added v1.4.0 labels Sep 4, 2014

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Sep 4, 2014

LGTM

@jpountz jpountz removed the review label Sep 4, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Sep 4, 2014

LGTM

@rjernst

This comment has been minimized.

Copy link
Member

rjernst commented Sep 4, 2014

I made the merge policy changes, but I have not added a test. There are no existing tests, but there really should be one for each provider checking that the settings can actually be set, and updated. I'll work on these separately, though, unless someone feels it is important to block the upgrade on adding these tests.

@rjernst rjernst added the enhancement label Sep 4, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Sep 4, 2014

looks good

@kimchy

This comment has been minimized.

Copy link
Member

kimchy commented Sep 4, 2014

@rjernst what about the NamedAnalyzer? I think it should use the same strategy as the analyzer it wraps, no?

@rmuir

This comment has been minimized.

Copy link
Contributor Author

rmuir commented Sep 4, 2014

I am sure I made the NamedAnalyzer commit. I just followed Uwe's approach for PerFieldAnalyzerWrapper: this strategy is in fact not used, except as a fallback in the case you wrap namedanalyzer in yet another wrapper (such as shingles) that isnt a pure delegator.

So its just "safety", but maybe there is a way we could just have an error if we do such a thing instead of increased memory usage? Personally I still would prefer if delegation and wrapping use-cases were totally separate and type-safe, but my head is still recovering from looking at this stuff the last time.

@kimchy

This comment has been minimized.

Copy link
Member

kimchy commented Sep 4, 2014

@rmuir yea, I think that in practice, it doesn't matter in the context of NamedAnalyzer, since its just a wrapper. I just think in practice, its cleaner, since NamedAnalyzer is just a delegate, and thats it, not similar to PerfieldAanalyzerWrapper, because of it, I think its cleaner (in the scope of just the NamedAnalyzer impl) to use the same reuse strategy as the analyzer it wraps.

@rmuir

This comment has been minimized.

Copy link
Contributor Author

rmuir commented Sep 4, 2014

Right i think the potential for a bug only happens if:

  1. NamedAnalyzer wraps something that is per-field (e.g. a PerFieldAnalyzerWrapper)
  2. this NamedAnalyzer is wrapped by something that modifies the chain (e.g. shingles or adds charfilter or whatever)

The confusing thing is the API for this "strategy" its passing, again if elasticsearch isn't doing crazy things (and i think its not), then this strategy is never really used:

 /**
  * Constructor.
  * @param fallbackStrategy is the strategy to use if delegation is not possible
  *  This is to support the common pattern:
  *  {@code new OtherWrapper(thisWrapper.getReuseStrategy())} 
  */
 protected DelegatingAnalyzerWrapper(ReuseStrategy fallbackStrategy) {

I guess my suggestion is whether we should force an error in such a "fallback" case, one evil way is to pass ThrowsExceptionOnEveryOperationReuseStrategy instead. There might be cleaner ways.

@kimchy

This comment has been minimized.

Copy link
Member

kimchy commented Sep 4, 2014

I am up for ThrowsExceptionOnEveryOperationReuseStrategy, I think that if we get into a case where we don't fallback, it smells like a bug in ES

@rmuir

This comment has been minimized.

Copy link
Contributor Author

rmuir commented Sep 4, 2014

I think those are the 3 choices we have today (since unfortunately we dont have great type safety):

  1. if we screw up, its safe but will chew up threadlocals (slower). thats what branch does.
  2. if we screw up, its buggy. thats what passing the wrapped strategy does.
  3. if we screw up, we get an error and tests fail.

Personally I am for option 3 as well. I will try to think of a cleaner way.

@uschindler

This comment has been minimized.

Copy link
Contributor

uschindler commented on dd6cdd0 Sep 5, 2014

+1, maybe change the message: "NamedAnalyzer cannot be wrapped with anaother analyzer, must always be top-level."

This comment has been minimized.

Copy link
Contributor Author

rmuir replied Sep 5, 2014

Thanks Uwe, I improved the error here: 890274b

The idea is that this fallbackReuseStrategy should never be actually used, its ok to wrap this analyzer with a delegator, which will never call it, but if this is called it means we misconfigured something (e.g. wrapped with a real wrapper that changes tokenstreams) and reuse is not working the way we expect.

@rmuir

This comment has been minimized.

Copy link
Contributor Author

rmuir commented Sep 5, 2014

What is left to do here?

@rmuir rmuir added the review label Sep 5, 2014

@kimchy

This comment has been minimized.

Copy link
Member

kimchy commented Sep 5, 2014

LGTM

@rmuir rmuir closed this in 223dab8 Sep 5, 2014

rmuir added a commit that referenced this pull request Sep 5, 2014

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

[Lucene] Upgrade to Lucene 4.10
Closes #7584

Conflicts:
	src/main/java/org/apache/lucene/search/suggest/analyzing/XAnalyzingSuggester.java
	src/main/java/org/elasticsearch/search/suggest/completion/Completion090PostingsFormat.java
	src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggester.java

@clintongormley clintongormley changed the title Upgrade to Lucene 4.10 Internal: Upgrade to Lucene 4.10 Sep 8, 2014

@jpountz jpountz removed the review label Oct 21, 2014

@s1monw s1monw deleted the enhancement/lucene_4_10_upgrade branch Apr 21, 2015

@clintongormley clintongormley changed the title Internal: Upgrade to Lucene 4.10 Upgrade to Lucene 4.10 Jun 7, 2015

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.