Details

    • Type: Improvement Improvement
    • Status: Reopened
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 4.10, Trunk
    • Component/s: spellchecker
    • Labels:
      None

      Description

      Right now Solr defaults to Jaspell, but we've deprecated it in LUCENE-5775 ... and in trunk I'd like to remove it. But first we need to fix Solr to not default to it.

      1. SOLR-6178.patch
        0.7 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Hoss Man added a comment -

          removed deprecation and CHANGES.txt entry.

          updating linkage to make it clear we can't deprecate until we come up with a better default (SOLR-6185)

          Show
          Hoss Man added a comment - removed deprecation and CHANGES.txt entry. updating linkage to make it clear we can't deprecate until we come up with a better default ( SOLR-6185 )
          Hide
          ASF subversion and git services added a comment -

          Commit 1619173 from hossman@apache.org in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1619173 ]

          SOLR-6178: backout deprecation until we have a diff default (merge r1619172)

          Show
          ASF subversion and git services added a comment - Commit 1619173 from hossman@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1619173 ] SOLR-6178 : backout deprecation until we have a diff default (merge r1619172)
          Hide
          ASF subversion and git services added a comment -

          Commit 1619172 from hossman@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1619172 ]

          SOLR-6178: backout deprecation until we have a diff default

          Show
          ASF subversion and git services added a comment - Commit 1619172 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1619172 ] SOLR-6178 : backout deprecation until we have a diff default
          Hide
          Uwe Schindler added a comment -

          I don't care. Maybe we should deprecate and change the default in Lucene 5. Changing defaults is a bad idea in stable branches, especially if they require stuff like reindexing.

          So maybe revert?

          Show
          Uwe Schindler added a comment - I don't care. Maybe we should deprecate and change the default in Lucene 5. Changing defaults is a bad idea in stable branches, especially if they require stuff like reindexing. So maybe revert?
          Hide
          Hoss Man added a comment -

          Uwe Schindler: can you please clarify the state of things with this issue?

          at a quick glance...

          • issues is currently unresolved, fixVersion=4.10
          • issue is currently listed in CHANGES.txt as part of 4.10
          • JaspellLookupFactory on branch 4x is currently marked deprecated
          • JaspellLookupFactory is still the default in Suggester.java

          ..should we rip out the deprecation & CHANGES.txt entry prior to 4.10 since this still appears to be an outstanding issue?

          Show
          Hoss Man added a comment - Uwe Schindler : can you please clarify the state of things with this issue? at a quick glance... issues is currently unresolved, fixVersion=4.10 issue is currently listed in CHANGES.txt as part of 4.10 JaspellLookupFactory on branch 4x is currently marked deprecated JaspellLookupFactory is still the default in Suggester.java ..should we rip out the deprecation & CHANGES.txt entry prior to 4.10 since this still appears to be an outstanding issue?
          Hide
          Uwe Schindler added a comment -

          I reverted and reopened for more discussion. The Change of defaults was moved to separate issue: SOLR-6185

          Show
          Uwe Schindler added a comment - I reverted and reopened for more discussion. The Change of defaults was moved to separate issue: SOLR-6185
          Hide
          ASF subversion and git services added a comment -

          Commit 1604711 from Uwe Schindler in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1604711 ]

          Merged revision(s) 1604710 from lucene/dev/trunk:
          Move changes of:
          SOLR-6178, LUCENE-5775: Deprecate JaspellLookupFactory

          Show
          ASF subversion and git services added a comment - Commit 1604711 from Uwe Schindler in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1604711 ] Merged revision(s) 1604710 from lucene/dev/trunk: Move changes of: SOLR-6178 , LUCENE-5775 : Deprecate JaspellLookupFactory
          Hide
          ASF subversion and git services added a comment -

          Commit 1604710 from Uwe Schindler in branch 'dev/trunk'
          [ https://svn.apache.org/r1604710 ]

          Move changes of:
          SOLR-6178, LUCENE-5775: Deprecate JaspellLookupFactory

          Show
          ASF subversion and git services added a comment - Commit 1604710 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1604710 ] Move changes of: SOLR-6178 , LUCENE-5775 : Deprecate JaspellLookupFactory
          Hide
          ASF subversion and git services added a comment -

          Commit 1604707 from Uwe Schindler in branch 'dev/branches/lucene_solr_4_9'
          [ https://svn.apache.org/r1604707 ]

          Revert:
          Merged revision(s) 1604122 from lucene/dev/trunk:
          SOLR-6178, LUCENE-5775: Deprecate JaspellLookupFactory

          Show
          ASF subversion and git services added a comment - Commit 1604707 from Uwe Schindler in branch 'dev/branches/lucene_solr_4_9' [ https://svn.apache.org/r1604707 ] Revert: Merged revision(s) 1604122 from lucene/dev/trunk: SOLR-6178 , LUCENE-5775 : Deprecate JaspellLookupFactory
          Hide
          Robert Muir added a comment -

          Also, changing the default gets even more complex. its been jaspell for a long time: but changing the default could cause a ton of confusion (users think 4.9 cannot read their "index"), even though we know that suggester data is really not part of the index... the user just may not see it that way: to them its their data and the upgrade broke them.

          Such a change needs to be planned out from a docs and release notes perspective, not shoved in right at the last minute hastily. Its too risky.

          Show
          Robert Muir added a comment - Also, changing the default gets even more complex. its been jaspell for a long time: but changing the default could cause a ton of confusion (users think 4.9 cannot read their "index"), even though we know that suggester data is really not part of the index... the user just may not see it that way: to them its their data and the upgrade broke them. Such a change needs to be planned out from a docs and release notes perspective, not shoved in right at the last minute hastily. Its too risky.
          Hide
          Robert Muir added a comment -

          Well, thats your opinion.

          There is absolutely no way I am going to let a lucene deprecation annotation have a destabilizing snowball effect on this release: first its a deprecation warning, then someone doesn't want the warning in the default configuration and wants a change of defaults, then the change of defaults breaks the example, then the change to the example breaks the documentation or tutorial, and on and on and on.

          There is just no reason for such risks.

          Show
          Robert Muir added a comment - Well, thats your opinion. There is absolutely no way I am going to let a lucene deprecation annotation have a destabilizing snowball effect on this release: first its a deprecation warning, then someone doesn't want the warning in the default configuration and wants a change of defaults, then the change of defaults breaks the example, then the change to the example breaks the documentation or tutorial, and on and on and on. There is just no reason for such risks.
          Hide
          Uwe Schindler added a comment -

          In my opinion, we should revert this one and the deprecation inside Lucene. To me it is also very controversal if Solr defaults to use something that is deprecated in Lucene because it uses too much memory!

          I agree to revert this issue if the deprecation in Lucene is also reverted.

          Show
          Uwe Schindler added a comment - In my opinion, we should revert this one and the deprecation inside Lucene. To me it is also very controversal if Solr defaults to use something that is deprecated in Lucene because it uses too much memory! I agree to revert this issue if the deprecation in Lucene is also reverted.
          Hide
          Robert Muir added a comment -

          Its last minute because it can be potentially controversial. Deprecating the lucene suggester has no impact to solr users. Putting a big warning in the default configuration is a different story.

          I didnt revert anything, your commit just didn't make it in time for the release candidate. I dont want such controversy: I care about keeping things stable and bad bugs. I can EASILY see such a warning turning into "we should change the solr default" and spinning totally out of control and somehow breaking the example.

          I waited extra time for SOLR-6182 because it looked like a really bad bug. And if a similar bug like that pops up I won't even have a second thought about respinning for it. But by no means is this a blocker.

          Show
          Robert Muir added a comment - Its last minute because it can be potentially controversial. Deprecating the lucene suggester has no impact to solr users. Putting a big warning in the default configuration is a different story. I didnt revert anything, your commit just didn't make it in time for the release candidate. I dont want such controversy: I care about keeping things stable and bad bugs. I can EASILY see such a warning turning into "we should change the solr default" and spinning totally out of control and somehow breaking the example. I waited extra time for SOLR-6182 because it looked like a really bad bug. And if a similar bug like that pops up I won't even have a second thought about respinning for it. But by no means is this a blocker.
          Hide
          Michael McCandless added a comment -

          In hindsight I should have just backported the bug fix in LUCENE-5775 (stack overflow when calling .ramBytesUsed()), and not the deprecation, for 4.9 .... next time I'll make separate issues. Sorry for the hassle.

          Show
          Michael McCandless added a comment - In hindsight I should have just backported the bug fix in LUCENE-5775 (stack overflow when calling .ramBytesUsed()), and not the deprecation, for 4.9 .... next time I'll make separate issues. Sorry for the hassle.
          Hide
          Uwe Schindler added a comment -

          This was not last minute. Mike just missed to add the deprecation also in Solr. If we go this way, please also revert Mike's commit. Sorry.

          Show
          Uwe Schindler added a comment - This was not last minute. Mike just missed to add the deprecation also in Solr. If we go this way, please also revert Mike's commit. Sorry.
          Hide
          Robert Muir added a comment -

          Well, you can vote however you like. I already spun the RC and its in progress.

          Its risky as shit to add such deprecations which will cause warning messages to solr users and create a controversy. There is no need to rush in such things at the last minute.

          Show
          Robert Muir added a comment - Well, you can vote however you like. I already spun the RC and its in progress. Its risky as shit to add such deprecations which will cause warning messages to solr users and create a controversy. There is no need to rush in such things at the last minute.
          Hide
          Uwe Schindler added a comment -

          Sorry, I don't see a release.

          Show
          Uwe Schindler added a comment - Sorry, I don't see a release.
          Hide
          Robert Muir added a comment -

          This didn't make 4.9

          Show
          Robert Muir added a comment - This didn't make 4.9
          Hide
          ASF subversion and git services added a comment -

          Commit 1604125 from Uwe Schindler in branch 'dev/branches/lucene_solr_4_9'
          [ https://svn.apache.org/r1604125 ]

          Merged revision(s) 1604122 from lucene/dev/trunk:
          SOLR-6178, LUCENE-5775: Deprecate JaspellLookupFactory

          Show
          ASF subversion and git services added a comment - Commit 1604125 from Uwe Schindler in branch 'dev/branches/lucene_solr_4_9' [ https://svn.apache.org/r1604125 ] Merged revision(s) 1604122 from lucene/dev/trunk: SOLR-6178 , LUCENE-5775 : Deprecate JaspellLookupFactory
          Hide
          ASF subversion and git services added a comment -

          Commit 1604124 from Uwe Schindler in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1604124 ]

          Merged revision(s) 1604122 from lucene/dev/trunk:
          SOLR-6178, LUCENE-5775: Deprecate JaspellLookupFactory

          Show
          ASF subversion and git services added a comment - Commit 1604124 from Uwe Schindler in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1604124 ] Merged revision(s) 1604122 from lucene/dev/trunk: SOLR-6178 , LUCENE-5775 : Deprecate JaspellLookupFactory
          Hide
          ASF subversion and git services added a comment -

          Commit 1604122 from Uwe Schindler in branch 'dev/trunk'
          [ https://svn.apache.org/r1604122 ]

          SOLR-6178, LUCENE-5775: Deprecate JaspellLookupFactory

          Show
          ASF subversion and git services added a comment - Commit 1604122 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1604122 ] SOLR-6178 , LUCENE-5775 : Deprecate JaspellLookupFactory
          Hide
          Uwe Schindler added a comment - - edited

          Ok lets split this issue: We need in any case the deprecateion, not the change of defaults. I will commit that now.

          Show
          Uwe Schindler added a comment - - edited Ok lets split this issue: We need in any case the deprecateion, not the change of defaults. I will commit that now.
          Hide
          Robert Muir added a comment -

          This is WAY too risky to do right before a release. Please do not commit anything at all to the 4.9 branch. This can definitely be done in 4.10

          Show
          Robert Muir added a comment - This is WAY too risky to do right before a release. Please do not commit anything at all to the 4.9 branch. This can definitely be done in 4.10
          Hide
          Robert Muir added a comment -

          Not a blocker. Sorry guys. Not even a bug!

          Show
          Robert Muir added a comment - Not a blocker. Sorry guys. Not even a bug!
          Hide
          Uwe Schindler added a comment -

          Simple patch for deprecation.

          Maybe we should change the default to FSTLookupFactory. This one looks most similar to the Jaspell one (from the functionality). I am not sure how this affects users that rely on the default impl (because they have not given one it in the solrconfig).

          Show
          Uwe Schindler added a comment - Simple patch for deprecation. Maybe we should change the default to FSTLookupFactory. This one looks most similar to the Jaspell one (from the functionality). I am not sure how this affects users that rely on the default impl (because they have not given one it in the solrconfig).
          Hide
          Uwe Schindler added a comment -

          I think for now, we should at least make the factory deprecated. As we deprecated Jaspell already in 4.9, this should be done before 4.9.

          Show
          Uwe Schindler added a comment - I think for now, we should at least make the factory deprecated. As we deprecated Jaspell already in 4.9, this should be done before 4.9.

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Michael McCandless
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:

                Development