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 2020/03/31 14:25:49 UTC
[GitHub] [lucene-solr] janhoy opened a new pull request #1392: SOLR-14371 Zk
StatusHandler should know about dynamic zk config
janhoy opened a new pull request #1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
URL: https://github.com/apache/lucene-solr/pull/1392
See https://issues.apache.org/jira/browse/SOLR-14371
Still draft
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] HoustonPutman commented on a change in pull request
#1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
URL: https://github.com/apache/lucene-solr/pull/1392#discussion_r401042663
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ZookeeperStatusHandler.java
##########
@@ -108,27 +128,30 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
if ("true".equals(String.valueOf(stat.get("ok")))) {
numOk++;
}
- String state = String.valueOf(stat.get("zk_server_state"));
+ String state = zk.role != null ? zk.role : String.valueOf(stat.get("zk_server_state"));
if ("follower".equals(state)) {
followers++;
} else if ("leader".equals(state)) {
leaders++;
reportedFollowers = Integer.parseInt(String.valueOf(stat.get("zk_followers")));
} else if ("standalone".equals(state)) {
standalone++;
+ } else if ("participant".equals(state)) {
+ // NOCOMMIT: What does participant mean vs leader or follower?
Review comment:
Looking at the docs here https://zookeeper.apache.org/doc/r3.5.7/zookeeperReconfig.html, in the "Specifying the client port" section, it looks like the role of the zk instance is either observer or participant (where participant is the default). Participant isn't a new thing, just a name for a regular member of the ensemble (not an observer).
This state check change may be making everything a "participant" as that's the default role. Maybe keep the original state logic, but add in an additional check in the beginning to see if the role is observer?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] HoustonPutman commented on a change in pull request
#1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
URL: https://github.com/apache/lucene-solr/pull/1392#discussion_r401045606
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java
##########
@@ -818,6 +834,49 @@ public void downloadFromZK(String zkPath, Path dir) throws IOException {
ZkMaintenanceUtils.downloadFromZK(this, zkPath, dir);
}
+ /**
+ * Represents one line in dynamic ZK config
+ */
+ public static class ZkConfigDyn {
+ // server.<positive id> = <address1>:<port1>:<port2>[:role];[<client port address>:]<client port>
+ public static Pattern linePattern = Pattern.compile("server\\.(?<serverId>\\d+) ?= ?(?<address>[^:]+):(?<leaderPort>\\d+):(?<leaderElectionPort>\\d+)(:(?<role>.*?))?(;((?<clientPortAddress>.*?):)?(?<clientPort>\\d+))?");
+ public int serverId;
Review comment:
these can all likely be final correct?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] HoustonPutman commented on a change in pull request
#1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
URL: https://github.com/apache/lucene-solr/pull/1392#discussion_r401790833
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ZookeeperStatusHandler.java
##########
@@ -109,25 +133,30 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
numOk++;
}
String state = String.valueOf(stat.get("zk_server_state"));
- if ("follower".equals(state)) {
+ if ("follower".equals(state) || "observer".equals(state)) {
followers++;
} else if ("leader".equals(state)) {
leaders++;
reportedFollowers = Integer.parseInt(String.valueOf(stat.get("zk_followers")));
} else if ("standalone".equals(state)) {
standalone++;
}
+ if (zk.role != null) {
+ stat.put("role", zk.role);
+ dynamicReconfig = true;
Review comment:
I think this should live up here where the reconfig check is done. https://github.com/apache/lucene-solr/pull/1392/files#diff-2f0decb1314fc7f7d3e6e0f497839745R109
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on issue #1392: SOLR-14371 Zk
StatusHandler should know about dynamic zk config
Posted by GitBox <gi...@apache.org>.
janhoy commented on issue #1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
URL: https://github.com/apache/lucene-solr/pull/1392#issuecomment-610384085
I was not happy with the `ZkConfigDyn` class inside SolrZkClient, so I am factoring it out into its own class `ZkDynamicConfig` which is passed to `ZookeeperStatusHandler.getZkStatus()` instead of the list.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy edited a comment on issue #1392: SOLR-14371 Zk
StatusHandler should know about dynamic zk config
Posted by GitBox <gi...@apache.org>.
janhoy edited a comment on issue #1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
URL: https://github.com/apache/lucene-solr/pull/1392#issuecomment-610384085
I was not happy with the `ZkConfigDyn` class inside SolrZkClient, so I am factoring it out into its own class `ZkDynamicConfig` which is passed to `ZookeeperStatusHandler.getZkStatus()` instead of the list.
Also fixed refGuide text, which still said that Solr supports dynamic reconfig.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy merged pull request #1392: SOLR-14371 Zk
StatusHandler should know about dynamic zk config
Posted by GitBox <gi...@apache.org>.
janhoy merged pull request #1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
URL: https://github.com/apache/lucene-solr/pull/1392
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1392:
SOLR-14371 Zk StatusHandler should know about dynamic zk config
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
URL: https://github.com/apache/lucene-solr/pull/1392#discussion_r401835111
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ZookeeperStatusHandler.java
##########
@@ -108,27 +128,30 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
if ("true".equals(String.valueOf(stat.get("ok")))) {
numOk++;
}
- String state = String.valueOf(stat.get("zk_server_state"));
+ String state = zk.role != null ? zk.role : String.valueOf(stat.get("zk_server_state"));
if ("follower".equals(state)) {
followers++;
} else if ("leader".equals(state)) {
leaders++;
reportedFollowers = Integer.parseInt(String.valueOf(stat.get("zk_followers")));
} else if ("standalone".equals(state)) {
standalone++;
+ } else if ("participant".equals(state)) {
+ // NOCOMMIT: What does participant mean vs leader or follower?
Review comment:
I confused server state with role. Seems like even in dynamic reconfig mode, the servers report state leader/follower, and can also have a role participant or observer.
The only thing I'm not 100% sure about is whether `zk_server_state` can have the value `observer` when not in dynamic reconfig mode, cause observer feature pre-dates dynamic reconfig I think. I put it in there as an OR check and count it as follower for now, but it might never get that value.
I do not generate any WARN/ERROR messages in UI based on number of observers vs participants, as it is allowed to scale down to 1 participant if you like. Not sure what happens if you have 2 participants, is that still "A Bad Thing™"? Today we warn if there is an even number of leader/follower, but our code does not yet know about observers. Perhaps we should count participants and WARN if they are even in number, or will ZK handle it gracefully so we don't need to warn?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] HoustonPutman commented on a change in pull request
#1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
URL: https://github.com/apache/lucene-solr/pull/1392#discussion_r401042877
##########
File path: solr/core/src/test/org/apache/solr/handler/admin/ZookeeperStatusHandlerTest.java
##########
@@ -47,8 +48,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.*;
Review comment:
precommit issue?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] HoustonPutman commented on a change in pull request
#1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #1392: SOLR-14371 Zk StatusHandler should know about dynamic zk config
URL: https://github.com/apache/lucene-solr/pull/1392#discussion_r400993317
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ZookeeperStatusHandler.java
##########
@@ -24,15 +24,12 @@
import java.io.Writer;
import java.lang.invoke.MethodHandles;
import java.net.Socket;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Locale;
-import java.util.Map;
+import java.nio.charset.StandardCharsets;
+import java.util.*;
Review comment:
The precommit may dislike this
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org