-
Notifications
You must be signed in to change notification settings - Fork 271
Fixes DATASOLR-264 - Extended SolrTemplate to support multicore operations #63
base: main
Are you sure you want to change the base?
Conversation
| * @author Venil Noronha | ||
| */ | ||
| public class SolrTemplate implements SolrOperations, InitializingBean, ApplicationContextAware { | ||
| public class SolrTemplate implements SolrOperations, MulticoreSolrOperations, InitializingBean, ApplicationContextAware { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is SolrTemplate really playing this role? Shouldn't this be implemented in MulticoreSolrTemplate?
AFAIK we can create the Template in a configuration. So it would be possible to exchange the behavior as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since SolrTemplate can be wired with a MulticoreSolrClientFactory, I think SolrTemplate should also support multicore operations. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this seems to be another problem. I think this shall be fixed first. Maybe we can re-think about the current solution. If i ask myself 'why do we need specialized classes for multi-core support?' my answer would be 'to specify the core name'. But is this really requiring all this duplication? Maybe we can use a core name provider which returns the default name in case of single core and the appropriate name in case of multi core? So we would change only the SolrTemplate by the changes in SolrOperations. Maybe we find a way how to provide the name without any change of SolrOperations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we handle methods like commit() and saveDocument(SolrInputDocument document) from SolrOperations then? How do we identify the core name in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the crux. We need a way to provide the appropriate core name from inside the SolrTemplate. In that case we don't need any core name inside SolrOperations. The implementer would add it to the executor call.
Or maybe we can 'find' SolrOperations for a core name and provide an appropriately prepared instance. Just because solrCore is already an attribute in SolrTemplate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But does that make sense? We use a single SolrTemplateto query multiple cores defined by SolrDocument.solrCoreName. On the other hand we need to solve problems to write changes to correct core. This screams to CQRS.
My main fear is that we get an instable Multicore Support by "duplicating" the SolrOperations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a look at MongoTemplate and it has a lot of similar methods i.e. to perform operations on specified collections. For instance, public void insert(Collection<? extends Object> batchToSave, String collectionName). So, for me it seems to be a default solution to accept collection name as an additional parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we go the MongoTemplate way or should we have a pure MulticoreSolrOperations implementation, not touching SolrTemplate? What do you say? Going the MongoTemplate way will give us better API consistency is what I feel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that it is not easy to solve this. Some more changes in determining the appropriate SolrTemplateare required. If your solution is appropriate to current design (and maybe API standards) it is maybe the right way. I think i made my concerns clear. Sorry for not providing code on this. But i'm currently really busy. I put it on my list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @rene-d-menoto for your inputs! I'll give it some more thought to find if there is a better way.
a8ad435 to
82914d5
Compare
3817842 to
22854d4
Compare
Firstly, I've created a new interface called
MulticoreSolrOperationswhich is similar toSolrOperations, but, all methods inMulticoreSolrOperationsaccept acoreNameparameter i.e. to specify a solr core over which the operation should be performed.SolrTemplatenow implementsMulticoreSolrOperationsand it heavily depends on theSolrClientFactoryinstance. It means that these operations would be executed on the specified Solr core only ifSolrTemplatewas wired withMulticoreSolrClientFactoryand notHttpSolrClientFactory(as it returns the same SolrClient instance for any coreName given to itsgetSolrClientmethod).The fix for the issue didn't have anything to do with what I've described above i.e. it was just about extracting the
coreNamefromSolrDocumentannotated overSolrsearchRequestProductDatawhile executingSolrTemplate#queryForFacetPage(query, SolrsearchRequestProductData.class)and performing the operation over that core. To be consistent, I modified all implemented methods ofSolrOperationsinsideSolrTemplateto use the newly createdSolrClientUtils#resolveSolrCoreName(Class<?> type, String defaultCoreName)method wherever aClasswas present in the method parameters. This however didn't seem to be a complete solution as not all methods were, in a sense, "multicore" supportive i.e. methods likecommitcouldn't operate on a core other than the default one. This was the reason for creatingMulticoreSolrOperations.Please review and pull if the solution looks fine. My CLA number is 149720151120072904.
Thanks,
Venil Noronha