Finalize elasticsearch 5 migration #1242

Tobion · 2017-01-31T20:13:51Z

Some things that are missing for #1184

Todo:

remove features that do not exist in ES 5.0
- e.g. missing, ttl, top_children
deprecate things that are also deprecated in ES 5.0
- e.g. minimum_number_should_match, geo_distance_range, Query\Match::setFieldType (see Update match query documentation elastic/elasticsearch#17536 )
Elastica\Script namespace pretty broken. Also stored script and referencing them by id is not implemented
- refactored as much as possible without breaking BC. unfortunately the design of the scripts extending AbstractUpdateAction is not good. scripts can be part of an update action but they are not the action itself. so the $id parameter that I renamed to $documentId (which was hard to grasp) doesn't belong there at all. but changing that would require rewrite of the scripts logic.
- added a ScriptId class to reference stored scripts. This was completely missing so far.
fix error parsing to exception (it already has a todo in code) (see Fix reading bool index setting & deprecate ElasticsearchException #1251)
MatchPhrase and MatchPhrasePrefix should NOT extend Match because they don't share the same options
fix skipped tests
There is not routing parameter when creating an index (and has never been). So this feature has always been useless but ES 5 now validates the wrong param. Creating aliases has the possibility to specify routing and a filter. But the alastica api doesn't provide those params yet, which could be added in the future (there is already an issue about this: Implement Elastica_Alias to handle alias create and features #297)

ruflin · 2017-02-01T09:31:52Z

@Tobion Thanks for taking this on. Can you ping me when I should have a look?

Tobion · 2017-02-03T18:20:20Z

@ruflin This should be ready now. Please have a look. The last piece I need to look at is the error parsing that seems to be a little strange at the moment (not giving as much info as possible). But identifying errors/success seems to work according to the tests. So I can do that in a separate PR.

ruflin · 2017-02-06T12:23:19Z

@Tobion Great work. Any chance you could update the CHANGELOG with some notes on the change? A summary as you have in the PR works for me.

ruflin

I went quickly through the changes. In general LGTM, but there are some parts where we have bigger changes which could require a bit more discussions. Could you split up this PR into multiple smaller PR's very similar to the commits you already have?

For example one PR with all the docs cleanup, an other (or multiple) with removal of features which do not exist anymore and so on. All in all the outcome will be the same, but in case we have some more discussion for one part, it does not hold back the others.

Let me know if that works for you?

ruflin · 2017-02-06T12:24:50Z

lib/Elastica/Bulk/Action/AbstractDocument.php

     */
    protected $_data;

    /**
-     * @param \Elastica\Document|\Elastica\Script\AbstractScript $document
+     * @param Document|AbstractScript $document


How does it know which is the correct path to Document as it is in a different path?

I don't understand the question. Elastica\Document is already imported via use statement.

The question probably is, if phpdoc automatically also detects this. So I assume the answer is yes.

ruflin · 2017-02-06T12:25:45Z

lib/Elastica/Index.php

-                } catch (ResponseException $e) {
-                    // Table can't be deleted, because doesn't exist
-                }
+        if (is_bool($options) && $options) {


Note for myself: Need to check this in detail again.

Why not $options === true?

Would work as well.

ruflin · 2017-02-06T12:26:55Z

lib/Elastica/Query/GeohashCell.php

 /**
 * Class GeohashCell.
 *
- * @link https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-geohash-cell-query.html
+ * @link https://www.elastic.co/guide/en/elasticsearch/reference/2.4/query-dsl-geohash-cell-query.html


This was removed in 5.0? If yes, we can also remove it here as it not work with 5.0 anyways, correct? But we should mention it in the changelog.

I ran the query against 5.0 and it didn't give an error. So I assume it was only removed by accident from the documentation and is only deprecated, just as geo_distance_range.

geo_distance_range and geohash_cell are deprecated in ES 5.0 and only work with indexes created in 2.x

The previous bad query now throws a syntax error from ES due to an invalid range option which makes the whole multi search fail instead of only a single search within the multi search. So I replaced it with a different kind of error that only fails for this single search

see elastic/elasticsearch#21312

…been). So this feature has always been useless but ES 5 now validates the wrong param. Creating aliases has the possibility to specify routing and a filter. But the alastica api doesn't provide those params yet, which could be added in the future

ScriptId is used to reference stored scripts ("id" property has been renamed to "stored" in ES 5.1 which we need to do as well in later releases)

and deprecate null transport for transition to NullTransport just as Bool is deprecated in favor of BoolQuery

Tobion · 2017-02-06T20:53:40Z

I added a detailed changelog and cleaned up some duplicated tests that have been forgotten to be removed when the filters got removed.

I would really prefer not to split the PR because it would create alot of overhead and conflicts like in the changelog if I have to add each entry separately.
Also we'd want to use this complete branch for our project until it is merged.

If you have questions let me know. We can also have a skype call if you want to chat about something.

ruflin · 2017-02-07T15:58:47Z

Ok, let me try to review this PR in its entirety 👍

ruflin

@Tobion That is a great peace of work you create here. Thanks a lot for putting all the work into this. I left a minor comment.

When I merge this I plan to squash the commits into one and very soon afterwards to release 5.1.

ruflin · 2017-02-08T12:41:19Z

lib/Elastica/Script/AbstractScript.php

     */
-    public function __construct(array $params = null, $id = null)


This seems to be a BC break, right?

Only for the AbstractScript which also contains the language as well. But I don't think anyone implemented an own subclass. The existing subclasses like ScriptFile still have the same constructor. So I don't think anyone is affected by this.

I also assume that everyone that used it hopefully contributed it back to Elastica. For completeness I would still like to mention it under breaking changes. That doesn't mean I will not create 5.1 out of it ;-)

ruflin · 2017-02-08T12:44:38Z

test/lib/Elastica/Test/Base.php

@@ -128,11 +128,6 @@ protected function _createIndex($name = null, $delete = true, $shards = 1)
        return $index;
    }

-    protected function _markSkipped50($message)


ruflin · 2017-02-08T12:47:11Z

test/lib/Elastica/Test/Query/GeoDistanceRangeTest.php


-class GeoDistanceRangeTest extends BaseTest
+class GeoDistanceRangeTest extends DeprecatedClassBase


I like that idea.

Tobion · 2017-02-08T14:55:19Z

@ruflin Do you not try to reflect the versioning of elastica with elasticsearch compatibility? So then the new version would be 5.0.1 and not 5.1 (on the other hand you might not want to add deprecations in a patch version release). I'd like to create another PR afterwards with some tiny changes to upgrade the dependency to elasticsearch 5.1

ruflin · 2017-02-08T15:00:27Z

Yes I try to reflect compatibility. Is there anything inside which breaks with 5.1? All seems to also work with 5.2: #1245 If we do 5.1 I agree we should have elasticsearch first set to 5.1.

Tobion · 2017-02-08T17:16:24Z

There does not seem to be anything that breaks with 5.1. But the field to reference scripts by id has been renamed from id to stored. Both versions work but stored seems to be the documented way now, see elastic/elasticsearch#21179

So the only thing I'd like to do for 5.1 is to change this line: https://github.com/ruflin/Elastica/pull/1242/files#diff-154aad1be01fbe69e4f5790bcf273893R57

ruflin · 2017-02-09T12:08:24Z

@Tobion Merged 🎉 THANKS

Tobion changed the title ~~WIP: Remove outdated features that do not exist anymore in elasticsearch 5~~ Finalize elasticsearch 5 migration Feb 3, 2017

ruflin reviewed Feb 6, 2017

View reviewed changes

Tobion added 14 commits February 6, 2017 21:02

Remove outdated features that do not exist anymore in elasticsearch 5

e543b21

geo_distance_range and geohash_cell are deprecated in ES 5.0 and only work with indexes created in 2.x

deprecate Match::setFieldType

dfa62d6

fix bool query test and dsl tests

bd47d8e

fix match and indices tests

7619b58

fix dynamic template test

b4bcd0d

fix aggregation tests

897f3f2

fix InnerHit tests

0291864

see elastic/elasticsearch#21312

fix MoreLikeThis tests and remove leftover filter logic

f1f8f09

fix geo distance tests and inner hits cannot be fixed due to ES bug

35fe238

fix Bulk tests

48e0b6f

fixing some script logic and introducing ScriptId

4671d0b

ScriptId is used to reference stored scripts ("id" property has been renamed to "stored" in ES 5.1 which we need to do as well in later releases)

cleanup duplicated tests for old deprecated ES test classes

01f0164

and deprecate null transport for transition to NullTransport just as Bool is deprecated in favor of BoolQuery

Tobion mentioned this pull request Feb 8, 2017

minimum_should_match query #1034

Closed

ruflin approved these changes Feb 8, 2017

View reviewed changes

Tobion mentioned this pull request Feb 8, 2017

Fix relation property in geo_shape query #1246

Merged

1 task

add changelog entries

8d65580

ruflin merged commit 4666078 into ruflin:master Feb 9, 2017

Tobion deleted the elasticsearch-5.0 branch February 9, 2017 14:10

Tobion mentioned this pull request Feb 17, 2017

add Script Id feature #920

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finalize elasticsearch 5 migration #1242

Finalize elasticsearch 5 migration #1242

Tobion commented Jan 31, 2017 •

edited

Loading

ruflin commented Feb 1, 2017

Tobion commented Feb 3, 2017 •

edited

Loading

ruflin commented Feb 6, 2017

ruflin left a comment

ruflin Feb 6, 2017

Tobion Feb 6, 2017

ruflin Feb 7, 2017

ruflin Feb 6, 2017

mfeltscher Feb 7, 2017

Tobion Feb 7, 2017

ruflin Feb 6, 2017

Tobion Feb 6, 2017

Tobion commented Feb 6, 2017 •

edited

Loading

ruflin commented Feb 7, 2017

ruflin left a comment

ruflin Feb 8, 2017

Tobion Feb 8, 2017

ruflin Feb 8, 2017

Tobion Feb 9, 2017

ruflin Feb 8, 2017

ruflin Feb 8, 2017

Tobion commented Feb 8, 2017 •

edited

Loading

ruflin commented Feb 8, 2017

Tobion commented Feb 8, 2017 •

edited

Loading

ruflin commented Feb 9, 2017

		*/
		public function __construct(array $params = null, $id = null)


		class GeoDistanceRangeTest extends BaseTest
		class GeoDistanceRangeTest extends DeprecatedClassBase

Finalize elasticsearch 5 migration #1242

Finalize elasticsearch 5 migration #1242

Conversation

Tobion commented Jan 31, 2017 • edited Loading

ruflin commented Feb 1, 2017

Tobion commented Feb 3, 2017 • edited Loading

ruflin commented Feb 6, 2017

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Feb 6, 2017 • edited Loading

ruflin commented Feb 7, 2017

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Feb 8, 2017 • edited Loading

ruflin commented Feb 8, 2017

Tobion commented Feb 8, 2017 • edited Loading

ruflin commented Feb 9, 2017

Tobion commented Jan 31, 2017 •

edited

Loading

Tobion commented Feb 3, 2017 •

edited

Loading

Tobion commented Feb 6, 2017 •

edited

Loading

Tobion commented Feb 8, 2017 •

edited

Loading

Tobion commented Feb 8, 2017 •

edited

Loading