SOLR-18113: Revamp ZookeeperInfoHandler by epugh · Pull Request #4124 · apache/solr

epugh · 2026-02-09T14:23:51Z

https://issues.apache.org/jira/browse/SOLR-18113

Description

Bit of revamp, see the JIRA.

Solution

TBD

Tests

Adding more tests

Now I see why we would want request handlers to inject their own request writers ;-)

…ameter

Copilot

Pull request overview

Revamps Solr’s /admin/zookeeper handler behavior to better support the Admin UI “Cloud -> Graph” view and adds initial SolrCloud tests around the endpoint.

Changes:

Update the Admin UI Angular Zookeeper service to stop requesting the removed /clusterstate.json pseudo-path.
Adjust ZookeeperInfoHandler to produce structured response data (rather than returning a raw ContentStream) and tweak paging/graph behavior.
Register wt=raw as a built-in response writer and add new SolrCloud tests for /admin/zookeeper.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
solr/webapp/web/js/angular/services.js	Align UI ZK calls with the handler’s updated graph/paging behavior (no `/clusterstate.json` path).
solr/core/src/test/org/apache/solr/handler/admin/ZookeeperInfoHandlerTest.java	Adds basic SolrCloud tests validating `/admin/zookeeper` responses for detail and graph views.
solr/core/src/java/org/apache/solr/response/ResponseWritersRegistry.java	Adds built-in `raw` response writer support for container-level handlers.
solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java	Switches from raw ContentStream responses to parsed/structured response values and adjusts graph pagination logic.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java

Copilot · 2026-02-09T14:34:50Z

solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java

+    // Force JSON response and omit header for cleaner output
+    Map<String, String> map = Map.of(WT, "json", OMIT_HEADER, "true");


The comment "allows any response writer (json, xml, etc.)" conflicts with the earlier wrapDefaults(new MapSolrParams(map), params) call, which forces wt=json because the forced params are the primary params. Either update the comment to reflect that JSON is enforced, or stop forcing wt if other writers are intended to work.

Suggested change

// Force JSON response and omit header for cleaner output

Map<String, String> map = Map.of(WT, "json", OMIT_HEADER, "true");

// Force omitting the response header for cleaner output; allow client to choose response writer

Map<String, String> map = Map.of(OMIT_HEADER, "true");

…request..

…, but have the same .print() method

epugh · 2026-02-09T20:40:24Z

I created a large number of collections using this script. Note, it would fail around collection 60 or so!
create_collections.py

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-09T20:44:13Z

solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java

+   * Handles the graph view request with paginated collections.
+   *
+   * @param params Request parameters including pagination settings
+   * @return JSON string representing paginated collection data


Javadoc mismatch: handleGraphViewRequest returns a ZkBasePrinter, but the @return description says it returns a JSON string. Please update the Javadoc to reflect the actual return type/behavior.

Suggested change

* @return JSON string representing paginated collection data

* @return a {@link ZkBasePrinter} that will render the graph view for the requested page of collections

Copilot · 2026-02-09T20:44:14Z

solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java

+   * Handles the path view request for a specific ZooKeeper path.
+   *
+   * @param params Request parameters including the path to display
+   * @return JSON string representing the ZooKeeper path data


Javadoc mismatch: handlePathViewRequest returns a ZkBasePrinter, but the @return description says it returns a JSON string. Please update the Javadoc to reflect the actual return type/behavior.

Suggested change

* @return JSON string representing the ZooKeeper path data

* @return a ZkBasePrinter that writes the ZooKeeper path data

Copilot · 2026-02-09T20:44:14Z

solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java

+    validateParameters(params);
+
+    // Determine request type and handle accordingly
+    boolean isGraphView = "graph".equals(params.get("view"));


handleRequestBody now treats any request with view=graph as a graph request and ignores PATH entirely. Previously, graph-mode pagination was only enabled for the legacy "/clusterstate.json" pseudo-path; other paths could still be viewed even if view=graph was present. If this is intentional, consider validating/rejecting incompatible params (e.g., view=graph with a non-empty PATH) or documenting the precedence to avoid surprising API consumers.

Suggested change

boolean isGraphView = "graph".equals(params.get("view"));

boolean isGraphView = "graph".equals(params.get("view"));

String path = params.get(PATH);

if (isGraphView

&& StringUtils.isNotEmpty(path)

&& !"/clusterstate.json".equals(path)) {

throw new SolrException(

ErrorCode.BAD_REQUEST,

"Parameter combination not supported: view=graph cannot be used with PATH='"

+ path

+ "'. Use an empty PATH or '/clusterstate.json' with view=graph.");

}

solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java

Copilot · 2026-02-09T20:44:15Z

solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java

      this.zkController = controller;
-      keeperAddr = controller.getZkServerAddress();
-      zkClient = controller.getZkClient();
+      this.detail = detail;
+      this.dump = dump;
+      this.keeperAddr = controller.getZkServerAddress();
+      this.zkClient = controller.getZkClient();


ZkBasePrinter dereferences the passed ZkController without a null check. CoreContainer registers this handler even when not ZooKeeper-aware, so /admin/zookeeper in standalone mode can trigger a NullPointerException here. Please guard against cores.getZkController()==null (e.g., throw a BAD_REQUEST/SERVICE_UNAVAILABLE with a clear message) or make ZkBasePrinter tolerate a null controller and emit an error response.

this guy doesnt't make sense in standalone, so it's okay.

Copilot · 2026-02-09T20:44:15Z

solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java

+    // start with json.startObject() and end with json.endObject()
+    @SuppressWarnings("unchecked")
+    Map<String, Object> jsonMap = (Map<String, Object>) Utils.fromJSONString(jsonString);

