Solr
  1. Solr
  2. SOLR-952

duplicated code in (Default)SolrHighlighter and HighlightingUtils

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.4
    • Fix Version/s: 1.4
    • Component/s: highlighter
    • Labels:
      None

      Description

      A large quantity of code is duplicated between the deprecated HighlightingUtils class and the newer SolrHighlighter and DefaultSolrHighlighter (which have been getting bug fixes and enhancements). The Utils class is no longer used anywhere in Solr, but people writing plugins may be taking advantage of it, so it should be cleaned up.

        Activity

        Hide
        Chris Harris added a comment - - edited

        I did a brief look through the svn logs:

        • In r422248, the class was first added. It was added to org/apache/solr/util/SolrPluginUtils.java. The purpose of this commit was: "order tokens by startOffset when highlighting"
        • In r510338 it was moved moved to org/apache/solr/util/HighlightingUtils.java.
        • In r639490 org/apache/solr/highlight/DefaultSolrHighlighter.java was created and the econd copy was added there; this revision did not, however, touch org/apache/solr/util/HighlightingUtils.java
        Show
        Chris Harris added a comment - - edited I did a brief look through the svn logs: In r422248, the class was first added. It was added to org/apache/solr/util/SolrPluginUtils.java. The purpose of this commit was: "order tokens by startOffset when highlighting" In r510338 it was moved moved to org/apache/solr/util/HighlightingUtils.java. In r639490 org/apache/solr/highlight/DefaultSolrHighlighter.java was created and the econd copy was added there; this revision did not, however, touch org/apache/solr/util/HighlightingUtils.java
        Hide
        Hoss Man added a comment -

        updated summary and description to point out that TokenOrderingFilter is just a small part of the code duplication that seems to have been left behind from an incomplete refactoring of HighlightingUtils.

        Show
        Hoss Man added a comment - updated summary and description to point out that TokenOrderingFilter is just a small part of the code duplication that seems to have been left behind from an incomplete refactoring of HighlightingUtils.
        Hide
        Hoss Man added a comment -

        Patch for cleaning up HighlighterUtils.

        I'm not a subject matter expert here, but from what i can tell this hsould be back-compat for anyone still using the utils class.

        I don't plan on committing unless someone more familiar with the highlighting code looks it over and gives it a thumbs up. (HighlightingUtils doesn't currently have any tests to verify against)

        Show
        Hoss Man added a comment - Patch for cleaning up HighlighterUtils. I'm not a subject matter expert here, but from what i can tell this hsould be back-compat for anyone still using the utils class. I don't plan on committing unless someone more familiar with the highlighting code looks it over and gives it a thumbs up. (HighlightingUtils doesn't currently have any tests to verify against)
        Hide
        Mike Klaas added a comment -

        HighlightingUtils has been deprecated for at least one release; can't we just rip it out?

        Show
        Mike Klaas added a comment - HighlightingUtils has been deprecated for at least one release; can't we just rip it out?
        Hide
        Hoss Man added a comment -

        HighlightingUtils has been deprecated for at least one release; can't we just rip it out?

        it depends on how strict we want to be about backwards compatibility – which has never been extremely well defined for Solr. following hte Lucene-Java model, we should be back compatible with java class compatibility until 2.0 ... but since Solr tends to focus more on the HTTP API, i'm certainly fine with saying we aim for HTTP/REST back-compat between major versions, and have a more relaxed view on the class compatibility for plugin developers ... but if we really want to get into this is a much broader discussion.

        for the purposes of this issue does anyone see any reason not to commit the patch?

        Show
        Hoss Man added a comment - HighlightingUtils has been deprecated for at least one release; can't we just rip it out? it depends on how strict we want to be about backwards compatibility – which has never been extremely well defined for Solr. following hte Lucene-Java model, we should be back compatible with java class compatibility until 2.0 ... but since Solr tends to focus more on the HTTP API, i'm certainly fine with saying we aim for HTTP/REST back-compat between major versions, and have a more relaxed view on the class compatibility for plugin developers ... but if we really want to get into this is a much broader discussion. for the purposes of this issue does anyone see any reason not to commit the patch?
        Hide
        Shalin Shekhar Mangar added a comment -

        for the purposes of this issue does anyone see any reason not to commit the patch?

        +1 for commit. I'm fine with removing the class too.

        Show
        Shalin Shekhar Mangar added a comment - for the purposes of this issue does anyone see any reason not to commit the patch? +1 for commit. I'm fine with removing the class too.
        Hide
        Hoss Man added a comment -

        Committed revision 758795.

        Show
        Hoss Man added a comment - Committed revision 758795.
        Hide
        Grant Ingersoll added a comment -

        Bulk close for Solr 1.4

        Show
        Grant Ingersoll added a comment - Bulk close for Solr 1.4

          People

          • Assignee:
            Hoss Man
            Reporter:
            Chris Harris
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development