Replace delimited_payload_filter by delimited_payload by liketic · Pull Request #26625 · elastic/elasticsearch

liketic · 2017-09-13T11:21:39Z

Closes #21978 . Any comments are appreciated. 😄

elasticmachine · 2017-09-13T11:21:41Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

cbuescher · 2017-10-25T17:04:22Z

I believe the issue this is trying to fix is #21978.
@liketic The PR seems a bit out of date, would you mind rebasing or merging master to this branch so it can be tested?

liketic · 2017-10-26T03:10:56Z

@cbuescher Thanks for your attention! Yes, my mistake. I rebased the pr, please review and test. Thanks a lot! 👍

cbuescher · 2017-10-26T14:12:05Z

@elasticmachine test this please

cbuescher · 2017-10-30T09:51:52Z

...es/analysis-common/src/main/java/org/elasticsearch/analysis/common/CommonAnalysisPlugin.java

                input -> new CommonGramsFilter(input, CharArraySet.EMPTY_SET)));
        filters.add(PreConfiguredTokenFilter.singleton("czech_stem", false, CzechStemFilter::new));
        filters.add(PreConfiguredTokenFilter.singleton("decimal_digit", true, DecimalDigitFilter::new));
+        // TODO deprecate delimited_payload_filter


I think the deprecation also needs to happen in this PR. Using the old name, either in the settings when creating an index or when using it in an already existing index should trigger a deprecation warning and an entry in the logs.

I pushed 7e7c54d

liketic · 2017-10-31T15:35:32Z

@cbuescher Thanks for your help! Could you guide me where to add that log? Thanks in advance!

cbuescher · 2017-11-01T14:02:14Z

@liketic I have to admit I'm not entirely sure where the best place would be to add that kind deprecation logging. I would need to do some digging as well, but one idea to look at might be the AnalysisRegistry itself, where I think all calls to obtain an instance of a named token filter need to go though "getTokenFilterProvider()". Not sure if this is the most elegant place to put the deprecation though.

liketic · 2017-11-02T12:49:02Z

@cbuescher Thanks! I'm trying to add deprecation log in AnalysisRegistry, please review again.

jpountz · 2017-11-03T09:44:12Z

I think a better way to do this would be to register delimited_payload_filter with a wrapper of the analysis factory of delimited_payload that emits a deprecation log when the version is >= 7.0. This way old indices that use delimited_payload_filter would not pollute the deprecation log since they would still be using the deprecated filter (which they cannot change).

liketic · 2017-11-21T09:30:38Z

@jpountz I tried to add a wrapper class. I'm not sure it's correct. Could you please review it?

jpountz

I left a comment about naming but things look good to me otherwise.

jpountz · 2017-11-22T07:27:10Z

...c/main/java/org/elasticsearch/analysis/common/DelimitedPayloadTokenFilterFactoryWrapper.java

