epugh commented on code in PR #2173:
URL: https://github.com/apache/solr/pull/2173#discussion_r1440574155
##########
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:
should this variable be called leaderUrl, or the message changned to
leaderCoreUrl?
##########
solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java:
##########
@@ -961,15 +960,14 @@ public Name getPermissionName(AuthorizationContext ctx) {
private static class PerReplicaCallable extends SolrRequest<SolrResponse>
implements Callable<Boolean> {
- String coreUrl;
+ Replica replica;
String prop;
int expectedZkVersion;
Number remoteVersion = null;
int maxWait;
- PerReplicaCallable(String coreUrl, String prop, int expectedZkVersion, int
maxWait) {
+ PerReplicaCallable(Replica replica, String prop, int expectedZkVersion,
int maxWait) {
Review Comment:
i like the passing of the object instead of yet another string property!
##########
solr/test-framework/src/java/org/apache/solr/BaseDistributedSearchTestCase.java:
##########
@@ -454,16 +454,7 @@ public SortedMap<Class<? extends Filter>, String>
getExtraRequestFilters() {
}
protected SolrClient createNewSolrClient(int port) {
- return getHttpSolrClient(getServerUrl(port));
- }
-
- protected String getServerUrl(int port) {
Review Comment:
love this!
##########
solr/solrj/src/java/org/apache/solr/common/util/URLUtil.java:
##########
Review Comment:
Should SolrCLI.java's "normalizeSolrUrl" method be moved over to here, or
reuse some of these methods???
##########
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:
do we still need leaderUrl? If we have leaderBaseUrl and leaderCore?
##########
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:
should this varialbe *still* be coreUrl? I guess you could say that this
core's url is the base url???
##########
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:
is this "url" variable one that is no longer needed?
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/ReindexCollectionCmd.java:
##########
@@ -711,7 +712,7 @@ private String getDaemonUrl(SolrResponse rsp, DocCollection
coll) {
// build a baseUrl of the replica
for (Replica r : coll.getReplicas()) {
if (replicaName.equals(r.getCoreName())) {
- return r.getBaseUrl() + "/" + r.getCoreName();
Review Comment:
i love that this is simpler! Lets crazy string parsing to create something
and then later split the same thing!
##########
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:
is LEADER_URL going to be awekward? Do we need a LEADER_CORE_URL?
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
