SOLR-14034: remove deprecated min_rf references #2152

trdillon · 2020-12-17T09:17:47Z

Description

Remove min_rf and MIN_REPFACT references in code, tests and documentation.

Solution

Incrementally remove min_rf and MIN_REPFACT references.

Tests

./gradlew check

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 master branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Ref Guide (for Solr changes only).

madrob

LGTM, will wait for another set of eyes before committing. @cpoerschke?

@trdillon can you add an entry to solr/CHANGES with your preferred attribution?

solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseCloudSolrClient.java

cpoerschke · 2020-12-22T15:21:25Z

solr/core/src/test/org/apache/solr/cloud/HttpPartitionTest.java

-    if (minRf != null) {
-      up.setParam(UpdateRequest.MIN_REPFACT, String.valueOf(minRf));
-    }


Hi @trdillon - thanks for opening this pull request!

Okay, I think I see it now, so your https://issues.apache.org/jira/browse/SOLR-14034?focusedCommentId=17223427&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17223427 question is about this sort of test change here i.e. whether or not minRf should remain an unused sendDoc argument or whether or not it should be removed. I'm thinking here in HttpPartitionTest removal makes sense (haven't yet looked at ReplicationFactorTest). What do you think?

@cpoerschke Thanks for all your help with this one.

sendDoc seems OK to remove from HttpPartitionTest as it's only passed minRf as an argument, but in ReplicationFactorTest it is passed expectedRf as an argument in addDocs:

lucene-solr/solr/core/src/test/org/apache/solr/cloud/ReplicationFactorTest.java

Line 417 in 98f12f4

sendDoc(idList[0], expectedRf);

sendDocsWithRetry is implemented here and used in a few different test classes (DistributedVersionInfoTest, LeaderFailoverAfterPartitionTest and ForceLeaderTest) with minRf as an argument:

lucene-solr/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java

Line 941 in 98f12f4

protected static int sendDocsWithRetry(CloudSolrClient cloudClient, String collection, List<SolrInputDocument> batch, int minRf, int maxRetries, int waitBeforeRetry) throws Exception {

But it's also implemented using expectedRfDBQ and expectedRf in ReplicationFactorTest:

lucene-solr/solr/core/src/test/org/apache/solr/cloud/ReplicationFactorTest.java

Line 230 in 98f12f4

sendDocsWithRetry(batch, expectedRfDBQ, 5, 1);

lucene-solr/solr/core/src/test/org/apache/solr/cloud/ReplicationFactorTest.java

Line 427 in 98f12f4

sendDocsWithRetry(batch, expectedRf, retries, 1);

I wasn't sure how to proceed with this as looking at the tests it seems that expectedRf is relied upon quite often.

Thanks for the code pointers and detailed explanation!

How about we scope this pull request to remove minRf in almost all code paths i.e. sendDoc gets its unused argument removed in both the replication factor and http partition tests but sendDocsWithRetry remains unchanged for now since as you say it's used in other test classes too?

It appears that the minRf argument in sendDocsWithRetry is indeed unused already but it might be clearer to tidy that up in a separate follow-up pull request rather than mix it in here at this time.

It appears that the minRf argument in sendDocsWithRetry is indeed unused already but it might be clearer to tidy that up in a separate follow-up pull request rather than mix it in here at this time.

I'm wondering if it would be better for this method to actually start using this parameter, and retry in the case of minRf not achieved? This is not something that was being done in this method, but may help improve some of the tests using it. I don't think such a change should be done in this PR, but maybe lets not remove the parameter yet then. WDYT?

I'm wondering if it would be better for this method to actually start using this parameter, ...

Ah, yes, I agree, if it's a case of a not-yet-used parameter then keeping it and starting to use it makes sense. Removal would only be for no-longer-used parameter cases. Good catch!

solr/core/src/test/org/apache/solr/cloud/ReplicationFactorTest.java

cpoerschke · 2020-12-22T15:38:06Z

At https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.7.0/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java#L754-L756 there's also a Implementing min_rf here was a bit tricky. ... comment reference to min_rf -- any thoughts on either removing it or rewording it somehow?

cpoerschke · 2020-12-23T18:07:43Z

solr/core/src/test/org/apache/solr/cloud/ReplicationFactorTest.java

-    boolean minRfExplicit = maybeAddMinRfExplicitly(minRf, up);
    SolrInputDocument doc = new SolrInputDocument();
    doc.addField(id, String.valueOf(docId));
    doc.addField("a_t", "hello" + docId);
    up.add(doc);
-    return runAndGetAchievedRf(up, minRfExplicit, minRf);
+    return runAndGetAchievedRf(up);


Alright, so with maybeAddMinRfExplicitly gone and the minRf argument in runAndGetAchievedRf gone the minRf argument of sendDoc becomes unused, right? I wonder how the code would work out if sendDoc had its minRf argument taken away too? Bit like peeling an onion this here i.e. multiple layers (but it hopefully won't make anyone cry).

The tests for ReplicationFactorTest and HttpPartitionTest passed on my end after removing the minRf argument from sendDoc. I committed the ReplicationFactorTest changes separately as it meant removing the expectedRf argument from the sendDoc call in addDocs and I wasn't sure if this was OK, even though the test still passed.

I appreciate you committing the changes separately, thanks. The ReplicationFactorTest change looks good to me.

cpoerschke · 2020-12-23T18:35:46Z

solr/core/src/test/org/apache/solr/cloud/HttpPartitionTest.java

-    if (minRf != null) {
-      up.setParam(UpdateRequest.MIN_REPFACT, String.valueOf(minRf));
-    }


Thanks for the code pointers and detailed explanation!

How about we scope this pull request to remove minRf in almost all code paths i.e. sendDoc gets its unused argument removed in both the replication factor and http partition tests but sendDocsWithRetry remains unchanged for now since as you say it's used in other test classes too?

It appears that the minRf argument in sendDocsWithRetry is indeed unused already but it might be clearer to tidy that up in a separate follow-up pull request rather than mix it in here at this time.

trdillon · 2020-12-26T03:54:15Z

At https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.7.0/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java#L754-L756 there's also a Implementing min_rf here was a bit tricky. ... comment reference to min_rf -- any thoughts on either removing it or rewording it somehow?

The comment still seems to be relevant and makes sense without the first sentence that references minRf. From my limited perspective I think it would be OK to just remove the first sentence.

tflobbe

LGTM

tflobbe · 2020-12-30T06:24:07Z

solr/core/src/test/org/apache/solr/cloud/HttpPartitionTest.java

-    if (minRf != null) {
-      up.setParam(UpdateRequest.MIN_REPFACT, String.valueOf(minRf));
-    }


It appears that the minRf argument in sendDocsWithRetry is indeed unused already but it might be clearer to tidy that up in a separate follow-up pull request rather than mix it in here at this time.

I'm wondering if it would be better for this method to actually start using this parameter, and retry in the case of minRf not achieved? This is not something that was being done in this method, but may help improve some of the tests using it. I don't think such a change should be done in this PR, but maybe lets not remove the parameter yet then. WDYT?

cpoerschke

LGTM. Will aim to proceed to merge the PR about this time tomorrow if there are no further comments etc. Thanks!

cpoerschke · 2020-12-30T13:10:19Z

solr/core/src/test/org/apache/solr/cloud/ReplicationFactorTest.java

-    boolean minRfExplicit = maybeAddMinRfExplicitly(minRf, up);
    SolrInputDocument doc = new SolrInputDocument();
    doc.addField(id, String.valueOf(docId));
    doc.addField("a_t", "hello" + docId);
    up.add(doc);
-    return runAndGetAchievedRf(up, minRfExplicit, minRf);
+    return runAndGetAchievedRf(up);


I appreciate you committing the changes separately, thanks. The ReplicationFactorTest change looks good to me.

cpoerschke · 2020-12-30T13:14:50Z

solr/core/src/test/org/apache/solr/cloud/HttpPartitionTest.java

-    if (minRf != null) {
-      up.setParam(UpdateRequest.MIN_REPFACT, String.valueOf(minRf));
-    }


I'm wondering if it would be better for this method to actually start using this parameter, ...

Ah, yes, I agree, if it's a case of a not-yet-used parameter then keeping it and starting to use it makes sense. Removal would only be for no-longer-used parameter cases. Good catch!

Removed min_rf and MIN_REPFACT parameter references

dfea135

madrob reviewed Dec 18, 2020

View reviewed changes

Added SOLR-14034 to CHANGES.txt

72dea2e

cpoerschke reviewed Dec 22, 2020

View reviewed changes

Remove assertMinRfInResponse and minRf references

77e7316

cpoerschke reviewed Dec 23, 2020

View reviewed changes

trdillon added 2 commits December 26, 2020 12:19

Remove minRf argument from sendDoc

e109454

Remove minRf argument from sendDoc in ReplicationFactorTest

babf2c8

tflobbe approved these changes Dec 30, 2020

View reviewed changes

cpoerschke approved these changes Dec 30, 2020

View reviewed changes

cpoerschke merged commit 17adcc7 into apache:master Dec 31, 2020

trdillon deleted the jira/SOLR-14034 branch January 2, 2021 06:58

ctargett pushed a commit to ctargett/lucene-solr that referenced this pull request Jan 11, 2021

SOLR-14034: remove deprecated min_rf references (apache#2152)

eecd808

epugh pushed a commit to epugh/lucene-solr-1 that referenced this pull request Jan 15, 2021

SOLR-14034: remove deprecated min_rf references (apache#2152)

e8065f4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-14034: remove deprecated min_rf references #2152

SOLR-14034: remove deprecated min_rf references #2152

trdillon commented Dec 17, 2020

madrob left a comment

cpoerschke Dec 22, 2020

trdillon Dec 22, 2020

cpoerschke Dec 23, 2020

tflobbe Dec 30, 2020

cpoerschke Dec 30, 2020

cpoerschke commented Dec 22, 2020

cpoerschke Dec 23, 2020

trdillon Dec 26, 2020

cpoerschke Dec 30, 2020

cpoerschke Dec 23, 2020

trdillon commented Dec 26, 2020

tflobbe left a comment

tflobbe Dec 30, 2020

cpoerschke left a comment

cpoerschke Dec 30, 2020

cpoerschke Dec 30, 2020

SOLR-14034: remove deprecated min_rf references #2152

SOLR-14034: remove deprecated min_rf references #2152

Conversation

trdillon commented Dec 17, 2020

Description

Solution

Tests

Checklist

madrob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpoerschke commented Dec 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trdillon commented Dec 26, 2020

tflobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpoerschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment