dsmiley commented on code in PR #3623:
URL: https://github.com/apache/solr/pull/3623#discussion_r2327904358
##########
solr/core/src/resources/ImplicitPlugins.json:
##########
Review Comment:
We'll need to remember to document the loss of these endpoints/handlers
##########
solr/webapp/web/js/angular/controllers/plugins.js:
##########
Review Comment:
Great job nonetheless!
I do see lots of repetition of the labels. I wonder if we should just
remove uninteresting labels, or put at the top of the screen the labels we know
that are going to be common if we're looking at a specific core.
##########
solr/core/src/test/org/apache/solr/cloud/TestPullReplica.java:
##########
@@ -311,16 +311,24 @@ public void testAddDocs() throws Exception {
waitForNumDocsInAllReplicas(numDocs, pullReplicas);
for (Replica r : pullReplicas) {
Review Comment:
this test seems a bit brittle... since it asserts null when maybe the
condition/filter changes and is always null for other reasons. I think it'd be
better to simply iterate over all jetty's, iterate all cores, extract the
metric of interest, then assert that it's either null or not null depending on
the type of replica.
##########
solr/core/src/test/org/apache/solr/cloud/TestPullReplicaWithAuth.java:
##########
@@ -105,18 +103,35 @@ public void testPKIAuthWorksForPullReplication() throws
Exception {
numDocs, pullReplicas, "*:*", SecurityJson.USER, SecurityJson.PASS);
for (Replica r : pullReplicas) {
- try (SolrClient pullReplicaClient = getHttpSolrClient(r)) {
- QueryResponse statsResponse =
- queryWithBasicAuth(
- pullReplicaClient, new SolrQuery("qt", "/admin/plugins",
"stats", "true"));
+ JettySolrRunner jetty =
+ cluster.getJettySolrRunners().stream()
+ .filter(j -> j.getBaseUrl().toString().equals(r.getBaseUrl()))
+ .findFirst()
+ .orElse(null);
+ assertNotNull("Could not find jetty for replica " + r, jetty);
+
+ try (SolrCore core =
jetty.getCoreContainer().getCore(r.getCoreName())) {
+ // Check that adds gauge metric is null/0 for pull replicas
+ var addOpsDatapoint =
+ SolrMetricTestUtils.getCounterDatapoint(
+ core,
+ "solr_core_update_committed_ops",
+ SolrMetricTestUtils.newCloudLabelsBuilder(core)
+ .label("category", "UPDATE")
+ .label("ops", "adds")
+ .build());
// the 'adds' metric is a gauge, which is null for PULL replicas
+ assertNull("Replicas shouldn't process the add document request",
addOpsDatapoint);
+ var cumulativeAddsDatapoint =
+ SolrMetricTestUtils.getGaugeDatapoint(
+ core,
+ "solr_core_update_cumulative_ops",
+ SolrMetricTestUtils.newCloudLabelsBuilder(core)
+ .label("category", "UPDATE")
+ .label("ops", "adds")
+ .build());
assertNull(
Review Comment:
same feedback applies
##########
solr/core/src/test/org/apache/solr/cloud/TestPullReplica.java:
##########
@@ -311,16 +311,24 @@ public void testAddDocs() throws Exception {
waitForNumDocsInAllReplicas(numDocs, pullReplicas);
for (Replica r : pullReplicas) {
- try (SolrClient pullReplicaClient = getHttpSolrClient(r)) {
- SolrQuery req = new SolrQuery("qt", "/admin/plugins", "stats",
"true");
- QueryResponse statsResponse = pullReplicaClient.query(req);
+ JettySolrRunner jetty =
+ cluster.getJettySolrRunners().stream()
+ .filter(j -> j.getBaseUrl().toString().equals(r.getBaseUrl()))
+ .findFirst()
+ .orElse(null);
Review Comment:
I suppose ideally the cluster get could the jetty by Url (a convenience
method) but I dunno how often that need is
--
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]
