lucene-dev mailing list archives

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

    [ 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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org


Mime
View raw message