Migration to Elasticsearch 5 #254

reuschling · 2017-07-05T15:15:41Z

This is the current result for Elasticsearch 5 migration, as asked for at #249 (from myself).

In this pull request, everything from #219 is included plus some commits for bugfixing (tests are running now at least on my machine, plus the jarhell fixes from #217. This is commit 538ee4d.

Further, I implemented the migration to ES 5. The tests are running, I also have a running server with results that looks subjectively good as a first impression (further testing needed of course). I just reindexed the old ES index into a new one for testing - currently I have no Nominatim server for testing the import code. Regarding the Nominatim import, there was also a regarding commit these days - I didn't rebased it yet for simplicity during the merge.

I am looking forwad to feedback and comments :)

- PhotonQueryBuilder now uses QueryBuilder instead of FilterBuilder (API change in 2.0). However, note that some methods are now deprecated and will be removed in ES 3.0. - Updated elasticsearch-wordending-tokenfilter-filter to ES 2.2 - Groovy module installation now required for local node. - Test gateway type is not NONE anymore (None throws an error). - _id.path has been removed in 2.0 - name.similarity, collector.similarity in mappings.json was throwing errors. Apparently, they well not properly defined (no behavior). A similarity is now only defined for collector.properties.default.similarity.

- updated FilterBuilder -> QueryBuilder (new API) # Conflicts: # src/main/java/de/komoot/photon/App.java

- better index clean up in TagFilterQueryBuilderSearchTest

… ES 2.2.0 including the language specific, ngram-relevant fields 'e.g. en, it' | added jarhell fixes from karussell in issue komoot#217, truncating the mock stuff which is only necessary in maven test scope. Tests are running, so this is safe | Added flag to ignore java 7 'broken javaDoc means compile error' behaviour | remove the index after each test case in order to be able to run the test suite (this was left at issue komoot#217). This comprises modifications in ESBaseTester and TagFilterQueryBuilderSearchTest

…rrors with ES 5.4.1. in order I can work. Changed nothing else so code diffs are possible afterwards.

…removed wordending

…m tokenizer

…s now requiered by ES in order to run it embedded | Excluding jna because it yields to a bad version conflict with the jvm-own native interface.

…s used for testing

simonpoole · 2017-07-07T15:31:32Z

Maybe komoot @ChristophLing could indicate if that have any interest in further improvements or if a fork or a move of the repo to new maintainers would be preferred.

karussell · 2017-07-07T16:09:38Z

@simonpoole this is not necessary IMO as @lonvia has access.

@lonvia provides some help (and we the server) in the background to let @reuschling test this PR for bigger data sets, if this works we'll try this for the world wide export. Maybe we should better communicate this activity though.

…he source code

…y against the distance to the given location in the location bias request. This would be distanceSort=true, which is also the default. 'false' gives the former behaviour.

hbruch · 2017-11-05T20:41:18Z

src/main/java/de/komoot/photon/query/PhotonRequestFactory.java

@@ -42,13 +53,25 @@ public PhotonRequestFactory(Set<String> supportedLanguages) {
        } catch (Exception nfe) {
            //ignore
        }
+        Boolean locationDistanceSort = true;


I'd suggest to switch to 'false' as default. While I like the distance sort (=stronger bias) when zoomed to greater zoom levels, the former results are closer to my expectations when zoomed out. E.g. for Stuttgart with a location bias of (51;9) I would expect to see Stuttgart in the results, rather than 'Stuttgarter Straße' in Frankfurt or Kassel. Perhaps providing a zoom level or weighting factor for the bias would be an option for a later PR.

Hmmh, probably we should argue then how it was before (I think it was kind of broken ie it should be indeed false) because if you argue on expectation I can give you a counter argument like when you search for "bahnhof" I would expect the train station within that city.

Perhaps providing a zoom level or weighting factor for the bias would be an option for a later PR.

Related to #198 ... probably we could also provide a bounding box filter.

