[
https://issues.apache.org/jira/browse/SOLR-3251?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13632897#comment-13632897
]
Steve Rowe commented on SOLR-3251:
----------------------------------
Thanks Robert and Yonik.
Replying to Robert first:
{quote}
In CodecFactory:
{code:java}
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.
{quote}
I removed it from SchemaSimilarityFactory because when the schema is
constructed, it doesn't have a SolrCore reference, but tries to obtain
similarity from the factory, so this assert was always tripped. And if you
recall, you asked me to remove the SchemaAware {{inform()}}. Catch-22. (I
think I removed it from SchemaCodecFactory for consistency with
SchemaSimilarityFactory, so I'll try put the assert back there.)
{quote}
In SolrCore:
{code:java}
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.
{quote}
SolrCore is where the schema lives and is updated, and zkIndexSchemaReader
keeps its schema up-to-date in SolrCloud mode, so it made sense for the update
function to live where the thing being updates lives. But I don't have a
strong feeling about this - I'll move it to the factory and make the factory
SolrCoreAware.
{quote}
In SolrIndexSearcher.java:
{code:java}
/** 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:
bq. 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 :)
{quote}
I'll investigate.
{quote}
In FieldCollectionResource.java:
{code:java}
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).
{quote}
I actually originally had {{addField()}} in the base class and overrode it in
the subclass, but in the shift to immutable schema, it seemed weird to me for
it to not affect the instance on which it was being called, so I made it
static, but static methods aren't overrideable... If it gets moved back, maybe
it should be named {{addFieldsAfterCloning()}} or something?
{quote}
In ExternalFileField.java:
{code:java}
/**
* 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).
{quote}
Thanks, I'll fix. IntelliJ auto-copies javadoc when you tell it to fix
unimplemented methods... I'll see if there's a setting to not do that by
default.
{quote}
If for example, realtime-get wants to get the 'latest', it should get it from
request.getCore().getCurrentSchema() (please, name it in such a way that its
not confusing).
{quote}
I'll rename {{SolrCore.getSchema()}} to get {{getLatestSchema()}}.
----
Replying to Yonik:
{quote}
in RealTimeGetComponent, it seems like we should use
req.getCore().getSchema() instead of req.getSchema()
since one can be concurrently reading docs out of the tlog at the same time
they are being added (and hence the schema bound to the request may be too old)
{quote}
Thanks, I'll fix.
> 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
