You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/01/29 17:18:53 UTC
[GitHub] [lucene-solr] megancarey commented on a change in pull request #2234: SOLR-15094: Replace all code references of coreNodeName to replicaName
megancarey commented on a change in pull request #2234:
URL: https://github.com/apache/lucene-solr/pull/2234#discussion_r566948054
##########
File path: solr/core/src/java/org/apache/solr/cloud/CloudDescriptor.java
##########
@@ -64,7 +66,7 @@ public CloudDescriptor(CoreDescriptor cd, String coreName, Properties props) {
// If no collection name is specified, we default to the core name
this.collectionName = props.getProperty(CoreDescriptor.CORE_COLLECTION, coreName);
this.roles = props.getProperty(CoreDescriptor.CORE_ROLES, null);
- this.nodeName = props.getProperty(CoreDescriptor.CORE_NODE_NAME);
+ this.nodeName = props.getProperty(REPLICA_NAME);
Review comment:
Can we rename `nodeName` internally here as well? I think it's a little misleading
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -1438,14 +1438,14 @@ private void joinElection(CoreDescriptor cd, boolean afterExpiration, boolean jo
// we only put a subset of props into the leader node
props.put(ZkStateReader.CORE_NAME_PROP, cd.getName());
props.put(ZkStateReader.NODE_NAME_PROP, getNodeName());
- props.put(ZkStateReader.CORE_NODE_NAME_PROP, coreNodeName);
+ props.put(ZkStateReader.CORE_NODE_NAME_PROP, replicaName);
Review comment:
Why is ZkStateReader.CORE_NODE_NAME_PROP staying?
##########
File path: solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java
##########
@@ -880,7 +880,7 @@ public void testRetryUpdatesWhenClusterStateIsStale() throws Exception {
.process(cluster.getSolrClient()).getStatus());
cluster.waitForActiveCollection(COL, 1, 1);
- // determine the coreNodeName of only current replica
+ // determine the replicaName of only current replica
Review comment:
Nit: Rename variable on 888
##########
File path: solr/core/src/java/org/apache/solr/cloud/ElectionContext.java
##########
@@ -35,10 +35,10 @@
volatile String leaderSeqPath;
private SolrZkClient zkClient;
- public ElectionContext(final String coreNodeName,
+ public ElectionContext(final String relicaName,
Review comment:
Typo :) missing p
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -1438,14 +1438,14 @@ private void joinElection(CoreDescriptor cd, boolean afterExpiration, boolean jo
// we only put a subset of props into the leader node
props.put(ZkStateReader.CORE_NAME_PROP, cd.getName());
props.put(ZkStateReader.NODE_NAME_PROP, getNodeName());
- props.put(ZkStateReader.CORE_NODE_NAME_PROP, coreNodeName);
+ props.put(ZkStateReader.CORE_NODE_NAME_PROP, replicaName);
Review comment:
On line 1548 we use REPLICA_NAME instead - can we replace everywhere?
##########
File path: solr/core/src/java/org/apache/solr/cloud/CloudUtil.java
##########
@@ -56,7 +56,7 @@
public static final int DEFAULT_TIMEOUT = 90;
/**
- * See if coreNodeName has been taken over by another baseUrl and unload core
+ * See if replicaName has been taken over by another baseUrl and unload core
Review comment:
Update the log on line 80, and the variable name "cnn" as well
##########
File path: solr/core/src/test/org/apache/solr/cloud/AssignBackwardCompatibilityTest.java
##########
@@ -97,9 +97,9 @@ public void test() throws IOException, SolrServerException, KeeperException, Int
Replica newReplica = getCollectionState(COLLECTION).getReplicas().stream()
.filter(r -> r.getCoreName().equals(coreName))
.findAny().get();
- String coreNodeName = newReplica.getName();
- assertFalse("Core node name is not unique", coreNodeNames.contains(coreName));
- coreNodeNames.add(coreNodeName);
+ String replicaName = newReplica.getName();
+ assertFalse("Core node name is not unique", replicaNames.contains(coreName));
Review comment:
Update assert message to say "Replica name"
##########
File path: solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java
##########
@@ -847,7 +847,7 @@ final private void sendPrepRecoveryCmd(String leaderBaseUrl, String leaderCoreNa
WaitForState prepCmd = new WaitForState();
prepCmd.setCoreName(leaderCoreName);
prepCmd.setNodeName(zkController.getNodeName());
- prepCmd.setCoreNodeName(coreZkNodeName);
+ prepCmd.setCoreNodeName(replicaName);
Review comment:
Use new method
##########
File path: solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java
##########
@@ -137,7 +137,7 @@ protected RecoveryStrategy(CoreContainer cc, CoreDescriptor cd, RecoveryListener
zkController = cc.getZkController();
zkStateReader = zkController.getZkStateReader();
baseUrl = zkController.getBaseUrl();
- coreZkNodeName = cd.getCloudDescriptor().getCoreNodeName();
+ replicaName = cd.getCloudDescriptor().getCoreNodeName();
Review comment:
Update so we're not using deprecated method
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -2636,7 +2640,7 @@ public NotInClusterStateException(ErrorCode code, String msg) {
}
}
- public boolean checkIfCoreNodeNameAlreadyExists(CoreDescriptor dcore) {
+ public boolean checkIfReplicaNameAlreadyExists(CoreDescriptor dcore) {
Review comment:
Line 2650: use new method
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java
##########
@@ -233,7 +231,7 @@ protected void handleCustomAction(SolrQueryRequest req, SolrQueryResponse rsp) {
.put(CoreAdminParams.SHARD, CoreDescriptor.CORE_SHARD)
.put(CoreAdminParams.COLLECTION, CoreDescriptor.CORE_COLLECTION)
.put(CoreAdminParams.ROLES, CoreDescriptor.CORE_ROLES)
- .put(CoreAdminParams.CORE_NODE_NAME, CoreDescriptor.CORE_NODE_NAME)
+ .put(REPLICA_NAME, REPLICA_NAME)
Review comment:
What's the purpose of having dupe param names for the other variables, like [SHARD](https://github.com/apache/lucene-solr/blob/master/solr/solrj/src/java/org/apache/solr/common/params/CoreAdminParams.java#L79)/[CORE_SHARD](https://github.com/apache/lucene-solr/blob/master/solr/core/src/java/org/apache/solr/core/CoreDescriptor.java#L58)?
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -1666,7 +1666,7 @@ public void unregister(String coreName, CoreDescriptor cd, boolean removeCoreFro
OverseerAction.DELETECORE.toLower(), ZkStateReader.CORE_NAME_PROP, coreName,
ZkStateReader.NODE_NAME_PROP, getNodeName(),
ZkStateReader.COLLECTION_PROP, cloudDescriptor.getCollectionName(),
- ZkStateReader.CORE_NODE_NAME_PROP, coreNodeName);
+ ZkStateReader.CORE_NODE_NAME_PROP, replicaName);
Review comment:
Same comment as above - use REPLICA_NAME?
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestCloudSearcherWarming.java
##########
@@ -328,7 +328,7 @@ public void postSoftCommit() {
@Override
public void newSearcher(SolrIndexSearcher newSearcher, SolrIndexSearcher currentSearcher) {
if (sleepTime.get() > 0) {
- TestCloudSearcherWarming.coreNodeNameRef.set(newSearcher.getCore().getCoreDescriptor().getCloudDescriptor().getCoreNodeName());
+ TestCloudSearcherWarming.replicaNameRef.set(newSearcher.getCore().getCoreDescriptor().getCloudDescriptor().getCoreNodeName());
Review comment:
Use getReplicaName
##########
File path: solr/core/src/test/org/apache/solr/core/CoreSorterTest.java
##########
@@ -187,7 +188,7 @@ private CoreDescriptor newCoreDescriptor(Replica r) {
Map<String,String> props = map(
CoreDescriptor.CORE_SHARD, r.getShard(),
CoreDescriptor.CORE_COLLECTION, r.getCollection(),
- CoreDescriptor.CORE_NODE_NAME, r.getNodeName()
+ CommonParams.REPLICA_NAME, r.getNodeName()
Review comment:
r.getNodeName() should return the name of the node that the replica resides on. Use r.getName()
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/request/CoreAdminRequest.java
##########
@@ -196,11 +201,22 @@ public void setNodeName(String nodeName) {
public String getNodeName() {
return nodeName;
}
-
+
+ @Deprecated
Review comment:
Super nit: keep getters/setters together :)
##########
File path: solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java
##########
@@ -887,7 +887,7 @@ public void testRetryUpdatesWhenClusterStateIsStale() throws Exception {
.process(cluster.getSolrClient()).getStatus());
cluster.waitForActiveCollection(COL, 1, 1);
- // determine the coreNodeName of only current replica
+ // determine the replicaName of only current replica
Review comment:
Nit: rename variable on 895
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org