New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 5 users

Issue metadata

Status: Submitted
Owner:
Closed: Jun 4
Cc:
Components:



Sign in to add a comment

Add support for Elasticsearch version 5.x

Project Member Reported by thomasmu...@yahoo.com, Apr 27 2017

Issue description

*****************************************************************
*****                                                       *****
***** !!!! THIS BUG TRACKER IS FOR GERRIT CODE REVIEW !!!!  *****
*****                                                       *****
***** DO NOT SUBMIT BUGS FOR CHROME, ANDROID, CYANOGENMOD,  *****
***** INTERNAL ISSUES WITH YOUR COMPANY'S GERRIT SETUP, ETC.*****
*****                                                       *****
*****   THOSE ISSUES BELONG IN DIFFERENT ISSUE TRACKERS     *****
*****                                                       *****
*****************************************************************

Affected Version:2.14+

What steps will reproduce the problem?
1. Using elasticsearch 5.x does not work with gerrit
2.
3.

What is the expected output?
For elasticsearch 5 to work

What do you see instead?
I see errors

Please provide any additional information below.
Elasticsearch should either be updated or we should be able to configure which version of the lib we can use.
 
Project Member

Comment 1 by thomasmu...@yahoo.com, Apr 27 2017

Or we could use curl http://stackoverflow.com/questions/2586975/how-to-use-curl-in-java

and remove the dependancy on elasticsearch dep.

Comment 2 Deleted

Comment 3 Deleted

Project Member

Comment 5 by thomasmu...@yahoo.com, Jul 28 2017

Status: Accepted (was: New)
Summary: Update elasticsearch to 5.x in gerrit (was: Update elasticsearch to 5.x in gerrit 2.14)
Project Member

Comment 6 by marco.mm...@gmail.com, Mar 12 2018

Components: Elasticsearch

Comment 7 Deleted

Project Member

Comment 8 by thomasmu...@yahoo.com, Mar 12 2018

We should switch to an api that does not depend on the lucent version. 

Which would allow us to support elasticsearch 5, 6, 7 and so on.

Namely using an api that uses the url to fetch it like curl
IIRC the API doesn't depend on the Lucene version, the only reason we are stuck to a specific Lucene is because the Elasticsearch server requires it and we use that in the tests.

There was an attempt to isolate the older Lucene dependency to the tests, but that didn't work in Eclipse because it just puts everything on the classpath.
Project Member

Comment 10 by thomasmu...@yahoo.com, Mar 13 2018

Strange as in phabricator we support versions 2 and 5. I doin't think i understand how elastcsearch lucene version is causing us to be blocked.
> I see errors

Please include the errors in this issue report.

Cc: david.pu...@gmail.com
 Issue 7856  has been merged into this issue.
Project Member

Comment 13 by david.os...@gmail.com, Mar 14 2018

So, the upgrade path could be: upgrade Lucene, upgrade ES and adjust the tests. 
Seems we do have a dep on Lucene from ElasticQueryBuilder, which imports:

org.apache.lucene.search.BooleanQuery


Project Member

Comment 15 by david.os...@gmail.com, Mar 14 2018

My understanding is, that ES depends on Lucene, so that we have
to use the same versions and we could only upgrade to the max.
Lucene version, that is supported by ES.
Yes, but before I thought the only dependency on Lucene was due to using ES during the tests. 

That might have been the case in earlier versions and the runtime dependency in the querybuilder was added later.

Project Member

Comment 17 by marco.mm...@gmail.com, Apr 12

Cc: david.os...@gmail.com
Status: Started (was: Accepted)
Adding both Davids in cc, as not sure about Monorail's behaviour here; no annoyance intended then.

Started working on this overall, by means of this initial change first:
https://gerrit-review.googlesource.com/c/gerrit/+/172010

Not setting myself as Owner, at least not yet, given the initial nature of this work. Same goes for not using the ChangeUnderReview status -yet. But please do feel free to amend, at will.
Project Member

Comment 18 by marco.mm...@gmail.com, Apr 13

'Elasticsearch BUILD: remove unneeded dependencies' <=>
https://gerrit-review.googlesource.com/c/gerrit/+/172250

-Only slightly related to this issue, but still.
That change also contributes, by means of removing extra dependencies, such as lucene-analyzers-common (which is of interest here).
Project Member

Comment 19 by marco.mm...@gmail.com, Apr 13

https://gerrit-review.googlesource.com/c/gerrit/+/172251
<=> lib/jest for Elasticsearch: refactor dependencies.

Project Member

Comment 20 by david.os...@gmail.com, Apr 13

