Skip to content

fix: Make sys.server_properties table filterable and resilient to unreachable servers#19459

Open
abhishekrb19 wants to merge 2 commits into
apache:masterfrom
abhishekrb19:server_properties_table_fixes
Open

fix: Make sys.server_properties table filterable and resilient to unreachable servers#19459
abhishekrb19 wants to merge 2 commits into
apache:masterfrom
abhishekrb19:server_properties_table_fixes

Conversation

@abhishekrb19
Copy link
Copy Markdown
Contributor

Summary:

  1. Made sys.server_properties filterable, similar to a few other sys tables like SegmentsTable — changed from ScannableTable to ProjectableFilterableTable, enabling Calcite to push down filters (e.g., WHERE server = '...') and projections to the table scan.
  2. Graceful error handling — Previously, if any server was unreachable or returned an HTTP error, the entire query threw an exception and failed. Now the table returns a row for that server with property/value as null and the new
    error_message column populated with what went wrong.
  3. New error_message column — this column indicates if properties couldn't be fetched (e.g., "Connection refused", "HTTP 503: Service Unavailable"). Null on success.

Release note:

Added error_message column to sys.server_properties table and made the table resilient to unreachable servers. Previously, the entire query would fail if any server was unreachable; now a row is returned with error_message populated. The table also now supports filter and projection pushdown.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

Copy link
Copy Markdown
Contributor

@GWphua GWphua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with non-blocking comments.

### SERVER_PROPERTIES table

The `server_properties` table exposes the runtime properties configured on for each Druid server. Each row represents a single property key-value pair associated with a specific server.
The `server_properties` table exposes the runtime properties configured on each Druid server. Each row represents a single property key-value pair associated with a specific server. This table supports filter and projection pushdown for efficient querying. If a server is unreachable, the table still returns a row for that server with the `error` column populated instead of failing the entire query.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The `server_properties` table exposes the runtime properties configured on each Druid server. Each row represents a single property key-value pair associated with a specific server. This table supports filter and projection pushdown for efficient querying. If a server is unreachable, the table still returns a row for that server with the `error` column populated instead of failing the entire query.
The `server_properties` table exposes the runtime properties configured on each Druid server. Each row represents a single property key-value pair associated with a specific server. This table supports filter and projection pushdown for efficient querying. If a server is unreachable, the table still returns a row for that server with the `error_message` column populated instead of failing the entire query.

public void test_serverPropertiesTable_errorMessageColumnExists()
{
final String result = cluster.runSql("SELECT COUNT(*) FROM sys.server_properties WHERE error_message IS NULL");
Assertions.assertFalse(result.isEmpty(), "Should return count of servers with null error_message");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think this test just checks if the query did not fail. Can we make the test stronger? For example, asserting a return of a known value (e.g. 0,1,etc.)

@GWphua
Copy link
Copy Markdown
Contributor

GWphua commented May 13, 2026

If this is not urgent, we can wait a day for the AI-assisted review to trigger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants