Lucene - Core
  1. Lucene - Core
  2. LUCENE-4303

Analysis factories should use ResourceLoader, not Class.forName

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0
    • Component/s: modules/analysis
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      This affects SnowballPorterFilterFactory and PhoneticFilterFactory.

      In Solr I encountered this problem when I specified an encoder and I was forced to put the library in WEB-INF/lib instead of /solr/lib/.

        Activity

        Hide
        Uwe Schindler added a comment -

        This is easy to fix, I will take care!

        Show
        Uwe Schindler added a comment - This is easy to fix, I will take care!
        Hide
        David Smiley added a comment - - edited

        I'm already doing it Uwe! That's why I assigned it to myself to signal it's being worked on. I'm almost done but there is an issue of initialization order that I need to fix since I assumed inform() is called first.

        Show
        David Smiley added a comment - - edited I'm already doing it Uwe! That's why I assigned it to myself to signal it's being worked on. I'm almost done but there is an issue of initialization order that I need to fix since I assumed inform() is called first.
        Hide
        Uwe Schindler added a comment -

        OK, thanks. I just wanted to review the changes to be made, as I am currently working with Chris Male on LUCENE-4256, handling the complex init of analysis factories. The current impl is fine for Solr, but not for a universally useable factory environment, so needs changes. So I can review your patches before committing

        Show
        Uwe Schindler added a comment - OK, thanks. I just wanted to review the changes to be made, as I am currently working with Chris Male on LUCENE-4256 , handling the complex init of analysis factories. The current impl is fine for Solr, but not for a universally useable factory environment, so needs changes. So I can review your patches before committing
        Hide
        David Smiley added a comment -

        Attached is a patch that works. Basically, the two affected factories were modified to support ResourceLoaderAware. Their init() methods were transferred to inform() instead.

        If you think it's good Uwe, I'll commit it. Perhaps a CHANGES.txt is needed?

        Show
        David Smiley added a comment - Attached is a patch that works. Basically, the two affected factories were modified to support ResourceLoaderAware. Their init() methods were transferred to inform() instead. If you think it's good Uwe, I'll commit it. Perhaps a CHANGES.txt is needed?
        Hide
        Uwe Schindler added a comment -

        Patch looks fine, I dont like the newInstance().getClass(), but as quick fix this is fine. In my opinion, the getEncoder() should request the encoder from ReosurceLoader. But its also fine how it is implemented at the moment.

        Some minor things: the inject field should have no access modifier, as test is in same package, so package-private is fine. Protected means access from subclasses, which is not the case.

        The tests are also fine, ClassPathResourceLoader is fine here!

        Show
        Uwe Schindler added a comment - Patch looks fine, I dont like the newInstance().getClass(), but as quick fix this is fine. In my opinion, the getEncoder() should request the encoder from ReosurceLoader. But its also fine how it is implemented at the moment. Some minor things: the inject field should have no access modifier, as test is in same package, so package-private is fine. Protected means access from subclasses, which is not the case. The tests are also fine, ClassPathResourceLoader is fine here!
        Hide
        David Smiley added a comment -

        I'll change the modifier for inject; I should know better. RE newInstance().getClass() – yeah not great I know but it's less change and it's possible the resourceLoader's resolution might involve more overhead which would get triggered on every create(TokenStream) call so I went for this approach.

        I'll commit soonish.

        Show
        David Smiley added a comment - I'll change the modifier for inject; I should know better. RE newInstance().getClass() – yeah not great I know but it's less change and it's possible the resourceLoader's resolution might involve more overhead which would get triggered on every create(TokenStream) call so I went for this approach. I'll commit soonish.
        Hide
        Uwe Schindler added a comment -

        +1

        Show
        Uwe Schindler added a comment - +1
        Hide
        Uwe Schindler added a comment -

        Will you backport?

        Show
        Uwe Schindler added a comment - Will you backport?
        Hide
        David Smiley added a comment -

        Committed to 4x r1372648 & trunk r1372631. Resolving issue.

        Show
        David Smiley added a comment - Committed to 4x r1372648 & trunk r1372631. Resolving issue.
        Hide
        Uwe Schindler added a comment -

        Closed after release.

        Show
        Uwe Schindler added a comment - Closed after release.

          People

          • Assignee:
            David Smiley
            Reporter:
            David Smiley
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development