[
https://issues.apache.org/jira/browse/SOLR-3251?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13632457#comment-13632457
]
Robert Muir commented on SOLR-3251:
-----------------------------------
Steve asked me for a review, so I took a quick look, just a few things i
noticed (the codec/sim factory is much better without the 2 inform methods,
thanks!):
In CodecFactory:
{code}
public Codec getCodec() {
- assert codec != null : "inform must be called first";
{code}
Why remove this assert? I think this is pretty useful otherwise you can get a
difficult-to-diagnose NPE. Same goes with the SimilarityFactory.
In SolrCore:
{code}
if (schema instanceof ManagedIndexSchema && schema.isMutable() &&
resourceLoader instanceof ZkSolrResourceLoader) {
this.zkIndexSchemaReader = new ZkIndexSchemaReader(this);
} else {
this.zkIndexSchemaReader = null;
}
{code}
Why is this in SolrCore? Nothing in SolrCore uses this "zkIndexSchemaReader". I
dont think this belongs here: i think it should be in
ManagedIndexSchemaFactory... like it should be core-aware or whatever and do
this itself.
In SolrIndexSearcher.java:
{code}
/** Direct access to the IndexSchema for use with this searcher */
- public IndexSchema getSchema() { return schema; }
+ public IndexSchema getSchema() { return core.getSchema(); }
{code}
I'm confused about this in conjunction with your previous comment:
{quote}
Schema is now effectively immutable: requests see the same schema snapshot for
their lifetimes.
{quote}
Then isn't it dangerous for things to be pulling moving-target schemas off of
SolrCores/SolrIndexSearchers? Shouldn't they be only getting this from the
request? I made this package-private just to see the damage and its not clear
to me that your statement really holds for all this query code :)
In FieldCollectionResource.java:
{code}
ManagedIndexSchema newSchema = ManagedIndexSchema.addFields(getSolrCore(),
newFieldsArray);
getSolrCore().setSchema(newSchema);
{code}
It would be nice if we could at least add a TODO to refactor some of this. I
think its a little confusing that IndexSchema itself has getMutable, but
operations like this go directly to the implementation (abstraction violation).
From a pluggability perspective it would be nice if e.g. addFields was factored
down (e.g. IndexSchema becomes abstract and minimal), and the immutable default
impl threw UOE for changes or whatever... But i know this is a lot of work, it
would be a good followup issue and probably good to do before schema gets any
more hair (there is already tons of backwards cruft thrown about it for compat
etc too).
In ExternalFileField.java:
{code}
/**
* Informs the {@link org.apache.solr.schema.IndexSchema} provided by the
<code>schema</code>
* parameter of an event (e.g., a new {@link
org.apache.solr.schema.FieldType} was added, etc.
*
* @param schema The {@link org.apache.solr.schema.IndexSchema} instance that
inform of the update to.
* @since SOLR-1131
*/
@Override
public void inform(IndexSchema schema) {
{code}
This should be unnecessary duplication... javadocs by default copies this from
the overridden interface (SchemaAware). So I'd remove it completely, if there
is anything ExternalFileField-specific that needs to be appended to this, then
the base doc can be sucked in with inheritDoc.
(the same goes for several other classes, e.g. i see this in
ExternalFileFieldReloader too).
> dynamically add field to schema
> -------------------------------
>
> Key: SOLR-3251
> URL: https://issues.apache.org/jira/browse/SOLR-3251
> Project: Solr
> Issue Type: New Feature
> Reporter: Yonik Seeley
> Assignee: Steve Rowe
> Fix For: 4.3
>
> Attachments: SOLR-3251.patch, SOLR-3251.patch, SOLR-3251.patch,
> SOLR-3251.patch, SOLR-3251.patch
>
>
> One related piece of functionality needed for SOLR-3250 is the ability to
> dynamically add a field to the schema.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