> Seems we do have a dep on Lucene from ElasticQueryBuilder, which imports:
>
> org.apache.lucene.search.BooleanQuery


I don't see, why this catch block is needed. What happens if we remove it: [1]?

* [1] http://paste.openstack.org/show/719197
Project Member

Comment 21 by david.os...@gmail.com, Apr 13

Ah, I see this is a runtime exception:

  /** Thrown when an attempt is made to add more than {@link
   * #getMaxClauseCount()} clauses. This typically happens if
   * a PrefixQuery, FuzzyQuery, WildcardQuery, or TermRangeQuery 
   * is expanded to many terms during search. 
   */
  public static class TooManyClauses extends RuntimeException {
    public TooManyClauses() {
      super("maxClauseCount is set to " + maxClauseCount);
    }
  }

Project Member

Comment 22 by marco.mm...@gmail.com, Apr 13

https://gerrit-review.googlesource.com/c/gerrit/+/172310
<=> lib/elasticsearch: remove unnecessary dependencies.

Now I think the work can start on gradually bumping our hereby versions, also joining the other thread right above.
Project Member

Comment 23 by marco.mm...@gmail.com, Apr 16

https://gerrit-review.googlesource.com/c/gerrit/+/172491
<=> lib/elasticsearch: restore jackson_dataformat_smile
(just for the comprehensive record, still).
Project Member

Comment 24 by marco.mm...@gmail.com, Apr 16

So the tests are currently using the Elasticsearch (native, embedded) Java API -[1].
[1] https://www.elastic.co/guide/en/elasticsearch/client/java-api/2.4/index.html

Production code too, although mostly relying on Jest [2] or REST API use -towards an Elasticsearch cluster.
[2] https://github.com/searchbox-io/Jest

Since then, Elastic have been recommending a newer REST API instead, coming in two paths:
[3] https://www.elastic.co/blog/state-of-the-official-elasticsearch-java-clients
[4] https://www.elastic.co/guide/en/elasticsearch/client/java-rest/current/java-rest-overview.html
-Vs.:
[5] https://www.elastic.co/guide/en/elasticsearch/client/java-api/current/java-api.html

We could then maybe aim at halting our usage of the native Java API [1,5], as well as Jest's ([2]).
All this in light of the [3,5]-listed shortcomings with (mostly) that legacy Java API.

As we potentially do so, we could go incremental with the chosen version(s), given our other Lucene deps in Gerrit.
We would then also start bumping Lucene versions, driven by (based on) such Elastic API changes but also Gerrit.
This would lead us into choosing specific Elasticsearch versions [6] for production too.
[6] https://www.elastic.co/downloads/past-releases

More thoughts on how to pursue this further though?
Project Member

Comment 25 by marco.mm...@gmail.com, Apr 25

Owner: marco.mm...@gmail.com
Resuming our work on this now for the coming weeks.
Project Member

Comment 26 by marco.mm...@gmail.com, May 11

I just replied to [1] about our current status regarding the WIP for this issue, if anyone has been wondering about this.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/177590

Project Member

Comment 27 by marco.mm...@gmail.com, May 18

Cc: huga...@gmail.com
The change [1] to remove the lucene dependency from the elasticsearch client in prod has just been pushed for review:

[1] https://gerrit-review.googlesource.com/c/gerrit/+/179710

Project Member

Comment 28 by marco.mm...@gmail.com, May 28

In case anyone interested in this issue would need to know: more changes were recently merged on top of the aforementioned [1], about also removing such dependencies for the test code.

Next steps are then to adapt production and test (Elasticsearch client) code to more/later Elasticsearch server versions; WIP.
Summary: Add support for Elasticsearch version 5.x (was: Update elasticsearch to 5.x in gerrit)
Tracking support for ES 6.x in  issue 9112 
Project Member

Comment 32 by marco.mm...@gmail.com, May 31

Status: ChangeUnderReview (was: Started)
https://gerrit-review.googlesource.com/c/gerrit/+/182230

Alternative solution uploaded:

https://gerrit-review.googlesource.com/c/gerrit/+/182270

With this all the tests pass, although there is still a deprecation warning:

  "Deprecated field [type] used, replaced by [match_phrase and match_phrase_prefix query]"

This seems to be the same as mentioned here:

https://discuss.elastic.co/t/type-deprecation-warnings/67038

Project Member

Comment 34 by marco.mm...@gmail.com, Jun 1

Currently working on fixing that warning; will let you know -thx for noticing it.
Labels: FixedIn-2.14.9
Status: Submitted (was: ChangeUnderReview)

Sign in to add a comment