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

HSEARCH-2434 Add support for Elasticsearch 5 #1329

Merged
merged 29 commits into from
Mar 24, 2017
Merged

Conversation

yrodiere
Copy link
Member

@yrodiere yrodiere commented Feb 27, 2017

This PR is based on #1343 , which will have to be merged first. => Done

Relevant JIRA ticket: https://hibernate.atlassian.net/browse/HSEARCH-2434

Compared to #1328, this PR adds the specific code needed to support ES5. To sum up #1328 introduced abstraction layers and interfaces, and this PR introduces alternative implementations of these interfaces.

Don't get fooled by the very high number of additions: I had to copy/paste 4 or 5 large test cases in order to adapt them to ES5. I tried to make them compatible with both ES2 and ES5 first, but the tests ended up almost as complex as the code being tested, so I figured it wasn't a good idea.

@yrodiere
Copy link
Member Author

yrodiere commented Mar 7, 2017

Note: I just pushed an update which added the commit "When validating Elasticsearch schemas, use "default" as a default value for analyzer references" and the next one.
I noticed the issue while working on something else, and since the PR hasn't been merged yet... :]

@yrodiere
Copy link
Member Author

Jenkins, retest this please

1 similar comment
@yrodiere
Copy link
Member Author

Jenkins, retest this please

@yrodiere
Copy link
Member Author

@Sanne This should be ready for review.

About the builds: the previous CI build passed, but the one before failed for no apparent reason, so I'm launching another one to see if it was a transient failure that can be blamed on the slave (concurrent build or whatever) or if there is a randomly occurring problem in Hibernate Search itself.

@yrodiere yrodiere added Ready for review and removed Waiting for other pull request Based on another PR that should be merged first labels Mar 21, 2017
…tead of doing so in elasticsearch.yml

This is required for ES5, where analyzers cannot be configured in
elasticsearch.yml.
…ng to the ES schema

There is no reason to do that, and it forces us to implement yet another
special case for ES5.
…ting in a particular order

The index managers are stored in a hashmap, whose order is undefined.
Strangely, our tests seem to have been working until now.
…iptFields

The hack is unnecessary because we use explicit source filtering
(we have been for a while), so source is always included in the query hits.

Incidentally, this hack is a problem in ES5 since we should use
"stored_fields" instead of "fields".
GeoPoints functions have changed: arcDistanceInKm has been removed,
we must use arcDistance which returns meters.
…pect to cluster status

"yellow" is now the color of a fully-started cluster with no replica, so
we'd better use that in test (where we don't have replicas).
…sticsearch 5

This is necessary because the "refresh" parameter in the Flush API has
been removed in ES5.

See elasticsearch/elasticsearch:7cc48c8e8723d3b31fbcb371070bc2a8d87b1f7e
If we reach the "if ( isNumeric(...)) " in "getType",
it means we don't have any bridge-defined field for this
field name (see the code at the top of "getType").
Thus "FieldHelper.getNumericEncoding(...)" is useless, we can simply
use "DocumentFieldMetadata.getNumericEncoding()".

Also, "DocumentFieldMetadata.isNumeric" already is true if the field
bridge is a NumericFieldBridge or wraps one.
Thus "FieldHelper.isNumeric(...)" is useless, we can simply use
"DocumentFieldMetadata.isNumeric()".
The bounding box query doesn't include geo points located on the left
and bottom sides of the box as of ES 5.
…ment array containing null with ES5

This happens at least on ES 5.0.
Our CI seems to be too slow: 30s isn't enough.
@yrodiere
Copy link
Member Author

Jenkins, retest this please

@Sanne Sanne self-assigned this Mar 21, 2017
@yrodiere
Copy link
Member Author

Jenkins, retest this please

1 similar comment
@yrodiere
Copy link
Member Author

Jenkins, retest this please

@Sanne
Copy link
Member

Sanne commented Mar 24, 2017

There are some problems with the Maven profiles:
if I run with profile elasticsearch-2.2 or elasticsearch-2.0 the "start" goal is no longer valid. I tried to change it to the new goal "runforked" as in the new ES5 profile but then there's another failure relating to not being able to find "org.elasticsearch.plugin:delete-by-query:zip:5.2.1".

The plugin versions and tasks need to have stricter separation among profiles?

@yrodiere
Copy link
Member Author

@Sanne So... This situation is not new, it's been here since I introduced the elasticsearch profiles.

The elasticsearch-5.0 profile is enabled automatically when the testElasticsearchVersion is not defined. I had to do this, because just making this a "default" profile means the profile will only be enabled when no other profile is... So, enable "docs" and you lose the elasticsearch5 profile :/

Unfortunately, "a property is not defined" for Maven means "not defined explicitly", and definitions in explicitly enabled profiles do not count. So in your case, you had both the elasticsearch-2.2 and the elasticsearch-5.0 profiles enabled, and I suppose the elasticsearch-5.0 profile defined the testElasticsearchVersion property, but elasticsearch-2.2 defined its own plugin configuration. Which won't work for several reasons.

So, if you want to run 2.0/2.2 tests, you must either:

  • Stick to the instructions in README.md and provide the testElasticsearchVersion explicitly:
 mvn clean install -Pelasticsearch-2.0 -DtestElasticsearchVersion=2.1.0
  • Or disable the default profile (elasticsearch-5.0) explicitly:
 mvn clean install -P'!elasticsearch-5.0,elasticsearch-2.0'

I'll see if I can change the poms to make things easier, but I doubt it...

@Sanne
Copy link
Member

Sanne commented Mar 24, 2017

ah right! Sorry I forgot about that. No rush with improvements, I'd like to integrate this first and the README instructions are good enough for now.

@Sanne Sanne merged commit 37bf023 into hibernate:master Mar 24, 2017
@Sanne
Copy link
Member

Sanne commented Mar 24, 2017

awesome work!
It's merged.

I added one commit to address a couple of super trivial javadoc copy/paste errors.

@yrodiere yrodiere deleted the es5 branch April 13, 2017 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants