nick-boss-tech commented on PR #5001:
URL: https://github.com/apache/solr/pull/5001#issuecomment-5958833218
Small correction and two notes on the latest push (6a30021):
The metrics wording in my previous comment and in the code was too broad.
The metrics decision in this PR is only about standalone mode. In SolrCloud,
`node=all` fans out to all live nodes via the shared validator, and that
behavior is unchanged. I reworded the comment in
`MetricsHandler.createMetricProxy` and the PR description to say exactly that.
Whether SolrCloud `node=all` for metrics should also change is a separate
question I have not touched here; happy to open it separately if you think it
is wrong.
On JS tests for the new UI branch: the legacy AngularJS Admin UI has no
unit-test harness in the repo (no karma/jasmine specs under solr/webapp), so
there is no existing suite to extend for the cloud/standalone/unresolved
branches. The controller change follows the exact `$watch('isCloudEnabled')`
pattern already used by paramsets.js and query.js rather than introducing a new
mechanism, but I agree the branch is untested at the JS level, and I did not
want to bolt a new harness onto this PR to cover one controller.
On verification: the runs I quoted were local. CI has since started on this
head: "gradle check", "Run Solr Tests using Crave.io resources" and "Run Admin
UI browser tests" are in progress, and the changelog and labeler checks have
passed.
--
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]
