[ 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