[
https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794833#action_12794833
]
Chris A. Mattmann edited comment on SOLR-1688 at 12/28/09 4:41 PM:
-------------------------------------------------------------------
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?
was (Author: chrismattmann):
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.
