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

Reply via email to