SOLR-17290: Update SyncStrategy and PeerSyncWithLeader to use the recovery Http2SolrClient

[iamsanjay]

https://issues.apache.org/jira/browse/SOLR-17290

As per the discussion on this thread SOLR-16505, It has been identified that PeerSyncWithLeader and SyncStrategy both were using the default HttpClient. Because both of these classes are meant for "recovery" operations, then why not switch their client to use recovery Http Client.

Also, now that SOLR-16505 has been merged, the above change will enable PeerSyncWithLeader and SyncStrategy to use Jetty HTTP2 client.

Tests

No Test Cases Provided.

Checklist

Please review the following and check all that apply:

• I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
• I have created a Jira issue and added the issue ID to my pull request title.
• I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
• I have developed this patch against the main branch.
• I have run ./gradlew check.
• I have added tests for my changes.
• I have added documentation for the Reference Guide

[dsmiley]

I don't think this PR should be for SOLR-16503. It should be SOLR-16505 (it's okay it's closed, can be reopened; is unreleased so there's time) as it's focused on "recovery". Or perhaps a new JIRA.

[dsmiley]

The code changes look good.

[iamsanjay]

I don't think this PR should be for SOLR-16503. It should be SOLR-16505 (it's okay it's closed, can be reopened; is unreleased so there's time) as it's focused on "recovery". Or perhaps a new JIRA.

New JIRA, Done!

[iamsanjay]

For SyncStrategy we have re-created Http2SolrClient due to custom timeouts and never called withListenerFactory meaning the auth listener is not available. The solr client is being used for REQUESTRECOVERY and did not seem to need the auth for that.

[dsmiley]

+1; will merge tomorrow.

The solr client is being used for REQUESTRECOVERY and did not seem to need the auth for that.

Maybe you know something I don't here; what technical mechanism doesn't happen for recovery for auth? AFAIK all Solr-to-Solr communication needs to support AuthenticationPlugin. And I believe it is == see org.apache.solr.update.UpdateShardHandler#setSecurityBuilder invoked by the auth plugin initialization and that which configures the update & recovery clients.

[iamsanjay]

+1; will merge tomorrow.

The solr client is being used for REQUESTRECOVERY and did not seem to need the auth for that.

Maybe you know something I don't here; what technical mechanism doesn't happen for recovery for auth? AFAIK all Solr-to-Solr communication needs to support AuthenticationPlugin. And I believe it is == see org.apache.solr.update.UpdateShardHandler#setSecurityBuilder invoked by the auth plugin initialization and that which configures the update & recovery clients.

I found a bug in SOLR-16505!
RecoveryStrategy is using a client without auth listener. It did not failed because, i believe, there is no specific test for Recovery where SolrCloud is configured along with Security.json. Going to submit PR today for a fix.
@dsmiley Please open again SOLR-16505.

[epugh]

Would love some thoughts on how we could handle the auth scenarios better without having to write an explicit test for it on every piece of code, once with auth and once with out ;-)

[iamsanjay]

Would love some thoughts on how we could handle the auth scenarios better without having to write an explicit test for it on every piece of code, once with auth and once with out ;-)

As per the quick search for the usages of MiniSolrCloudCluster#withSecurityJson, There are not many test cases that have written along with Auth enabled in SolrCloud environment, and apparently it's none for this specific scenario #2462 where a RecoveryStrategy is begin tested in Auth enabled environment. I am not sure how many cases this new test class #2462 can cover, however the idea would be to keep on extending this one to cover more cases for Cloud related scenario.

For instance, after merging #2462, same test case would be extends to test SyncStrategy and PeerSyncWithLeader Http2SolrClient calls to make sure that they are executing without any errors in auth enabled SolrCloud environment.

[dsmiley]

Should this be merged or should it be pending some authentication/authorization fix?

[iamsanjay]

I updated the SyncStrategy to call withListenerFactory. However, this one can wait, let's merge this one first #2467 to fix RecoveryStrategy.

[dsmiley]

Just one little glitch here but otherwise I approve.