lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Muir (JIRA)" <>
Subject [jira] [Commented] (SOLR-3251) dynamically add field to schema
Date Tue, 16 Apr 2013 01:09:14 GMT


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:

   public Codec getCodec() {
-    assert codec != null : "inform must be called first";

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:
    if (schema instanceof ManagedIndexSchema && schema.isMutable() && resourceLoader
instanceof ZkSolrResourceLoader) {
      this.zkIndexSchemaReader = new ZkIndexSchemaReader(this);
    } else {
      this.zkIndexSchemaReader = null;

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.

   /** Direct access to the IndexSchema for use with this searcher */
-  public IndexSchema getSchema() { return schema; }
+  public IndexSchema getSchema() { return core.getSchema(); }

I'm confused about this in conjunction with your previous comment:

Schema is now effectively immutable: requests see the same schema snapshot for their lifetimes.

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 :)

    ManagedIndexSchema newSchema = ManagedIndexSchema.addFields(getSolrCore(), newFieldsArray);

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

   * 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,
   * @param schema The {@link org.apache.solr.schema.IndexSchema} instance that inform of
the update to.
   * @since SOLR-1131
  public void inform(IndexSchema schema) {

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:
>             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,
> 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:

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message