search

mikemccand / stargazers-migration-test

Testing Lucene's Jira -> GitHub issues migration
0 stars 0 forks source link

Provide backward compatibility for loading analysis factories [LUCENE-8907] #904

Closed mikemccand closed 5 years ago

mikemccand commented 5 years ago

The changes in LUCENE-8778 have breaking changes in the analysis factory interface and  custom factories implemented by users / 3rd parties will be affected. We need to keep some backwards compatibility during 8.x.

Please see the discussion in SOLR-13593 for details.

Legacy Jira details

LUCENE-8907 by Tomoko Uchida (@mocobeta) on Jul 10 2019, resolved Jul 11 2019 Linked issues:

mikemccand commented 5 years ago

There may be two options:

  1. Completely emulate the factory loading method of Lucene 8.1. (i.e., copy the old logic and use it if the "NAME" field does not exist in a factory.)
  2. Ignore factories which have no proper "NAME" field. (We have to note the behaviour somewhere.)

Either way, some warning messages would be helpful to let users know that they have to change their code by Lucene 9.0 (can we show such "warnings"?).

[Legacy Jira: Tomoko Uchida (@mocobeta) on Jul 10 2019]

mikemccand commented 5 years ago

I don't like 2, it feels too breaking to me for a minor release. I like 1 better but then I think we should also fail if an analysis factory has a NAME constant that is not the same as the name that would be derived from the class name?

Another option could be to revert from 8.x. In any case we should add migration instructions to lucene/MIGRATE.txt on master.

some warning messages would be helpful

We never log from Lucene since this is a library.

[Legacy Jira: Adrien Grand (@jpountz) on Jul 10 2019]

mikemccand commented 5 years ago

Hi, reverting is a feasible option because there have been no changes - as far as I know - that depends on LUCENE-8778 other than my commits. I am sure that I can safely revert all my related commits so far on branch_8x (though I need to confirm that). Personally I would prefer reverting instead of blocking release 8.2, which seems to happen soon. Maybe we can rethink the way to safely include the changes into 8.x again a little later. Are there any other suggestions/opinions?

[Legacy Jira: Tomoko Uchida (@mocobeta) on Jul 10 2019]

mikemccand commented 5 years ago

I have a slight preference for reverting on 8.x and have this change only in 9.0. My worry is that the backward compatibility layer would either need to introduce leniency (factories without a NAME) or stronger checks (Same NAME and class name), and it could end up causing as much trouble as the original change.

[~thetaphi‍] What do you think?

[Legacy Jira: Adrien Grand (@jpountz) on Jul 10 2019]

mikemccand commented 5 years ago

I agree partly with Adrien. But we can still allow to use old factories in 8.2, in a lenient way: If the factory has no NAME field, it generates one using old algorithm - and ideally would log a warning. The biggest problem ist that we can't log warnings, which is not good. Solr could do this.

Sorry for coming up with this issue so late, but I did not notice that the whole change was committed to 8.x. I have here a customer project with many custom analyzers, so it came to my mind.

[Legacy Jira: Uwe Schindler (@uschindler) on Jul 10 2019]

mikemccand commented 5 years ago

Reverting sounds to cause least friction, not delaying 8.2. Unless a patch is presented within say 24h, which is very simple and straight forward.?

[Legacy Jira: Jan Høydahl (@janhoy) on Jul 10 2019]

mikemccand commented 5 years ago

Hi, I picked the commits which should be reverted from commit log mail. There are five commits: all is mine and there is no conflicts other than lucene/CHANGES.txt on the reverted 8x branch. All tests is OK and that passed precommit.

git revert 2804d00137957dc27dce922103c6286bbc7a9eca
git revert 7e05bd717309088d3b35cba6be0f70fd14b3c8d0
git revert d1678a3a68685acd1432dfadcd5f49fa817273ad
git revert 12e3451fb80e151b16f51493761b9dfc580f8fa0
git revert 0cc1753e76eb61000c1d3513448719e5f7923e48

I think we can delay the discussion about backporting LUCENE-8778 into 8.x with transparent backwards compatibility. (From my perspective, the backporting to the 8x branch was my fault and we do not need to change the SPI loader until release 9.0.)

[Legacy Jira: Tomoko Uchida (@mocobeta) on Jul 10 2019]

mikemccand commented 5 years ago

I don't want to stop 8.2 release for this issue. If there is no strong objection within 24 hours, let me revert the changes (mentioned in my previous comment) for now and open another issue to discuss about backporting LUCENE-8778 to 8.x. It would be easy to cherry-pick those changes to the 8x branch again.

[Legacy Jira: Tomoko Uchida (@mocobeta) on Jul 11 2019]

mikemccand commented 5 years ago

Commit 59c7eb92cfcc766ee6866fe14ec609b09d41fcf6 in lucene-solr's branch refs/heads/branch_8x from Tomoko Uchida https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=59c7eb9

LUCENE-8907: Revert LUCENE-8778 and succeeding commits.

[Legacy Jira: ASF subversion and git services on Jul 11 2019]

mikemccand commented 5 years ago

Commit 824a196d780a5d5694a579969c2aaf45554555c8 in lucene-solr's branch refs/heads/branch_8_2 from Tomoko Uchida https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=824a196d

LUCENE-8907: Revert LUCENE-8778 and succeeding commits.

[Legacy Jira: ASF subversion and git services on Jul 11 2019]

mikemccand commented 5 years ago

Commit 6d79cc9e443b957baf72abcc7757c5edfb7ff1c1 in lucene-solr's branch refs/heads/master from Tomoko Uchida https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=6d79cc9

LUCENE-8907: Move change logs for LUCENE-8778 and following issues to the 9.0.0 updates section.

[Legacy Jira: ASF subversion and git services on Jul 11 2019]

mikemccand commented 5 years ago

Commits for LUCENE-8778 and succeeding issues were reverted on branch_8x and branch_8_2.

[Legacy Jira: Tomoko Uchida (@mocobeta) on Jul 11 2019]

mikemccand commented 5 years ago

I opened to carry on the discussion here: LUCENE-8911.

[Legacy Jira: Tomoko Uchida (@mocobeta) on Jul 11 2019]

mikemccand commented 5 years ago

Commit 6d79cc9e443b957baf72abcc7757c5edfb7ff1c1 in lucene-solr's branch refs/heads/jira/SOLR-13565 from Tomoko Uchida https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=6d79cc9

LUCENE-8907: Move change logs for LUCENE-8778 and following issues to the 9.0.0 updates section.

[Legacy Jira: ASF subversion and git services on Jul 12 2019]

mikemccand commented 5 years ago

Closing after the 8.2.0 release

[Legacy Jira: Ignacio Vera (@iverase) on Jul 26 2019]