lucene-solr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tricia Williams <williams.tri...@gmail.com>
Subject Re: [jira] Commented: (SOLR-386) Add confuguration to specify SolrHighlighter implementation
Date Thu, 06 Mar 2008 00:48:51 GMT
OK.  So I think I fixed the whitespace problem. 

Thanks for explaining the problem with interfaces -- that makes sense 
now.  I assume that EventListener and RequestHandler are interfaces 
because they've been thought long and hard about and are not going to 
change?

My first try at the patch was just to include the public methods, which 
are the ones you list:
> .initialize(Config)
> .isHighlightEnabled(SolrParams)
> .doHighlighting(...)
> .getHighlightFields(...) 
I discovered that the unit tests call the formatters and fragmenters 
directly so in the interface version I made public get methods for 
these.  Now that we're using an abstract class I am able to just include 
these as is - so no changes to HighlighterTest this time.  But speaking 
of unit tests... Anecdotally I know that the SolrCore changes allow the 
highlighter to be configured (my custom highlighter).  I guess I should 
write a unit test that ensures this works.  I'll do that now.

After doing some thinking I decided to leave the default implementation 
of isHighlightingEnabled(SolrParams), and getHighlightFields(...) in the 
abstract class because both methods deal with reading parameters.  I 
can't think of a use case of a highlighter that wouldn't use this or at 
worst ignore/override this method.  Is this a reasonable decision?

I wasn't sure what to do with the logger, so I left it in the 
AbstractSolrHighlighter.

Thoughts?
Tricia

p.s.  I've attached the updated patch since JIRA appears to be down.

Mike Klaas wrote:
> 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.
>>>>
>>>
>>>
>>
>
>


Mime
View raw message