Skip to content
This repository has been archived by the owner on Sep 19, 2023. It is now read-only.

Fixes DATASOLR-264 - Extended SolrTemplate to support multicore operations #63

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

venilnoronha
Copy link
Contributor

Firstly, I've created a new interface called MulticoreSolrOperations which is similar to SolrOperations, but, all methods in MulticoreSolrOperations accept a coreName parameter i.e. to specify a solr core over which the operation should be performed. SolrTemplate now implements MulticoreSolrOperations and it heavily depends on the SolrClientFactory instance. It means that these operations would be executed on the specified Solr core only if SolrTemplate was wired with MulticoreSolrClientFactory and not HttpSolrClientFactory (as it returns the same SolrClient instance for any coreName given to its getSolrClient method).

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 coreName from SolrDocument annotated over SolrsearchRequestProductData while executing SolrTemplate#queryForFacetPage(query, SolrsearchRequestProductData.class) and performing the operation over that core. To be consistent, I modified all implemented methods of SolrOperations inside SolrTemplate to use the newly created SolrClientUtils#resolveSolrCoreName(Class<?> type, String defaultCoreName) method wherever a Class was 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 like commit couldn'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

*/
public class SolrTemplate implements SolrOperations, InitializingBean, ApplicationContextAware {
public class SolrTemplate implements SolrOperations, MulticoreSolrOperations, InitializingBean, ApplicationContextAware {

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.

Copy link
Contributor Author

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?

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.

Copy link
Contributor Author

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?

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.

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

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.

Copy link
Contributor Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: waiting-for-triage An issue we've not yet triaged
Development

Successfully merging this pull request may close these issues.

3 participants