+import org.elasticsearch.env.Environment;
+import org.elasticsearch.index.IndexSettings;
+
+public class DelimitedPayloadTokenFilterFactoryWrapper extends DelimitedPayloadTokenFilterFactory {


Maybe rename it to something like LegacyDelimitedPayloadTokenFilterFactory but other than that, this is indeed the approach I was thinking of.

liketic · 2017-11-22T08:17:44Z

Hi @cbuescher Could you please review again? Thanks in advance.

cbuescher · 2017-11-22T12:09:20Z

docs/reference/migration/migrate_7_0/analysis.asciidoc

+
+==== The `delimited_payload_filter` is deprecated
+
+The `delimited_payload_filter` is deprecated and should be replaced by `delimited_payload`.


Maybe we should make it clearer that the filter is just renamed, and its only the old name that is deprecated, something like "The delimited_payload_filter is renamed to delimited_payload, the old name is deprecated and will be removed at some point, so it should be replaces by delimited_payload" or slightly different.

cbuescher

@liketic thanks, this looks also good to me. I left a tiny comment regarding the migration note but other than that I agree this is ready. I will kick off the remaining CI tests once this small change has been adressed and will continue to merge after CI is green.

liketic · 2017-11-22T12:39:26Z

@cbuescher Thanks for your help. I pushed d3e6cfb . 👍

cbuescher · 2017-11-22T12:52:30Z

@liketic thanks a lot for the last change
@elasticmachine test this please

cbuescher · 2017-11-22T15:30:36Z

@liketic seems like there are still some checkstyle conventions that CI is complaining about now, sorry about that:

LegacyDelimitedPayloadTokenFilterFactory.java:30: Line is longer than 140 characters

btw. you can run gradle precommit (this skips all the long-running tests) either in the root folder or the affected module to check for these kind of errors locally, but I also keep forgetting that. Can you change that line and then run these checks locally once to make sure there are no other style errors?

liketic · 2017-11-23T14:39:08Z

@cbuescher My fault. Please test again. Thanks!

cbuescher · 2017-11-23T14:42:49Z

@elasticmachine test this please

cbuescher · 2017-11-23T16:08:21Z

@liketic thanks, the checkstyle etc... passes now, the current errors seem unrelated to this PR but I'd still like to try to get a clean run. Would you be able to merge in master once more so we get a current state there?

…payload_filter

cbuescher · 2017-11-24T10:39:19Z

@elasticmachine test this please

cbuescher · 2017-11-24T12:05:18Z

@liketic sorry this took a while longer, but thanks for this PR. I just merged it to master.
@jpountz maybe we can backport this to one of the coming 6.x versions? If we deprecate in, say 6.2., like in this PR, would you be okay to remove in 7.0 or should we leave this as is, wdyt?

* es/master: (38 commits) Backport wait_for_initialiazing_shards to cluster health API Carry over version map size to prevent excessive resizing (#27516) Fix scroll query with a sort that is a prefix of the index sort (#27498) Delete shard store files before restoring a snapshot (#27476) Replace `delimited_payload_filter` by `delimited_payload` (#26625) CURRENT should not be a -SNAPSHOT version if build.snapshot is false (#27512) Fix merging of _meta field (#27352) Remove unused method (#27508) unmuted test, this has been fixed by #27397 Consolidate version numbering semantics (#27397) Add wait_for_no_initializing_shards to cluster health API (#27489) [TEST] use routing partition size based on the max routing shards of the second split Adjust CombinedDeletionPolicy for multiple commits (#27456) Update composite-aggregation.asciidoc Deprecate `levenstein` in favor of `levenshtein` (#27409) Automatically prepare indices for splitting (#27451) Validate `op_type` for `_create` (#27483) Minor ShapeBuilder cleanup muted test Decouple nio constructs from the tcp transport (#27484) ...

lcawl · 2017-11-24T17:15:13Z

I added the following in elastic/elasticsearch/migration/migrate_7_0.asciidoc to address documentation build errors (commit af971b3 ):

include::migrate_7_0/analysis.asciidoc[]

cbuescher · 2017-11-24T20:37:30Z

@lcawl thanks for fixing this and sorry I didn't catch this in the review.

lcawl · 2017-11-24T21:13:33Z

No problem, it was a quick fix!

clintongormley · 2017-11-27T09:57:44Z

@jpountz maybe we can backport this to one of the coming 6.x versions? If we deprecate in, say 6.2., like in this PR, would you be okay to remove in 7.0 or should we leave this as is, wdyt?

I think we should deprecate in 6.x and remove in 7.0, but still allow the old form in 7.0 for indices created in 6.x.

Also, we should add a check for this to the x-pack migration assistant.

) The `delimited_payload_filter` is renamed to `delimited_payload`, the old name is deprecated and should be replaced by `delimited_payload`. Closes elastic#21978

The `delimited_payload_filter` is renamed to `delimited_payload`, the old name is deprecated and should be replaced by `delimited_payload`. Closes #21978

cbuescher · 2017-12-07T14:47:44Z

@clintongormley I opened #27704 for the final removal in 7.0 and adding checks to the migration assistant.

From 7.0 on, using `delimited_payload_filter` should throw an error. It was deprecated in 6.2 in favour of `delimited_payload` (#26625). Relates to #27704

…r is used (#43684) #26625 deprecated delimited_payload_filter and added tests to check that warnings would be emitted when both a normal and pre-configured filter were used. Unfortunately, due to a bug in the Analyze API, the pre- configured filter check was never actually triggered, and it turns out that the deprecation warning was not in fact being emitted in this case. #43568 fixed the Analyze API bug, which then surfaced this on backport. This commit ensures that the preconfigured filter also emits the warnings and triggers an error if a new index tries to use a preconfigured delimited_payload_filter

Replace delimited_payload_filter by delimited_payload

7704d34

liketic force-pushed the rebane-delimited_payload_filter branch from 6ddd158 to 7704d34 Compare October 26, 2017 03:08

cbuescher requested changes Oct 30, 2017

View reviewed changes

cbuescher self-assigned this Nov 1, 2017

Add deprecation log for delimited_payload_filter

7e7c54d

cbuescher added :Search Foundations/Mapping Index mappings, including merging and defining field types review :Search Relevance/Analysis How text is split into tokens and removed :Search Foundations/Mapping Index mappings, including merging and defining field types labels Nov 16, 2017

Add a wrapper of token filter factory for delimited_payload_filter

706955c

jpountz approved these changes Nov 22, 2017

View reviewed changes

liketic added 2 commits November 22, 2017 16:10

Fix naming of legacy delimited payload filter factory class

194d56c

Merge master

45f6aed

liketic force-pushed the rebane-delimited_payload_filter branch from ced2441 to 45f6aed Compare November 22, 2017 08:16

cbuescher reviewed Nov 22, 2017

View reviewed changes

cbuescher approved these changes Nov 22, 2017

View reviewed changes

Update document wording

d3e6cfb

liketic force-pushed the rebane-delimited_payload_filter branch from f8e3e3f to d3e6cfb Compare November 22, 2017 12:38

Fix code style

f1d33f2

Merge remote-tracking branch 'upstream/master' into rebane-delimited_…

9df8254

…payload_filter

cbuescher added v7.0.0 >deprecation labels Nov 24, 2017

cbuescher merged commit 4885acb into elastic:master Nov 24, 2017

liketic deleted the rebane-delimited_payload_filter branch November 24, 2017 14:59

cbuescher mentioned this pull request Nov 27, 2017

Backport delimited payload filter renaming #27535

Closed

cbuescher mentioned this pull request Dec 7, 2017

Remove delimited_payload_filter and add to migration assistant #27704

Closed

cbuescher mentioned this pull request Dec 7, 2017

Remove delimited_payload_filter #27705

Merged

cbuescher pushed a commit that referenced this pull request Apr 5, 2018

Remove delimited_payload_filter (#27705)

231fd4e

From 7.0 on, using `delimited_payload_filter` should throw an error. It was deprecated in 6.2 in favour of `delimited_payload` (#26625). Relates to #27704

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

romseygeek mentioned this pull request Jun 27, 2019

Issue deprecation warnings for preconfigured delimited_payload_filter #43684

Merged

Mpdreamz mentioned this pull request Aug 7, 2019

[meta] 7.3 Release elastic/elasticsearch-net#4001

Closed

16 tasks


		==== The `delimited_payload_filter` is deprecated

		The `delimited_payload_filter` is deprecated and should be replaced by `delimited_payload`.

Conversation

liketic commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 13, 2017

Uh oh!

cbuescher commented Oct 25, 2017

Uh oh!

liketic commented Oct 26, 2017

Uh oh!

cbuescher commented Oct 26, 2017

Uh oh!

cbuescher Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

liketic Nov 2, 2017

Choose a reason for hiding this comment

Uh oh!

liketic commented Oct 31, 2017

Uh oh!

cbuescher commented Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liketic commented Nov 2, 2017

Uh oh!

jpountz commented Nov 3, 2017

Uh oh!

liketic commented Nov 21, 2017

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

jpountz Nov 22, 2017

Choose a reason for hiding this comment

Uh oh!

liketic Nov 22, 2017

Choose a reason for hiding this comment

Uh oh!

liketic commented Nov 22, 2017

Uh oh!

cbuescher Nov 22, 2017

Choose a reason for hiding this comment

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

liketic commented Nov 22, 2017

Uh oh!

cbuescher commented Nov 22, 2017

Uh oh!

cbuescher commented Nov 22, 2017

Uh oh!

liketic commented Nov 23, 2017

Uh oh!

cbuescher commented Nov 23, 2017

Uh oh!

cbuescher commented Nov 23, 2017

Uh oh!

cbuescher commented Nov 24, 2017

Uh oh!

cbuescher commented Nov 24, 2017

Uh oh!

lcawl commented Nov 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbuescher commented Nov 24, 2017

Uh oh!

lcawl commented Nov 24, 2017

Uh oh!

clintongormley commented Nov 27, 2017

Uh oh!

cbuescher commented Dec 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

liketic commented Sep 13, 2017 •

edited

Loading

cbuescher commented Nov 1, 2017 •

edited

Loading

lcawl commented Nov 24, 2017 •

edited

Loading