Varun Thacker Mark Miller Thanks for the comments!
I think we need to deal with the "location" param better. Before this patch we used to read location as a query param . If the query param is empty then we read it from the cluster property.
With this patch we are adding the ability to specify "location" in the solr.xml file but it will never be used? CollectionsHandler will bail out early today .
This is a partial patch handling ONLY core level changes. The collection level changes are being captured in the patch for
SOLR-9055. I did this to keep the patch relatively short and easier to review. In the patch for SOLR-9055 - I have changed the CollectionsHandler implementation to read default location from solr.xml (instead of cluster property). Since this core level operation is "internal" - technically we don't have to handle the case for missing "location" param in this patch (i.e. we can keep the original behavior). I think I made this change to simplify unit testing.
One approach would be to deprecate the usage of cluster prop and look at query param followed by solr.xml ? Looking at three places seems messy .
[Mark] It is a bit odd to have some config in solr.xml and then default location as a cluster prop, but much nicer to be able to easily change the default location on the fly. solr.xml is a pain to change and requires a restart.
I agree that cluster property approach is more convenient as compared to solr.xml. But since we allow users to configure multiple repositories in solr.xml, we can not really use the current cluster property as is. This is because user may want to specify different location for different file-systems (or repositories). Hence at minimum we need one cluster property per repository configuration (e.g. name could be <repository-name>-location). But based on my understanding CLUSTERPROP API implementation requires fixed (or well-known) property names,
We may have to relax this restriction for this work. On the other hand, specifying default location in solr.xml is not so bad since user can always specify a location parameter to avoid restarting the Solr cluster. Thoughts?
Can we reuse the "location" string with (BackupRepository.DEFAULT_LOCATION_PROPERTY) on line 871/925 of CoreAdminOperation? Let's fix it in CollectionsHandler and OverseerCollectionMessageHandler as well?
Let me fix the CoreAdminOperation in this patch. I will defer the collection level changes to
In RestoreCore do we need to deprecate the old RestoreCore ctor ? Any reason why we can't remove it directly?
The deprecated constructor is used by the ReplicationHandler. The new constructor expects the BackupRepository reference which can be obtained only via CoreContainer. I couldn't find a way to get hold of CoreContainer in ReplicationHandler. Hence I didn't remove this constructor.
repo is the key used to specify the implementation. In Solr xml the tag is called repository . Should we just use repository throughout?
Sure that make sense. Let me fix this.