Re: [PR] [SOLR-17837] omit PULL nodes from preferredLeader prop distribution [solr]
HoustonPutman merged PR #3451: URL: https://github.com/apache/solr/pull/3451 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [SOLR-17837] omit PULL nodes from preferredLeader prop distribution [solr]
liangkaiwen commented on PR #3451: URL: https://github.com/apache/solr/pull/3451#issuecomment-3293873261 The test passes on my local machine, but agreed it is quite slow (18.4s to completion). As for the replica numbers, the number of tlog nodes is equivalent to the max possible of tlog nodes(4) in the test prior to my change with the only change there being removing the random factor. For the pull nodes, the previous value before my change was a globally hard-coded 2, however I bumped this up to 4 so that there is more likelihood the test will catch any issue where atleast 1 pull node gets assigned leader incorrectly. It was noted by a colleague of mine that the test base class here is outdated and might potentially be contributing to the flakiness? If it would be better, I could also reduce the number of replicas to 2 tlog and 2 pull? Let me know if you would like me to proceed with either or both of these ideas -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [SOLR-17837] omit PULL nodes from preferredLeader prop distribution [solr]
HoustonPutman commented on PR #3451: URL: https://github.com/apache/solr/pull/3451#issuecomment-3293347007 Yeah, that's right. Though it looks like the cloud clients dead server checker can't be killed for some reason. Not sure why this would do that. But @liangkaiwen what's the reason for adding so many replicas to the test? It seems like it is slowing everything down a good amount. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [SOLR-17837] omit PULL nodes from preferredLeader prop distribution [solr]
iamsanjay commented on PR #3451: URL: https://github.com/apache/solr/pull/3451#issuecomment-3290847327 This test failure might be related to this commit, https://develocity.apache.org/scans/tests?search.rootProjectNames=solr-root&search.timeZoneId=Asia%2FCalcutta&tests.container=org.apache.solr.cloud.api.collections.TestReplicaProperties ``` - org.apache.solr.cloud.api.collections.TestReplicaProperties (:solr:core) Test history: https://develocity.apache.org/scans/tests?search.rootProjectNames=solr-root&tests.container=org.apache.solr.cloud.api.collections.TestReplicaProperties Test output: /home/jenkins/jenkins-agent/workspace/Solr/Solr-Test-main/solr/core/build/test-results/test/outputs/OUTPUT-org.apache.solr.cloud.api.collections.TestReplicaProperties.txt Reproduce with: ./gradlew :solr:core:test --tests "org.apache.solr.cloud.api.collections.TestReplicaProperties" -Ptests.haltonfailure=false "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC -XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=62B45FC2B6ED55E3 -Ptests.multiplier=2 -Ptests.useSecurityManager=true -Ptests.badapples=false -Ptests.file.encoding=US-ASCII ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [SOLR-17837] omit PULL nodes from preferredLeader prop distribution [solr]
liangkaiwen commented on PR #3451: URL: https://github.com/apache/solr/pull/3451#issuecomment-3272068468 @HoustonPutman can this be merged? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [SOLR-17837] omit PULL nodes from preferredLeader prop distribution [solr]
liangkaiwen commented on PR #3451: URL: https://github.com/apache/solr/pull/3451#issuecomment-3177809759 It has been noted by one of my teammates that `AbstractFullDistribZkTestBase` has been replaced in favor of `SolrCloudTestCase` and sometimes thread leaks can occur @HoustonPutman do you think it is worth rewriting the test class entirely or can we get this change in even with the sporadic test failures? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [SOLR-17837] omit PULL nodes from preferredLeader prop distribution [solr]
liangkaiwen commented on PR #3451: URL: https://github.com/apache/solr/pull/3451#issuecomment-3177785048 It's unclear why this test is failing (looks like the test cloud is somehow repeatedly unable to elect a leader within 4s). Running the same tests locally on my own machine do not yield the same failure -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [SOLR-17837] omit PULL nodes from preferredLeader prop distribution [solr]
liangkaiwen commented on code in PR #3451: URL: https://github.com/apache/solr/pull/3451#discussion_r2260382119 ## solr/CHANGES.txt: ## @@ -4,6 +4,27 @@ This file lists Solr's raw release notes with details of every change to Solr. Most people will find the solr-upgrade-notes.adoc file more approachable. https://github.com/apache/solr/blob/main/solr/solr-ref-guide/modules/upgrade-notes/pages/solr-upgrade-notes.adoc +== 9.10.0 == + +New Features +- + +Improvements +- + +Optimizations +- + +Bug Fixes +- +* SOLR-17837: Disallow PULL replicas from being counted/marked for "preferredLeader" property by BALANCESHARDUNIQUE + +Dependency Upgrades +- + +Other Changes +- + Review Comment: Oops, my main branch was out of date. Updated -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [SOLR-17837] omit PULL nodes from preferredLeader prop distribution [solr]
HoustonPutman commented on code in PR #3451: URL: https://github.com/apache/solr/pull/3451#discussion_r2258520424 ## solr/CHANGES.txt: ## @@ -4,6 +4,27 @@ This file lists Solr's raw release notes with details of every change to Solr. Most people will find the solr-upgrade-notes.adoc file more approachable. https://github.com/apache/solr/blob/main/solr/solr-ref-guide/modules/upgrade-notes/pages/solr-upgrade-notes.adoc +== 9.10.0 == + +New Features +- + +Improvements +- + +Optimizations +- + +Bug Fixes +- +* SOLR-17837: Disallow PULL replicas from being counted/marked for "preferredLeader" property by BALANCESHARDUNIQUE + +Dependency Upgrades +- + +Other Changes +- + Review Comment: The section already exists below 10.0.0. Just add your line there. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [SOLR-17837] omit PULL nodes from preferredLeader prop distribution [solr]
liangkaiwen commented on PR #3451: URL: https://github.com/apache/solr/pull/3451#issuecomment-3161757574 > There's an easier way to check if a replica can be leader. But otherwise, this looks like a good change. Can you make a Changelog entry as well for 9.10? Done. Let me know if I need to fix the format of the changelog -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [SOLR-17837] omit PULL nodes from preferredLeader prop distribution [solr]
liangkaiwen commented on PR #3451: URL: https://github.com/apache/solr/pull/3451#issuecomment-3156775388 Currently working on tests. Getting a thread leak in the test whenever I add pull replicas, but not sure why :/ -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [SOLR-17837] omit PULL nodes from preferredLeader prop distribution [solr]
HoustonPutman commented on code in PR #3451:
URL: https://github.com/apache/solr/pull/3451#discussion_r2255246575
##
solr/core/src/java/org/apache/solr/cloud/ExclusiveSliceProperty.java:
##
@@ -129,6 +129,10 @@ private boolean isActive(Replica replica) {
return replica.getState() == Replica.State.ACTIVE;
}
+ private boolean canBeLeader(Replica replica) {
+return replica.getType() == Replica.Type.NRT || replica.getType() ==
Replica.Type.TLOG;
+ }
+
Review Comment:
```suggestion
```
##
solr/core/src/java/org/apache/solr/cloud/ExclusiveSliceProperty.java:
##
@@ -151,6 +155,12 @@ private boolean collectCurrentPropStats() {
}
continue;
}
+if (SliceMutator.PREFERRED_LEADER_PROP.equals(property)
+&& !canBeLeader(
+replica)) { // omit replicas that cannot potentially be leader
from preferredLeader
Review Comment:
```suggestion
// omit replicas that cannot potentially be leader from
preferredLeader
&& replica.getType().leaderEligible) {
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
