dsmiley commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1706383802
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##########
@@ -291,7 +297,7 @@ public void testHttpCspPerf() throws Exception {
// lookup)
assertLogCount(collectionClusterStateLogs, 1);
// 1 call to fetch entire cluster state from HttpCSP.fetchLiveNodes()
- assertLogCount(entireClusterStateLogs, 1);
+ assertLogCount(entireClusterStateLogs, 0);
Review Comment:
Awesome.
At this juncture, I recommend changes to this test because the way it works
now is awkward: Only have one LogListener for the "adminRequestLogs" (simple;
3 is more confusing). Consume the log messages in the test (don't merely
count) to assert that the log message is what it should be, ideally with a
substring check or a simple regex if you must. Consume & assert this after
each step that we know will emit a request: after client construction and once
more after the first "add" (doc). Your code comments here are then redundant
as the assertions will be more specific at the right times to be self
explanatory.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##########
@@ -100,7 +100,13 @@ public class CloudHttp2SolrClientTest extends
SolrCloudTestCase {
private static final Pattern PATTERN_WITHOUT_COLLECTION =
Pattern.compile(
"path=/admin/collections.*params=\\{[^}]*action=CLUSTERSTATUS"
- + "(?![^}]*collection=)[^}]*\\}");
+ + "(?=[^}]*includeAll=true)"
Review Comment:
let's do away with this regex; it's gotten out of control and should cause
us to re-think what we're doing (see my other comment)
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -84,6 +84,11 @@ public void init(List<String> solrUrls) throws Exception {
/** Create a SolrClient implementation that uses the specified Solr node URL
*/
protected abstract SolrClient getSolrClient(String baseUrl);
+ @Override
Review Comment:
minor: I meant only a code comment and not javadocs. Javadocs are to define
the purpose of the method; the ~first comment inside a method (or class when
appropriate) is for implementation notes/details like this
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -191,43 +234,23 @@ public void getClusterStatus(NamedList<Object> results)
}
String configName = clusterStateCollection.getConfigName();
collectionStatus.put("configName", configName);
- if (message.getBool("prs", false) &&
clusterStateCollection.isPerReplicaState()) {
+ if (solrParams.getBool("prs", false) &&
clusterStateCollection.isPerReplicaState()) {
PerReplicaStates prs = clusterStateCollection.getPerReplicaStates();
collectionStatus.put("PRS", prs);
}
collectionProps.add(name, collectionStatus);
}
- List<String> liveNodes =
-
zkStateReader.getZkClient().getChildren(ZkStateReader.LIVE_NODES_ZKNODE, null,
true);
-
- // now we need to walk the collectionProps tree to cross-check replica
state with live nodes
- crossCheckReplicaStateWithLiveNodes(liveNodes, collectionProps);
-
- NamedList<Object> clusterStatus = new SimpleOrderedMap<>();
- clusterStatus.add("collections", collectionProps);
-
- // read cluster properties
- Map<String, Object> clusterProps = zkStateReader.getClusterProperties();
- if (clusterProps != null && !clusterProps.isEmpty()) {
- clusterStatus.add("properties", clusterProps);
- }
-
// add the alias map too
Map<String, String> collectionAliasMap = aliases.getCollectionAliasMap();
// comma delim
if (!collectionAliasMap.isEmpty()) {
clusterStatus.add("aliases", collectionAliasMap);
Review Comment:
the concept of `includeAll=false` is that you're then specifying what
sections you want and only getting those, not other stuff. "aliases" is a
top-level response section that doesn't yet have a boolean to ask for it.
Wether it's cheap or not (what you imply?) shouldn't matter IMO.
--
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]
