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/07/03 22:31:04 UTC
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1644: SOLR-14623: Minor Improvements to SolrCloud tests
tflobbe commented on a change in pull request #1644:
URL: https://github.com/apache/lucene-solr/pull/1644#discussion_r449712037
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java
##########
@@ -207,18 +208,38 @@ private void addDocToWhenOtherReplicasAreDown(String collection, Replica leader,
* Leader should be on node - 0
*/
private void addDocWhenOtherReplicasAreNetworkPartitioned(String collection, Replica leader, int docId) throws Exception {
- for (int i = 0; i < 3; i++) {
- proxies.get(cluster.getJettySolrRunner(i)).close();
+ DocCollection col = cluster.getSolrClient().getZkStateReader().getClusterState().getCollection(collection);
+ Replica shard1Leader = col.getLeader("shard1");
+ String baseUrl = shard1Leader.getBaseUrl();
+ JettySolrRunner j1 = null; // This will be the jetty hosting shard1's leader
+ for (JettySolrRunner j : cluster.getJettySolrRunners()) {
+ System.out.println("cmp:" + j.getProxyBaseUrl() + " " + baseUrl);
+ if (j.getProxyBaseUrl().toString().equals(baseUrl)) {
+ j1 = j;
+ break;
+ }
+ }
+
+ assertNotNull(baseUrl, j1);
Review comment:
Maybe you can improve this message? A failure would look confusing
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java
##########
@@ -207,18 +208,38 @@ private void addDocToWhenOtherReplicasAreDown(String collection, Replica leader,
* Leader should be on node - 0
*/
private void addDocWhenOtherReplicasAreNetworkPartitioned(String collection, Replica leader, int docId) throws Exception {
- for (int i = 0; i < 3; i++) {
- proxies.get(cluster.getJettySolrRunner(i)).close();
+ DocCollection col = cluster.getSolrClient().getZkStateReader().getClusterState().getCollection(collection);
+ Replica shard1Leader = col.getLeader("shard1");
+ String baseUrl = shard1Leader.getBaseUrl();
+ JettySolrRunner j1 = null; // This will be the jetty hosting shard1's leader
+ for (JettySolrRunner j : cluster.getJettySolrRunners()) {
+ System.out.println("cmp:" + j.getProxyBaseUrl() + " " + baseUrl);
+ if (j.getProxyBaseUrl().toString().equals(baseUrl)) {
+ j1 = j;
+ break;
+ }
+ }
+
+ assertNotNull(baseUrl, j1);
+
+ for (JettySolrRunner j : cluster.getJettySolrRunners()) {
+ if (j != j1) {
+ proxies.get(j).close();
+ }
}
- addDoc(collection, docId, cluster.getJettySolrRunner(0));
- JettySolrRunner j1 = cluster.getJettySolrRunner(0);
+
+ addDoc(collection, docId, j1);
+
j1.stop();
cluster.waitForJettyToStop(j1);
- for (int i = 1; i < 3; i++) {
- proxies.get(cluster.getJettySolrRunner(i)).reopen();
+ for (JettySolrRunner j : cluster.getJettySolrRunners()) {
+ if (j != j1) {
+ proxies.get(j).reopen();
+ }
}
waitForState("Timeout waiting for leader goes DOWN", collection, (liveNodes, collectionState)
- -> collectionState.getReplica(leader.getName()).getState() == Replica.State.DOWN);
+ -> collectionState.getReplica(leader.getName()).getState() == Replica.State.DOWN);
+ Thread.sleep(1000);
Review comment:
Is this sleep here to wait for the stop? Why is it now needed?
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java
##########
@@ -207,18 +208,38 @@ private void addDocToWhenOtherReplicasAreDown(String collection, Replica leader,
* Leader should be on node - 0
*/
private void addDocWhenOtherReplicasAreNetworkPartitioned(String collection, Replica leader, int docId) throws Exception {
- for (int i = 0; i < 3; i++) {
- proxies.get(cluster.getJettySolrRunner(i)).close();
+ DocCollection col = cluster.getSolrClient().getZkStateReader().getClusterState().getCollection(collection);
+ Replica shard1Leader = col.getLeader("shard1");
+ String baseUrl = shard1Leader.getBaseUrl();
+ JettySolrRunner j1 = null; // This will be the jetty hosting shard1's leader
+ for (JettySolrRunner j : cluster.getJettySolrRunners()) {
+ System.out.println("cmp:" + j.getProxyBaseUrl() + " " + baseUrl);
Review comment:
Is this intentionally left here?
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java
##########
@@ -207,18 +208,38 @@ private void addDocToWhenOtherReplicasAreDown(String collection, Replica leader,
* Leader should be on node - 0
Review comment:
I guess this is no longer true?
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java
##########
@@ -207,18 +208,38 @@ private void addDocToWhenOtherReplicasAreDown(String collection, Replica leader,
* Leader should be on node - 0
*/
private void addDocWhenOtherReplicasAreNetworkPartitioned(String collection, Replica leader, int docId) throws Exception {
- for (int i = 0; i < 3; i++) {
- proxies.get(cluster.getJettySolrRunner(i)).close();
+ DocCollection col = cluster.getSolrClient().getZkStateReader().getClusterState().getCollection(collection);
+ Replica shard1Leader = col.getLeader("shard1");
+ String baseUrl = shard1Leader.getBaseUrl();
+ JettySolrRunner j1 = null; // This will be the jetty hosting shard1's leader
Review comment:
Maybe `s/j1/leaderJetty/` or something?
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java
##########
@@ -207,18 +208,38 @@ private void addDocToWhenOtherReplicasAreDown(String collection, Replica leader,
* Leader should be on node - 0
*/
private void addDocWhenOtherReplicasAreNetworkPartitioned(String collection, Replica leader, int docId) throws Exception {
- for (int i = 0; i < 3; i++) {
- proxies.get(cluster.getJettySolrRunner(i)).close();
+ DocCollection col = cluster.getSolrClient().getZkStateReader().getClusterState().getCollection(collection);
+ Replica shard1Leader = col.getLeader("shard1");
+ String baseUrl = shard1Leader.getBaseUrl();
+ JettySolrRunner j1 = null; // This will be the jetty hosting shard1's leader
+ for (JettySolrRunner j : cluster.getJettySolrRunners()) {
+ System.out.println("cmp:" + j.getProxyBaseUrl() + " " + baseUrl);
+ if (j.getProxyBaseUrl().toString().equals(baseUrl)) {
+ j1 = j;
+ break;
+ }
+ }
+
+ assertNotNull(baseUrl, j1);
+
+ for (JettySolrRunner j : cluster.getJettySolrRunners()) {
+ if (j != j1) {
+ proxies.get(j).close();
+ }
}
- addDoc(collection, docId, cluster.getJettySolrRunner(0));
- JettySolrRunner j1 = cluster.getJettySolrRunner(0);
+
+ addDoc(collection, docId, j1);
+
j1.stop();
cluster.waitForJettyToStop(j1);
- for (int i = 1; i < 3; i++) {
- proxies.get(cluster.getJettySolrRunner(i)).reopen();
+ for (JettySolrRunner j : cluster.getJettySolrRunners()) {
+ if (j != j1) {
+ proxies.get(j).reopen();
+ }
}
waitForState("Timeout waiting for leader goes DOWN", collection, (liveNodes, collectionState)
- -> collectionState.getReplica(leader.getName()).getState() == Replica.State.DOWN);
+ -> collectionState.getReplica(leader.getName()).getState() == Replica.State.DOWN);
+ Thread.sleep(1000);
// the meat of the test -- wait to see if a different replica become a leader
// the correct behavior is that this should time out, if it succeeds we have a problem...
Review comment:
Suggestion, if the expected behavior is the timeout here (no leader elected), WDYT about waiting for less time in normal tests (1second maybe?) and only doing 10 seconds on nightly?
----------------------------------------------------------------
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