lucene-solr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Shalin Shekhar Mangar (JIRA)" <j...@apache.org>
Subject [jira] Commented: (SOLR-1688) Inner class FieldCacheSources should be refactored into their own classes
Date Mon, 28 Dec 2009 09:10:29 GMT

    [ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794782#action_12794782
] 

Shalin Shekhar Mangar commented on SOLR-1688:
---------------------------------------------

{quote}
IMO, most of these should remain implementation details (i.e. not public)... they weren't
thought out in sufficient detail to support as public classes (and there has been little reason
to do so).
If we need StrValueSource to be public for another issue, then we should limit the change
to that. 
{quote}

+1

As they say, lets not fix what ain't broken.

{quote}
If they are defined as a "core" data structure part of the JDK, then I would say yes. It's
not as black and white of a line as you make it out to be. You can have SOLR be entirely a
plugin-based system, with nothing but configuration inside of SVN, or you can have every piece
of code that interacts with SOLR be inside the SOLR SVN. Neither solution will work, you have
to strike a balance. The same applies for code organization and using absolutes or extremes
doesn't really illustrate much.
{quote}

Chris, we are striving for balance and we are OK with the change to StrFieldSource. In this
particular case, you seem to be pushing towards extremes in the name of consistency.

{quote}
Can you tell me the reason that e.g., StrFieldSource exists inside of StrField while DoubleFieldSource
exists outside of DoubleField? Or why the other 4 or 5 FieldSources that are defined inside
of their own java file exist there, while the other 4 or 5 defined inside of the FieldType's
java file exist there? What's the litmus test?
{quote}

It is not a public API and I guess that at the time it was written, there was no reason to
make it one. It was convenient or a matter of personal style or most likely a random choice.
There is no litmus test and there does not have to be one.

{quote}
Because it's more consistent, and thus, more maintainable.
{quote}

Actually it is the other way round. Once you make it public, it is harder to maintain. All
changes should then be backward compatible as far as possible. The bottom line is that making
all of them public is not needed. Your opinion is that it is broken because it is not consistent.
My opinion is that it is OK and it does not matter. We shouldn't lean towards making something
a public API in the name of consistency.

{quote}
Because when you tell someone to modify one of the core FieldSources or ValueSources, they
know where to look instead of, "oh is this one inside of a class inside of o.a.solr.schema,
or is this one in the o.a.solr.search.function package"?
{quote}

Most IDEs have a way to goto the source of a particular class, otherwise there is grep. The
point is that many (most?) of these classes don't need to be modified unless in very rare
cases. If it becomes a common practice to modify them, then there is probably something wrong
with our APIs and we need to re-think them.

> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
>                 Key: SOLR-1688
>                 URL: https://issues.apache.org/jira/browse/SOLR-1688
>             Project: Solr
>          Issue Type: Improvement
>          Components: search
>    Affects Versions: 1.4
>         Environment: indep. of env.
>            Reporter: Chris A. Mattmann
>             Fix For: 1.5
>
>         Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level),
you can't really reference FieldCacheSources that are defined inside of their FieldType constituents
(e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that
the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner
classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual
classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each
FieldCacheSource as an outside class, present in o.a.solr.search.function.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message