cpoerschke commented on a change in pull request #2152:
URL: https://github.com/apache/lucene-solr/pull/2152#discussion_r547337314
##########
File path: solr/core/src/test/org/apache/solr/cloud/HttpPartitionTest.java
##########
@@ -548,9 +548,6 @@ protected int sendDoc(int docId, Integer minRf, SolrClient
solrClient, String co
doc.addField("a_t", "hello" + docId);
UpdateRequest up = new UpdateRequest();
- if (minRf != null) {
- up.setParam(UpdateRequest.MIN_REPFACT, String.valueOf(minRf));
- }
Review comment:
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?
##########
File path: solr/core/src/test/org/apache/solr/cloud/ReplicationFactorTest.java
##########
@@ -461,38 +447,24 @@ protected void doDelete(UpdateRequest req, String msg,
int expectedRf, int retri
protected int sendDoc(int docId, int minRf) throws Exception {
UpdateRequest up = new UpdateRequest();
- 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, minRf);
}
- private int runAndGetAchievedRf(UpdateRequest up, boolean minRfExplicit, int
minRf) throws SolrServerException, IOException {
+ private int runAndGetAchievedRf(UpdateRequest up, int minRf) throws
SolrServerException, IOException {
NamedList<Object> response = cloudClient.request(up);
- if (minRfExplicit) {
- assertMinRfInResponse(minRf, response);
- }
return
cloudClient.getMinAchievedReplicationFactor(cloudClient.getDefaultCollection(),
response);
}
private void assertMinRfInResponse(int minRf, NamedList<Object> response) {
- Object minRfFromResponse = response.findRecursive("responseHeader",
UpdateRequest.MIN_REPFACT);
+ Object minRfFromResponse = response.findRecursive("responseHeader");
assertNotNull("Expected min_rf header in the response", minRfFromResponse);
assertEquals("Unexpected min_rf in response",
((Integer)minRfFromResponse).intValue(), minRf);
}
Review comment:
Looks like `assertMinRfInResponse` is now also unused then, if so
suggest to remove it also.
##########
File path: solr/core/src/test/org/apache/solr/cloud/ReplicationFactorTest.java
##########
@@ -461,38 +447,24 @@ protected void doDelete(UpdateRequest req, String msg,
int expectedRf, int retri
protected int sendDoc(int docId, int minRf) throws Exception {
UpdateRequest up = new UpdateRequest();
- 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, minRf);
}
- private int runAndGetAchievedRf(UpdateRequest up, boolean minRfExplicit, int
minRf) throws SolrServerException, IOException {
Review comment:
How about also removing the `minRf` argument here since it's now no
longer used?
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseCloudSolrClient.java
##########
@@ -716,7 +716,6 @@ protected RouteException
getRouteException(SolrException.ErrorCode serverError,
if (rf == null || routeRf < rf)
rf = routeRf;
}
- minRf = (Integer)header.get(UpdateRequest.MIN_REPFACT);
Review comment:
Suggest to also remove the `Integer minRf = null;` at line 696 above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