karussell · 2017-11-06T07:16:36Z

And I'm worried that those who only look for a search-as-you-type solution now need to pay with bigger hardware for a feature they don't need.

Please @reuschling correct me if I'm wrong but I think even without reverse geocoding we need the geo index to provide the location bias.

reuschling · 2017-11-06T08:36:10Z

@karussell yes indeed, geocoding is a must for searching and sorting against geo coordinates, even to provide the photon standard functionalities. @lonvia I removed the unnecessary jar, thanks for finding this. @hbruch, @karussell I would also go for the bounding box or a radius, to keep the sorting criteria well defined. We should also discuss this in a different issue.

Perform first queries with zero fuzziness | Fix throws declaration | Adapt tests to perform zero fuzziness query

reuschling · 2017-11-06T08:53:44Z

@hbruch provided a nice solution for the 'Steigstr Stuttgart' issue. I merged it, thanks for this again.

karussell · 2017-11-06T09:30:35Z

Thanks! I've deployed the recent change to the test server and ran the benchmark #263 - now the counts for every subtests are identical to the current master. (Haven't looked into the details though)

karussell · 2017-11-06T10:35:16Z

The response times are worse than before except for France: poland 80sec, USA 54s, Germany 60s, France1 31s, France2 350s

But the single runs are sometimes different in itself - so I'll have to make the measurement more robust as a first step

karussell · 2017-11-06T14:40:54Z

sorry for the bubble, was a measurement problem: Germany 18s, USA 17s, poland 6sec, France1 9s, France2 100s

karussell · 2017-11-24T15:41:48Z

@lonvia do you agree or disagree regarding mergeability of this PR?

lonvia

Next time please make sure you only address one issue per PR.

karussell · 2017-12-17T00:04:09Z

Thanks @reuschling, @lonvia, @hbruch and finally the initiator @masda70!

karussell · 2017-12-17T00:12:00Z

btw: @reuschling if you connect your git commit email to github, the commits will be linked to your account and you'll also appear as contributor in the stats.

imadk · 2017-12-27T02:48:00Z

Is the radius parameter public? how would an api call look like?

karussell · 2017-12-27T12:52:29Z

Please ask questions on the mailing list

masda70 and others added 21 commits February 10, 2016 17:05

- clean up tabs in App.java

def7fce

Merge branch 'develop' into feature/multiple_reverse_results

68325c2

- updated FilterBuilder -> QueryBuilder (new API) # Conflicts: # src/main/java/de/komoot/photon/App.java

- remove useless file

aed0c07

- multiple reverse results (limit and radius params)

fcba44c

lat/lon range checks

f0e7593

- properly limit reverse query results

9cb1605

strictly positive limit/radius

c634361

- similarity BM25 is now default

ea48f5e

- fix jar hell problems when setting up photon server

4c15e17

- removed deletebyquery plugin for tests

ff6eb8f

- better index clean up in TagFilterQueryBuilderSearchTest

changed source format - nothing else - in files that have pcompiler e…

bdf0269

…rrors with ES 5.4.1. in order I can work. Changed nothing else so code diffs are possible afterwards.

migration of es modules: new version (5.4.1) of groovy and painless, …

078a7cb

…removed wordending

migration of function score scripts. Changed to painless.

6e89723

migrating index settings to ES 5: removed wordending, using edge_ngra…

502be30

…m tokenizer

changed to ES version 5.4.1 (the current one), added some dependencie…

aba9510

…s now requiered by ES in order to run it embedded | Excluding jna because it yields to a bad version conflict with the jvm-own native interface.

bugfix: json object was not closed

249b2f2

code migration to ES 5.4.1

e151faa

test code migration to ES 5.4.1 | migration of reference query string…

2b73519

…s used for testing

code cleanup - removing unnecessary comments

fa29784

This was referenced Jul 6, 2017

Update to ES 5.x? #249

Closed

Reverse geocoding performance improvements and Radius/Limit parameters #219

Closed

Update to ES 2.2 #217

Closed

reuschling added 4 commits July 14, 2017 14:01

switched to ES version 5.5

baa413c

switched from external score scripts to internal ones, specified in t…

6b89d14

…he source code

added new request parameter 'distanceSort=true|false' for sorting onl…

c321b06

…y against the distance to the given location in the location bias request. This would be distanceSort=true, which is also the default. 'false' gives the former behaviour.

fix tests again - query has changed because of inline scripting

cbc8750

hbruch reviewed Nov 5, 2017

View reviewed changes

hbruch mentioned this pull request Nov 5, 2017

Unable to find address even if exact match #221

Closed

removed unnecessary lang-painless-5.4.1.jar

f558c3e

Perform first queries with zero fuzziness (#1)

3a9225d

Perform first queries with zero fuzziness | Fix throws declaration | Adapt tests to perform zero fuzziness query

karussell approved these changes Nov 6, 2017

View reviewed changes

This was referenced Nov 6, 2017

Fix logging problems #266

Closed

Improve error handling #267

Closed

Provide distance or bounding box filter #268

Closed

lonvia approved these changes Dec 16, 2017

View reviewed changes

karussell merged commit 344c694 into komoot:master Dec 17, 2017

karussell added the enhancement label Dec 17, 2017

karussell added this to the 0.3.0 milestone Dec 21, 2017

karussell mentioned this pull request Dec 23, 2017

Location Preference does not appear to be honored #198

Closed

hbruch mentioned this pull request Jan 9, 2018

How to pre-install ES plugins? #264

Closed

karussell mentioned this pull request Jan 15, 2018

Worse reverse geocoding #295

Closed

karussell mentioned this pull request Apr 15, 2018

Elasticsearch 6 Support pelias/pelias#719

Closed

23 tasks

hbruch mentioned this pull request Oct 12, 2018

AbstractReverseRequestHandler should have a changeable limit parameter #190

Closed

lonvia mentioned this pull request Mar 9, 2023

Reverse search could support osm tag filters #703

Closed

jbelien mentioned this pull request Jul 21, 2023

[Photon] Reverse Geocoding with query filters geocoder-php/Geocoder#1195

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration to Elasticsearch 5 #254

Migration to Elasticsearch 5 #254

reuschling commented Jul 5, 2017

simonpoole commented Jul 7, 2017

karussell commented Jul 7, 2017 •

edited

Loading

hbruch Nov 5, 2017

karussell Nov 6, 2017

karussell commented Nov 6, 2017

reuschling commented Nov 6, 2017

reuschling commented Nov 6, 2017

karussell commented Nov 6, 2017

karussell commented Nov 6, 2017

karussell commented Nov 6, 2017

karussell commented Nov 24, 2017

lonvia left a comment

karussell commented Dec 17, 2017

karussell commented Dec 17, 2017

imadk commented Dec 27, 2017

karussell commented Dec 27, 2017

Migration to Elasticsearch 5 #254

Migration to Elasticsearch 5 #254

Conversation

reuschling commented Jul 5, 2017

simonpoole commented Jul 7, 2017

karussell commented Jul 7, 2017 • edited Loading

hbruch Nov 5, 2017

Choose a reason for hiding this comment

karussell Nov 6, 2017

Choose a reason for hiding this comment

karussell commented Nov 6, 2017

reuschling commented Nov 6, 2017

reuschling commented Nov 6, 2017

karussell commented Nov 6, 2017

karussell commented Nov 6, 2017

karussell commented Nov 6, 2017

karussell commented Nov 24, 2017

lonvia left a comment

Choose a reason for hiding this comment

karussell commented Dec 17, 2017

karussell commented Dec 17, 2017

imadk commented Dec 27, 2017

karussell commented Dec 27, 2017

karussell commented Jul 7, 2017 •

edited

Loading