-    try {
-      if (paginateCollections) {
-        // List collections and allow pagination, but no specific znode info like when looking at a
-        // normal ZK path
-        printer.printPaginatedCollections();
-      } else {
-        printer.print(path);
-      }
-    } finally {
-      printer.close();
+    for (Map.Entry<String, Object> entry : jsonMap.entrySet()) {


This implementation stringifies JSON in the printer and then immediately parses it back into a Map (Utils.fromJSONString) before writing the response. That round-trip adds CPU + heap overhead proportional to response size (potentially large for deep ZK paths). Consider building structured objects directly (Map/NamedList) and adding them to rsp, or keep streaming output, to avoid the stringify->parse overhead.

Copilot is so right here. "Raw" was being used to avoid this very thing. Let's not serialize & decode needlessly!

Another way to avoid this is a potentially large refactor of ZkBasePrinter to instead either produce plain Maps & arrays & such that Solr will serialize (to either Json or Xml even). I think you/LLM will find that easiest to understand. Or embrace Solr's MapWriter that allows very efficient just-in-time streaming. See https://issues.apache.org/jira/browse/SOLR-17582 #2916 for an example of that.

…Handler.java Co-authored-by: Copilot <[email protected]>

epugh · 2026-02-11T14:53:42Z

This bug in main is due to ZKPrinter not being Public and how reflection works:

However, this PR changes things around and avoids the bug... I can fix the bug, but I'd rather just merge this PR!

dsmiley

Whatever becomes of ZookeeperInfoHandler, I think we should continue to keep "raw" an option at the node level. ZIH was maybe a hacky case since it returns JSON.

dsmiley · 2026-02-11T21:19:31Z

solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java

+    // start with json.startObject() and end with json.endObject()
+    @SuppressWarnings("unchecked")
+    Map<String, Object> jsonMap = (Map<String, Object>) Utils.fromJSONString(jsonString);

-    try {
-      if (paginateCollections) {
-        // List collections and allow pagination, but no specific znode info like when looking at a
-        // normal ZK path
-        printer.printPaginatedCollections();
-      } else {
-        printer.print(path);
-      }
-    } finally {
-      printer.close();
+    for (Map.Entry<String, Object> entry : jsonMap.entrySet()) {


Copilot is so right here. "Raw" was being used to avoid this very thing. Let's not serialize & decode needlessly!

Another way to avoid this is a potentially large refactor of ZkBasePrinter to instead either produce plain Maps & arrays & such that Solr will serialize (to either Json or Xml even). I think you/LLM will find that easiest to understand. Or embrace Solr's MapWriter that allows very efficient just-in-time streaming. See https://issues.apache.org/jira/browse/SOLR-17582 #2916 for an example of that.

epugh added 5 commits February 9, 2026 07:45

Raw is used by /solr/admin/zookeeper ZookeeperInfoHandler endpoint

6fa37b7

Now I see why we would want request handlers to inject their own request writers ;-)

Add basic tests

0fd3316

migrate away from ContentStream, eliminate clusterstate.json as a par…

455850b

…ameter

Update test now that we don't need this param from services.js

20b3ff0

Code cleanups

97b0627

epugh requested a review from Copilot February 9, 2026 14:23

github-actions bot added admin-ui tests cat:search labels Feb 9, 2026

Copilot started reviewing on behalf of epugh February 9, 2026 14:24 View session

Copilot AI reviewed Feb 9, 2026

View reviewed changes

no longer need raw

1a935f9

github-actions bot removed the cat:search label Feb 9, 2026

epugh added 5 commits February 9, 2026 13:09

Small UI fixes for pagination

804f90e

bit more whitespace.

d7b1f0d

Refactor the code to be clearer on when doing a graph or a file path …

1cd94fd

…request..

Code cleanups and rename print() to printPath() as thats what it does.

6906f42

refactor the ZKPrinter into two subclasses that do their specific job…

0ce7890

…, but have the same .print() method

epugh marked this pull request as ready for review February 9, 2026 20:33

epugh requested a review from Copilot February 9, 2026 20:33

Copilot started reviewing on behalf of epugh February 9, 2026 20:34 View session

doc the change

2fd1efe

Copilot AI reviewed Feb 9, 2026

View reviewed changes

epugh and others added 3 commits February 9, 2026 15:52

We need raw at the core level for the ShowFileHandler.

39d5c30

Merge branch 'main' into SOLR-18113

4925672

Update solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfo…

92d7a3f

…Handler.java Co-authored-by: Copilot <[email protected]>

epugh requested review from dsmiley and janhoy February 11, 2026 14:52

dsmiley reviewed Feb 11, 2026

View reviewed changes

		// Force JSON response and omit header for cleaner output
		Map<String, String> map = Map.of(WT, "json", OMIT_HEADER, "true");

	* @return JSON string representing paginated collection data
	* @return a {@link ZkBasePrinter} that will render the graph view for the requested page of collections

	* @return JSON string representing the ZooKeeper path data
	* @return a ZkBasePrinter that writes the ZooKeeper path data

-    boolean isGraphView = "graph".equals(params.get("view"));
+    boolean isGraphView = "graph".equals(params.get("view"));
+    String path = params.get(PATH);
+    if (isGraphView
+        && StringUtils.isNotEmpty(path)
+        && !"/clusterstate.json".equals(path)) {
+      throw new SolrException(
+          ErrorCode.BAD_REQUEST,
+          "Parameter combination not supported: view=graph cannot be used with PATH='"
+              + path
+              + "'. Use an empty PATH or '/clusterstate.json' with view=graph.");
+    }

Conversation

epugh commented Feb 9, 2026

Description

Solution

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

epugh commented Feb 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

epugh Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

dsmiley Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

epugh commented Feb 11, 2026

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

dsmiley Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants