gerlowskija commented on code in PR #2173:
URL: https://github.com/apache/solr/pull/2173#discussion_r1440817786
##########
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##########
@@ -332,24 +336,26 @@ private void setLeaderUrl(String leaderUrl) {
solrCore
.getCoreContainer()
.getAllowListUrlChecker()
- .checkAllowList(Collections.singletonList(leaderUrl),
clusterState);
+ .checkAllowList(Collections.singletonList(leaderCoreUrl),
clusterState);
} catch (MalformedURLException e) {
throw new SolrException(
- SolrException.ErrorCode.SERVER_ERROR, "Malformed 'leaderUrl' " +
leaderUrl, e);
+ SolrException.ErrorCode.SERVER_ERROR, "Malformed 'leaderUrl' " +
leaderCoreUrl, e);
} catch (SolrException e) {
throw new SolrException(
SolrException.ErrorCode.FORBIDDEN,
"The '"
+ LEADER_URL
Review Comment:
Ditto, re: my reply on L342 above.
##########
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##########
@@ -216,16 +217,26 @@ protected String getReplicateLeaderUrl(ZkNodeProps
leaderprops) {
return new ZkCoreNodeProps(leaderprops).getCoreUrl();
}
- private final void replicate(String nodeName, SolrCore core, ZkNodeProps
leaderprops)
+ protected String getReplicateLeaderBaseUrl(ZkNodeProps leaderProps) {
+ return new ZkCoreNodeProps(leaderProps).getBaseUrl();
+ }
+
+ protected String getReplicateLeaderCoreName(ZkNodeProps leaderProps) {
+ return new ZkCoreNodeProps(leaderProps).getCoreName();
+ }
+
+ private void replicate(String nodeName, SolrCore core, ZkNodeProps
leaderprops)
throws SolrServerException, IOException {
+ final String leaderBaseUrl = getReplicateLeaderBaseUrl(leaderprops);
+ final String leaderCore = getReplicateLeaderCoreName(leaderprops);
final String leaderUrl = getReplicateLeaderUrl(leaderprops);
log.info("Attempting to replicate from [{}].", leaderUrl);
Review Comment:
I'm not sure. RecoveryStrategy looks like its supposed to be at least
semi-pluggable - so we should probably keep getReplicateLeaderUrl as a part of
that interface. (Though there are 'lucene.experimental' annotations all over
this class, so maybe we're not held to backcompat here?)
But I guess we could get rid of the variable itself if we wanted to.
##########
solr/core/src/java/org/apache/solr/update/StreamingSolrClients.java:
##########
@@ -71,7 +72,10 @@ public synchronized SolrClient getSolrClient(final
SolrCmdDistributor.Req req) {
// reordered on a greater scale since the current behavior is to only
increase the number of
// connections/Runners when the queue is more than half full.
client =
- new ErrorReportingConcurrentUpdateSolrClient.Builder(url,
httpClient, req, errors)
Review Comment:
No, it's still needed as a key to the Map that holds each client.
##########
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##########
@@ -332,24 +336,26 @@ private void setLeaderUrl(String leaderUrl) {
solrCore
.getCoreContainer()
.getAllowListUrlChecker()
- .checkAllowList(Collections.singletonList(leaderUrl),
clusterState);
+ .checkAllowList(Collections.singletonList(leaderCoreUrl),
clusterState);
} catch (MalformedURLException e) {
throw new SolrException(
- SolrException.ErrorCode.SERVER_ERROR, "Malformed 'leaderUrl' " +
leaderUrl, e);
+ SolrException.ErrorCode.SERVER_ERROR, "Malformed 'leaderUrl' " +
leaderCoreUrl, e);
Review Comment:
I think 'coreUrl' adds clarity in the code for devs, but the log message
should probably remain unchanged because the url value this code is handling
ultimately comes from a config-property/parameter called 'leaderUrl'. Since
the log msg is for user consumption IMO matching that config-prop name makes
sense.
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchema.java:
##########
@@ -281,7 +282,7 @@ public static void waitForSchemaZkVersionAgreement(
}
if (vers == -1) {
- String coreUrl = concurrentTasks.get(f).coreUrl;
+ String coreUrl = concurrentTasks.get(f).baseUrl;
Review Comment:
The problem here isn't the names idt - they're both accurate. I just didn't
notice the mismatch. Fixed.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
