janhoy commented on code in PR #3908:
URL: https://github.com/apache/solr/pull/3908#discussion_r2586497596
##########
solr/core/src/java/org/apache/solr/response/PrometheusResponseWriter.java:
##########
@@ -41,7 +43,26 @@ public void write(
OutputStream out, SolrQueryRequest request, SolrQueryResponse response,
String contentType)
throws IOException {
+ // Check if we have pre-formatted Prometheus text (from single-node proxy)
+ if (response.getValues().get("stream") instanceof InputStream stream) {
+ try {
+ stream.transferTo(out);
+ } finally {
+ stream.close();
+ }
+ return;
+ }
+
+ if (response.getException() != null) {
+
out.write(response.getException().toString().getBytes(StandardCharsets.UTF_8));
+ return;
Review Comment:
Well spotted
##########
changelog/unreleased/SOLR-18004-frontend-parse-prometheus.yml:
##########
@@ -4,6 +4,7 @@ type: fixed # added, changed, fixed, deprecated, removed,
dependency_update, sec
authors:
- name: Jan Høydahl
url: https://home.apache.org/phonebook.html?uid=janhoy
+ - name: David Smiley
Review Comment:
Satisfying to see logchange in actuve use, with no risk of merge conflicts!
##########
solr/core/src/java/org/apache/solr/handler/admin/AdminHandlersProxy.java:
##########
@@ -136,8 +125,15 @@ private static void handleNamedListFormat(
/** Makes a remote request asynchronously. */
public static CompletableFuture<NamedList<Object>> callRemoteNode(
- String nodeName, String uriPath, SolrParams params, ZkController
zkController)
- throws IOException, SolrServerException {
+ String nodeName, String uriPath, SolrParams params, ZkController
zkController) {
+
+ // Validate that the node exists in the cluster
+ if
(!zkController.zkStateReader.getClusterState().getLiveNodes().contains(nodeName))
{
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "Requested node " + nodeName + " is not part of cluster");
Review Comment:
Agree that this may be a safer place to check that the node is actually part
of the cluster instead of earlier.
When moving from `nodes` to `node` parameter for prometheus, it was an
omission of me to not add a liveNodes check since we no longer call
`resolveNodes`.
The only downside I can see from this change is with other endpoints, e.g. a
call `/admin/info/system?nodes=foo:8983_solr,bad_node`, then earlier we'd fail
the request before proxying at all, while now we'll send the request to
`foo:8983`, while `bad_node` will throw exception and likely fail the entire
request eventually.
Being a corner case and no real harm, I think this is acceptable.
--
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]
