Changes

1. SolrResourceLoader keeps a reference to the CoreDescriptor to get access to the core specific properties without breaking API compatibility.
2. CoreContainer and CoreDescriptor keep a Properties instance each for global and core-specific properties respectively
3. DOMUtil#substituteSystemProperties has been refactored (without breaking compatibility) to another method which accepts a Properties instance. This instance is checked for a value failing which, System#getProperty is used.
4. TestSolrProperties is a test for this borrowed from Henri's patch in SOLR-646. It is actually in SolrJ test sources rather than main sources for ease of testing.

The only thing not supported is that users cannot specify a variable inside solr.xml itself. Variables can only be used in the solrconfig.xml and schema.xml files (actually they can be used in any instance of Config class).

Reviews and comments are invited.

SolrResourceLoader keeps a reference to the CoreDescriptor

It would seem preferable & possible to only store the properties (versus the CoreDescriptor) in the SolrResourceLoader; this is the only piece needed for evaluation.
IMHO, the CoreDescriptor giving potential access to everything (through the CoreContainer), it should not be shared/stored outside of the SolrCore without careful impact consideration.

users cannot specify a variable inside solr.xml

You could do a second evaluation pass to expand the property values (if they are property expressions) created for the CoreContainer and for each CoreDescriptor.
If you were though, you'd need to keep the property expression map & the property value map separated.

Changes

1. Per Henri's comment, SolrResourceLoader now keeps a reference to the Properties instance instead of keeping a reference to CoreDescriptor

It would seem preferable & possible to only store the properties (versus the CoreDescriptor) in the SolrResourceLoader; this is the only piece needed for evaluation.

CoreDescriptor can be easily obtained from SolrCore anyway and SolrCore is available to every single class in Solr . SolrResourceLoader is our own class and it does not expose the object (it is private and there is no getter). This enables the resourceloader to get access to any other property from CodeDescriptor (in future) if needed .

users cannot specify a variable inside solr.xml

Is it very useful ? considering that it can only substitute system properties?

You could do a second evaluation pass to expand the property values (if they are property expressions) created for the CoreContainer and for each CoreDescriptor.

We can do that but that will require modifying the Config constructor to pass in a boolean parameter "substituteProperties". If set to false, Config should not try to substitute properties on the given node. When creating the Config object for solr.xml itself, we don't know the properties because we haven't got that point yet so we must suppress substitution. Later when we have read the global properties and we start reading the "core" node, we can substitute global+system properties on the "core" sub-tree. We can do that but I'm not sure if that's necessary. Is it very useful?

If you were though, you'd need to keep the property expression map & the property value map separated.

I didn't understand what you meant. Can you elaborate?

I guess what is already there meets most of the requirements . May be more enhancements can come in Solr1.4.

property expressions can be used in solr.xml & are preserved during solr.xml persistence (solr-716.patch).

If you were though, you'd need to keep the property expression map & the property value map separated

CoreDescriptor members can be property expressions - thus a map of expressions in both CoreDescriptor & CoreContainer to keep them around so we can reevaluate & persist them.
.
substituteProperties is made explicit so we can choose when to run it (instead of a ctor modification) and the substituteProperties() method would have been necessary anyway in that case.

Henri – I'd like some help in understanding how having expressions in solr.xml itself help make multi cores easier to use. Can we see a use-case?

So I can define properties in a core scope based on properties defined in the core-container scope.
Seems like a natural & expected feature.
Most of the code to allow idem-potent persistence allows it almost for free.
I did not expect that getting back all the solr-646 feature set (beside include) with very little modifications to your code would re-create a problem.
Sorry, I wont interfere again.

Oh no, no problem here. Interfere is the wrong word – more eyes and hands can only help

I only wanted to understand how it helps since I haven't used multiple cores a lot.

The patch looks good, some comments:

1. Why is the public constructor of CoreContainer made package private?
2. Can we remove the properties parameter in CoreContainer#evalExpressions. It is confusing to call this method with two properties instances.
3. Note that the way you create coreProperties in CoreContainer#create(SolrCore) method, the implicit properties are set after SolrConfig is created. Unless I'm misunderstanding something, you will not be able to use the implicit/automatic properties in your solrconfig.xml.
4. Can you please help me understand what this bit of code in CoreContainer#persist is doing?

   // restore the core expr name if it was one and has not changed
       String cname = StrUtils.join(aliases, ',');
       Map<String,String> pcore = dcore.getCoreExpressions();
       if (pcore != null) {
         String namex = pcore.get(null);
         if (namex != null) {
           String namev = DOMUtil.substituteProperty(namex, loader.getProperties());
           if (namev != null && namev.equals(cname))
             cname = namex;
         }
       }
   

So I can define properties in a core scope based on properties defined in the core-container scope. Seems like a natural & expected feature.

I've been thinking more on this. The core scope already inherits all properties defined in container scope. You can also override them. The only thing which is not possible is having expressions in the attributes of the <core> node such as name, instanceDir etc. Is that really necessary? If you enable expressions in a file whose information can be modified at run-time then it becomes a bit more complicated to persist back. You'd need to track both expressions and keep their evaluated values separately instead of just one object.

I feel that this discussion should not hold up the 1.3 release any more. Let us go ahead with the patch I posted and handle enhancements as part of SOLR-646 with 1.3.1 or 1.4 which ever comes first.

No changes in functionality. A bit of javadocs added.

I shall commit this shortly.

Might be post-commit but this will give a base to solr-646.
Fixed all issues raised by Shalin in previous version.
Added the persistence verification test. (thus the size bloat).

Committed revision 688359.

Thank you Henri for your support and flexibility! Let us work towards all required enhancements in the next release.

Thanks to everybody who helped out in the related issue.