You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "dsmiley (via GitHub)" <gi...@apache.org> on 2024/10/31 04:15:33 UTC
[PR] SOLR-17534 ClusterState.getCollectionNames [solr]
dsmiley opened a new pull request, #2826:
URL: https://github.com/apache/solr/pull/2826
https://issues.apache.org/jira/browse/SOLR-17534
This PR also calls ClusterState.size() in some places where one of the map methods were used as an intermediary. Added javadocs to that method as well, and indicated it's an approximation.
--
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
Re: [PR] SOLR-17534 ClusterState.getCollectionNames [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2826:
URL: https://github.com/apache/solr/pull/2826#issuecomment-2452352907
testTwoServers [has been unstable lately](https://ge.apache.org/scans/tests?search.rootProjectNames=solr-root&search.timeZoneId=America%2FNew_York&tests.container=org.apache.solr.client.solrj.TestLBHttp2SolrClient&tests.test=testTwoServers) (<-- linked from build failures, conveniently); certainly isn't this PR
After maybe flipping that Set.copyOf to a Set cast in the test, I will wait till Monday to merge. CHANGES.txt in Other section would be "Added ClusterState.getCollectionNames"
--
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
Re: [PR] SOLR-17534 ClusterState.getCollectionNames [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2826:
URL: https://github.com/apache/solr/pull/2826#discussion_r1826144232
##########
solr/core/src/test/org/apache/solr/cloud/OverseerTest.java:
##########
@@ -758,7 +758,7 @@ private void waitForCollections(ZkStateReader stateReader, String... collections
while (0 < maxIterations--) {
final ClusterState state = stateReader.getClusterState();
- Set<String> availableCollections = state.getCollectionsMap().keySet();
+ Set<String> availableCollections = Set.copyOf(state.getCollectionNames());
Review Comment:
state.getCollectionNames returns a Collection that just-so-happens to be a Set but may end up being a List in the future. I didn't want to choose a Set on the first introduction of this method unless we think this choice is stable.
Since this spot here is merely a test, maybe I'll simply cast it to a Set. If one day it becomes a List or something... whatever; easy enough to switch to Set.copyOf. I wish the JDK had a convenient option to cast-or-create. Didn't seem worth it in this class to hand-role a few lines of code for it.
--
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
Re: [PR] SOLR-17534 ClusterState.getCollectionNames [solr]
Posted by "iamsanjay (via GitHub)" <gi...@apache.org>.
iamsanjay commented on PR #2826:
URL: https://github.com/apache/solr/pull/2826#issuecomment-2452367694
> testTwoServers [has been unstable lately](https://ge.apache.org/scans/tests?search.rootProjectNames=solr-root&search.timeZoneId=America%2FNew_York&tests.container=org.apache.solr.client.solrj.TestLBHttp2SolrClient&tests.test=testTwoServers) (<-- linked from build failures, conveniently); certainly isn't this PR
Really want to disable this one and another one from the same class (or, find better solution). They failed so frequently that It wastes lot of time. (May be discussion for dev-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.
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
Re: [PR] SOLR-17534 ClusterState.getCollectionNames [solr]
Posted by "aparnasuresh85 (via GitHub)" <gi...@apache.org>.
aparnasuresh85 commented on code in PR #2826:
URL: https://github.com/apache/solr/pull/2826#discussion_r1826055397
##########
solr/core/src/test/org/apache/solr/cloud/OverseerTest.java:
##########
@@ -758,7 +758,7 @@ private void waitForCollections(ZkStateReader stateReader, String... collections
while (0 < maxIterations--) {
final ClusterState state = stateReader.getClusterState();
- Set<String> availableCollections = state.getCollectionsMap().keySet();
+ Set<String> availableCollections = Set.copyOf(state.getCollectionNames());
Review Comment:
Minor: `state.getCollectionNames()` might already return an unmodifiable view of keys from `immutableCollectionStates`. Is the `Set.copyOf()` necessary?
--
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
Re: [PR] SOLR-17534 ClusterState.getCollectionNames [solr]
Posted by "aparnasuresh85 (via GitHub)" <gi...@apache.org>.
aparnasuresh85 commented on PR #2826:
URL: https://github.com/apache/solr/pull/2826#issuecomment-2452216429
+1. Nice and simple. There is a failing test that needs to be addressed: TestLBHttp2SolrClient.testTwoServers
--
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
Re: [PR] SOLR-17534 ClusterState.getCollectionNames [solr]
Posted by "aparnasuresh85 (via GitHub)" <gi...@apache.org>.
aparnasuresh85 commented on code in PR #2826:
URL: https://github.com/apache/solr/pull/2826#discussion_r1826055397
##########
solr/core/src/test/org/apache/solr/cloud/OverseerTest.java:
##########
@@ -758,7 +758,7 @@ private void waitForCollections(ZkStateReader stateReader, String... collections
while (0 < maxIterations--) {
final ClusterState state = stateReader.getClusterState();
- Set<String> availableCollections = state.getCollectionsMap().keySet();
+ Set<String> availableCollections = Set.copyOf(state.getCollectionNames());
Review Comment:
`state.getCollectionNames()` might already return an unmodifiable view of keys from `immutableCollectionStates`. Is the `Set.copyOf()` necessary?
--
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
Re: [PR] SOLR-17534 ClusterState.getCollectionNames [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #2826:
URL: https://github.com/apache/solr/pull/2826
--
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