-
Notifications
You must be signed in to change notification settings - Fork 492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migration HttpSolrClient to Http2SolrClient and ConcurrentUpdateHttp2SolrClient #10241
base: develop
Are you sure you want to change the base?
Migration HttpSolrClient to Http2SolrClient and ConcurrentUpdateHttp2SolrClient #10241
Conversation
with refactoring in order to use one service to query solr, and another service to manage index
@jeromeroucou - thanks for the PR! A ~quick question - have you run this extensively? I ask because I tried to make a simple switch from HttpSolrClient to Http2SolrClient in the SolrClientService class at QDR and started seeing errors w.r.t. the client being closed that started after some delay (~days). It's possible that there were restarts of solr going on at QDR so I just reverted and was going to check again at some point after the solr changes were done, but I haven't yet. If the new Http2SolrClient does somehow timeout (or perhaps it is just worse at handling a solr restart?), we'd want to address that (e.g. with reinitialize). If you haven't tried running this branch for days, I may try again at QDR and/or will make sure that we test a longer duration in QA. |
Hi @qqmyers, thanks for this feedback. No, I don't run |
@jeromeroucou hi! Any results to share? Thanks! |
Unfortunately no, our deployment environment is still broken. We're still working on restoring it. I hope to be able to give you an update within 2 weeks. |
General remark on the PR: the HTTP2 classes are still flagged experimental, see https://solr.apache.org/guide/solr/latest/deployment-guide/solrj.html#types-of-solrclients. Do we really want to do this? Should we potentially use a feature-flag, so it keeps the experimental nature also in Dataverse? |
Hi @poikilotherm, I think the documentation you're referring to specifies that the client is in progress (I understand that it's not out of the question for a method signature to be modified for exemple). However, Solr Javadoc clearly indicates But your proposition to use a feature-flag may be a good thing until |
The PR is finally ready for review again! I've taken @poikilotherm feedback into account by using a feature flag. Regarding @qqmyers comments, we deployed this branch in an environment that remained switched on for over a week, without observing any connection interruption. However, not really knowing how to check, we may not have noticed the errors? |
Thanks @jeromeroucou. I had mischaracterized this PR as a feature but it's really more of a bug fix (stopping the use of deprecated code) so the team will talk about it at our weekly review of "ready for triage" on Tuesday. |
What this PR does / why we need it:
This PR replace deprecated
HttpSolrClient
with 2 new implementations :Http2SolrClient
to make queries to SolrConcurrentUpdateHttp2SolrClient
to perform index to SolrWhich issue(s) this PR closes:
Closes #10161
Special notes for your reviewer:
I've been careful to centralize code into an abstract class, and delete local client on
IndexServiceBean
to use instead the 2 new service client (one for queries, one for indexation)Suggestions on how to test this:
I run some local tests on my laptop : create collection, publish dataset with files, search on index page, harvesting other repository, call
clear-orphan
API. But I didn't index a large collection to measure if performance had improved.Does this PR introduce a user interface change? If mockups are available, please link/include them here:
N/A
Is there a release notes update needed for this change?:
If indexation performance is increase, I think it would be nice to mention it.
Additional documentation:
Configuration documentation has been updated with the new feature-flag to enable it.