Hi Tricia,
I don't suggest removing whitespace by hand--the best way to avoid the
situation is to make sure that your editor does not automatically edit
the whitespace of a file (some good-intentioned editors do this).
The maintenance concern with interfaces is that if we want to add a
method to the highlighter contract, doing so will break all custom
implementors of the interface. If it is an abstract class that must
be subclassed, we can add a default implementation to the new method
without breaking everyone's code.
As for the contract, I was going to review the proposed patch to see
what the interface consists of, but I think that all that is required
is:
.initialize(Config)
.isHighlightEnabled(SolrParams)
.doHighlighting(...)
.getHighlightFields(...)
(which is probably what you had in your patch already---JIRA seems
down at the moment so I can't check).
cheers,
-Mike
On 5-Mar-08, at 2:49 PM, Tricia Williams wrote:
Thanks to Grant and Mike for the feedback! It is much appreciated.
Is there a quick and easy way to check for unnecessary whitespace
changes? It isn't that hard for me to go through the patch by hand
to find and remove them, but if there is an easier way I'm happy to
hear it.
I had taken the suggestion that Eli gave somewhat literally and made
SolrHighlighter an interface like RequestHandler. In the SolrCore
there are three existing objects that are configured:
SolrEventListener, SolrRequestHandler, and UpdateHandler. The first
two are interfaces and the third is an abstract class. While I'm
sure the maintenance concern is legitimate, I'm not sure what the
maintenance concern is - could someone elaborate?
I'll take your advice and make an AbstractSolrHighlighter that
SolrHighlighter (and my custom highlighter) extends. I noticed that
some of the other configurable objects implement SolrInfoMBean. Is
this something that the SolrHighlighter/AbstractSolrHighlighter
should also do?
Thanks,
Tricia
Mike Klaas (JIRA) wrote:
[ https://issues.apache.org/jira/browse/SOLR-386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12574337
#action_12574337 ]
Mike Klaas commented on SOLR-386:
---------------------------------
Hi Tricia,
I'm not sure that I would ever use SolrHighlighter overriding (if I
had the need, I probably would just make a separate search
component). However, several people want this functionality and it
has relatively low implementation/maintenance cost.
There are a few things that need to be done to get the patch
committed. First, the unnecessary whitespace changes in SolrCore
shouldn't be in the diff (it makes it really hard to see the
changes, and might make it hard to apply/revert). Also, I'm
skeptical of using interfaces for this type of thing, for
maintenance reasons. Perhaps we could move to one of the
approaches that Grant suggested?
Thanks again for the contribution, and sorry it took so long
Add confuguration to specify SolrHighlighter implementation
-----------------------------------------------------------
Key: SOLR-386
URL: https://issues.apache.org/jira/browse/SOLR-386
Project: Solr
Issue Type: Improvement
Components: highlighter
Affects Versions: 1.3
Reporter: Eli Levine
Assignee: Mike Klaas
Attachments: SOLR-386-SolrHighlighter.patch, SOLR-386-
SolrHighlighter.patch, SOLR-386-SolrHighlighter.patch, SOLR-386-
SolrHighlighter.patch
It would be great if SolrCore allowed the highlighter class to be
configurable. A good way would be to add a +class+ attribute to
the <highlighting> element in solrconfig.xml, similar to how the
RequestHandler instance is initialized in SorCore.
