lucene-solr-dev mailing list archives

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

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

Chris A. Mattmann commented on SOLR-1688:
-----------------------------------------

bq. 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.

Yes, I am, because it's better to be extremely consistent when you are developing a code base
that's seen by thousands of developers around the world.

bq. 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.

Um, actually it _is_ a public API. ValueSource and FieldCacheSource are both public classes
within the o.a.solr.search.function package. Anyone can write them. And if you're agreeing
it was a random choice, why not turn it into a principled decision?

{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}

Actually it's not the other way around. Public APIs aren't harder to maintain. It's kind of
a matter of debate. It's also mixing levels of granularity because public from an external
(client) to SOLR server interface perspective is different from public at the code, library
or function level as part of SOLR. Additionally, changes being backwards compatible is one
of the many non-functional concerns -- there are others. Ease of use, portability, maintainability,
understandability, etc.You can't have a blanket policy for maximizing one, without affecting
the others.

Let's be clear. I'm not advocating that something be made a public API that _isn't_ already
public. ValueSource and FieldCacheSource are public _classes_. And note the difference between
_class_ and _API_. ValueSource and FieldCacheSource are not _API_s, they are Java classes
(different levels of granularity). 

Because of this class-level decision, the codebase itself contains inconsistent use of these
2 classes:

* as separate Java classes defined in a FieldType's Java file
* as Java classes defined in their own Java file that lives within o.a.solr.search.function

Also let's be clear also -- I never said "broken", I said "inconsistent". If what you're saying
is that you as a SOLR committer don't care about inconsistency, then I'm sorry to hear that.


{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.
{quote}

If you're advocating using grep or using an IDE's search functionality as opposed to just
understanding where code should be located based on principled architectural design, then
again, I'm sorry to hear that. Especially when the principled design comes at 0 cost. The
patch I attached didn't break anything, didn't change any APIs, furthermore, didn't change
any Java classes, didn't change anything. All I did was re-organize the code to be a bit more
principled in its organization. I can explain why I would put all the code in o.a.solr.search.function.
Can you explain why it's not?

> 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