Abstract Airlift Discovery from InternalNodeManager #26083

dain · 2025-06-27T01:49:32Z

This PR encapsulates Airlift discovery behind a new NodeInventory API. This is pre-work to replace discovery with a simpler system and to support K8s discovery directly.

The only significant change is that the information about nodes is now loaded directly from the node itself instead of relying on discovery announcements. This additional information is fetched from the node when the node state is being fetched, so it does not place any new additional burden on the nodes.

Release notes

(X) This is not user-visible or is docs only, and no release notes are required.

wendigo · 2025-06-27T12:41:01Z

Thanks @dain for doing that!

Copilot

Pull Request Overview

This PR abstracts Airlift-based discovery behind a new NodeInventory API, migrating node-related types from the io.trino.metadata package into io.trino.node, and updating consumers to use the new interfaces. It also refactors NodeStateManager to use the new CurrentNodeState supplier and enhances tests to wait for asynchronous node updates.

Introduce io.trino.node package with NodeInventory, InternalNode, NodeState, DiscoveryNodeManager, and related classes.
Refactor NodeStateManager to delegate state/version tracking to CurrentNodeState.
Update imports across the codebase and add polling loops in tests to stabilize asynchronous node visibility.

Reviewed Changes

Copilot reviewed 108 out of 108 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
testing/trino-tests/src/test/java/io/trino/tests/TestMinWorkerRequirement.java	Add `@Timeout` and polling loop to wait for node drop before assert
testing/trino-testing/src/main/java/io/trino/testing/DistributedQueryRunner.java	Replace immediate `verify` with a polling loop and timeout
core/trino-main/src/main/java/io/trino/node/NodeInventory.java	New interface to abstract node discovery
core/trino-main/src/main/java/io/trino/node/DiscoveryNodeManager.java	New discovery manager using `NodeInventory` and HTTP polling
core/trino-main/src/main/java/io/trino/server/NodeStateManager.java	Refactor to remove internal `VersionedState` and use `CurrentNodeState`

Comments suppressed due to low confidence (2)

core/trino-main/src/main/java/io/trino/server/NodeStateManager.java:316

[nitpick] This nested VersionedState class duplicates logic also found in the outer NodeStateManager refactor. Consider extracting it into a shared static class or moving it into CurrentNodeState as a static nested class to reduce duplication and improve clarity.

    public static class CurrentNodeState

core/trino-main/src/main/java/io/trino/node/AirliftNodeInventory.java:49

[nitpick] Consider adding unit tests for AirliftNodeInventory.getNodes() to verify that it correctly filters out failed services and extracts URIs based on the httpsRequired flag.

    public Set<URI> getNodes()

core/trino-main/src/main/java/io/trino/node/RemoteNodeState.java

testing/trino-tests/src/test/java/io/trino/tests/TestMinWorkerRequirement.java

core/trino-main/src/main/java/io/trino/metadata/DiscoveryNodeManager.java

core/trino-main/src/main/java/io/trino/metadata/RemoteNodeState.java

core/trino-main/src/main/java/io/trino/node/NodeState.java

core/trino-main/src/main/java/io/trino/node/RemoteNodeState.java

electrum · 2025-07-02T01:12:08Z

core/trino-main/src/main/java/io/trino/node/InternalNodeManager.java

+        return getAllNodes().getActiveCoordinators();
+    }

    AllNodes getAllNodes();


Unrelated, but I noticed that AllNodes could become a record

I added a commit at the end to do this.

core/trino-main/src/main/java/io/trino/node/TestingInternalNodeManager.java

The testing HTTP endpoint was invalid so state was always active

When new nodes are discovered the initial state is set to ACTIVE, but a node must be probed to get the initial state. It appears this bug was introduced when worker reactivation was added. Additional, nodes without the correct version are not tracked at all

Create InternalNode directly for current node Add bound CurrentNodeState which is a supplier of NodeState for the current node. This is requires changes to NodeStateManager to avoid circular Guice dependencies.

Instead of relying on discovery, node information can be pulled from the node when state is being fetched.

Workers only need a node manager to satisfy injection, but is otherwise not used.

Also, consolidate remaining discovery services into AirliftNodeInventoryModule

dain requested a review from electrum June 27, 2025 01:49

cla-bot bot added the cla-signed label Jun 27, 2025

github-actions bot added the postgresql PostgreSQL connector label Jun 27, 2025

dain marked this pull request as draft June 27, 2025 04:48

dain force-pushed the discovery-node-manager branch 5 times, most recently from ff703ad to 2f93c9d Compare June 28, 2025 19:21

dain marked this pull request as ready for review June 28, 2025 20:10

dain force-pushed the discovery-node-manager branch from 2f93c9d to 5e2de86 Compare June 30, 2025 20:56

wendigo requested a review from Copilot July 1, 2025 08:32

Copilot AI reviewed Jul 1, 2025

View reviewed changes

core/trino-main/src/main/java/io/trino/node/RemoteNodeState.java Outdated Show resolved Hide resolved

testing/trino-tests/src/test/java/io/trino/tests/TestMinWorkerRequirement.java Outdated Show resolved Hide resolved

github-actions bot added the delta-lake Delta Lake connector label Jul 1, 2025

dain force-pushed the discovery-node-manager branch 4 times, most recently from 3c009e5 to a6f5074 Compare July 1, 2025 23:07

electrum approved these changes Jul 2, 2025

View reviewed changes

dain added 10 commits July 1, 2025 19:22

Fix IntelliJ warnings in DiscoveryNodeManager

9b15fc1

Simplify coordinator classification in DiscoveryNodeManager

96a7208

Use enhanced switch statement in DiscoveryNodeManager

2ca0df6

Use ticker in TestDiscoveryNodeManager to eliminate wait times

76b980d

Fix simulated node state in TestDiscoveryNodeManager

7078ba5

The testing HTTP endpoint was invalid so state was always active

Fix testing server startup

7598c40

Convert TestDiscoveryNodeManager to simple parallel test

10efaeb

Simplify DiscoveryNodeManager node registration

11c367d

Add nodeId to ServerInfo

44cb85b

dain added 18 commits July 1, 2025 19:51

Simplify management of current InternalNode and NodeState

74d2c76

Create InternalNode directly for current node Add bound CurrentNodeState which is a supplier of NodeState for the current node. This is requires changes to NodeStateManager to avoid circular Guice dependencies.

Bind InternalNode for current node

98cfe89

Load node information from remote server info

3e3390b

Instead of relying on discovery, node information can be pulled from the node when state is being fetched.

Add INVALID node state for nodes with wrong version or environment

d4ca702

Abstract Airlift discovery from DiscoveryNodeManager

91f7555

Update CatalogPruneTask to use InternalNodeManager instead of discovery

f6cb6d0

Move node management classes to new node module

3438ba5

Use TestingNodeManager in TestingConnectorContext

e147255

Rename InMemoryNodeManager to TestingInternalNodeManager

2db8d04

Use shared CURRENT_NODE constant in testing node managers

5fb6266

Remove InternalNodeManager getCurrentNode

271dfb6

Split coordinator and worker node manager

bc31d5b

Workers only need a node manager to satisfy injection, but is otherwise not used.

Use separate http client for CatalogPruneTask

658d05e

Add gone tracking to CoordinatorNodeManager

08cf4cb

Simplify InternalNodeManager implementations with default methods

313285a

Improve TestingInternalNodeManager internals to support all methods

828fae4

Convert tests to use TestingInternalNodeManager

8c1a5c2

Also, consolidate remaining discovery services into AirliftNodeInventoryModule

Convert AllNodes to a Java record

a844f0d

dain force-pushed the discovery-node-manager branch from a6f5074 to a844f0d Compare July 2, 2025 03:06

dain merged commit 62db38e into trinodb:master Jul 2, 2025
121 checks passed

github-actions bot added this to the 477 milestone Jul 2, 2025

dain deleted the discovery-node-manager branch July 2, 2025 06:22

ebyhr mentioned this pull request Aug 6, 2025

Add Trino 477 release notes #26350

Merged

vishalya mentioned this pull request Oct 1, 2025

ClusterStatsJmxMonitor is broken with trino version 477+ trinodb/trino-gateway#773

Open

ksobolew mentioned this pull request Oct 21, 2025

Configure internal communication for ANNOUNCE discovery mode #27030

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Abstract Airlift Discovery from InternalNodeManager #26083

Abstract Airlift Discovery from InternalNodeManager #26083

Uh oh!

dain commented Jun 27, 2025 •

edited by electrum

Loading

Uh oh!

wendigo commented Jun 27, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

electrum Jul 2, 2025

Uh oh!

dain Jul 2, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Abstract Airlift Discovery from InternalNodeManager #26083

Abstract Airlift Discovery from InternalNodeManager #26083

Uh oh!

Conversation

dain commented Jun 27, 2025 • edited by electrum Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release notes

Uh oh!

wendigo commented Jun 27, 2025

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

electrum Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

dain Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

dain commented Jun 27, 2025 •

edited by electrum

Loading