You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/03/17 12:09:11 UTC

[GitHub] [solr] janhoy opened a new pull request #749: SOLR-16106 ClusterState and BaseHttpClusterStateProvider should not depend on ZkStateReader

janhoy opened a new pull request #749:
URL: https://github.com/apache/solr/pull/749


   https://issues.apache.org/jira/browse/SOLR-16106
   


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


[GitHub] [solr] HoustonPutman commented on a change in pull request #749: SOLR-16106 ClusterState and BaseHttpClusterStateProvider should not depend on ZkStateReader

Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #749:
URL: https://github.com/apache/solr/pull/749#discussion_r829213839



##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
##########
@@ -51,6 +50,10 @@
   private Set<String> liveNodes;
   private Set<String> hostAllowList;
 
+  // Copy of constants from ZkStateReader. Keep in sync if changed
+  public static final String NODE_NAME_PROP = "node_name";

Review comment:
       I think that a good amount of those props in ZKStateReader could live elsewhere and not be duplicated (since they aren't specific to ZK).
   
   This includes the URL_SCHEME in the file 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.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


[GitHub] [solr] janhoy commented on pull request #749: SOLR-16106 ClusterState and BaseHttpClusterStateProvider should not depend on ZkStateReader

Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #749:
URL: https://github.com/apache/solr/pull/749#issuecomment-1072496155


   > Some of this was already done by @haythemkh . I'm not sure it needs a dedicated JIRA issue (compared to an overall issue for the other things for separating SolrJ ZK).
   
   Yea, the PR #584 touches 264 source files, I forgot about it for a second, so this is perhaps duplicated.
   
   Problem with those huge PRs is that you don't get enough reviewers. Perhaps a central git feature branch is the way to go for this, and then open several small isolated PRs against that branch, and frequently merge in main branch?


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


[GitHub] [solr] HoustonPutman commented on a change in pull request #749: SOLR-16106 ClusterState and BaseHttpClusterStateProvider should not depend on ZkStateReader

Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #749:
URL: https://github.com/apache/solr/pull/749#discussion_r829213839



##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
##########
@@ -51,6 +50,10 @@
   private Set<String> liveNodes;
   private Set<String> hostAllowList;
 
+  // Copy of constants from ZkStateReader. Keep in sync if changed
+  public static final String NODE_NAME_PROP = "node_name";

Review comment:
       I think that a good amount of those props in ZKStateReader could live elsewhere and not be duplicated (since they aren't specific to ZK).
   
   This includes the URL_SCHEME in the file 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.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


[GitHub] [solr] janhoy commented on a change in pull request #749: SOLR-16106 ClusterState and BaseHttpClusterStateProvider should not depend on ZkStateReader

Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #749:
URL: https://github.com/apache/solr/pull/749#discussion_r829247192



##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
##########
@@ -51,6 +50,10 @@
   private Set<String> liveNodes;
   private Set<String> hostAllowList;
 
+  // Copy of constants from ZkStateReader. Keep in sync if changed
+  public static final String NODE_NAME_PROP = "node_name";

Review comment:
       I thought about the same. Some CloudConstants file that can be part of common, and be used even by ZkStateReader.




-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


[GitHub] [solr] janhoy commented on a change in pull request #749: SOLR-16106 ClusterState and BaseHttpClusterStateProvider should not depend on ZkStateReader

Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #749:
URL: https://github.com/apache/solr/pull/749#discussion_r829247192



##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
##########
@@ -51,6 +50,10 @@
   private Set<String> liveNodes;
   private Set<String> hostAllowList;
 
+  // Copy of constants from ZkStateReader. Keep in sync if changed
+  public static final String NODE_NAME_PROP = "node_name";

Review comment:
       I thought about the same. Some CloudConstants file that can be part of common, and be used even by ZkStateReader.




-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org