Skip to content

Conversation

@gertjanal
Copy link
Contributor

Description

The /metrics endpoint lists active nodes, but not all nodes are workers and # of workers is not always equal to # nodes - # coordinators as coordinators can be workers as well.

This PR adds CoordinatorNodeManager_ActiveCoordinatorCount and CoordinatorNodeManager_ActiveWorkerCount metrics. I can't find any tests for the current metrics in the CoordinatorNodeManager, so I'm not sure they should be added.

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( X ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

## Metrics
* Added coordinator and worker counts to the metrics endpoint

@cla-bot cla-bot bot added the cla-signed label Nov 21, 2025
@github-actions github-actions bot added the docs label Nov 21, 2025
@gertjanal gertjanal marked this pull request as draft November 21, 2025 20:03
@gertjanal gertjanal force-pushed the add_activeWorkerCount_and_activeCoordinatorCount_metrics branch from c3f98f0 to 8243d5f Compare November 21, 2025 20:19
@gertjanal gertjanal requested a review from dain November 21, 2025 20:20
@gertjanal gertjanal self-assigned this Nov 21, 2025
@gertjanal gertjanal marked this pull request as ready for review November 21, 2025 20:20
@gertjanal gertjanal force-pushed the add_activeWorkerCount_and_activeCoordinatorCount_metrics branch 2 times, most recently from 9990048 to 55146ba Compare November 23, 2025 20:31
private final boolean worker;
private final long longHashCode;

public InternalNode(String nodeIdentifier, URI internalUri, NodeVersion nodeVersion, boolean coordinator)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This constructor is used in quite some test files. Shall I remove this constructor and update the tests, or shall I keep it this way?

@gertjanal gertjanal force-pushed the add_activeWorkerCount_and_activeCoordinatorCount_metrics branch from 55146ba to 110890f Compare November 23, 2025 20:35
<item>
<ignore>true</ignore>
<code>java.method.addedToInterface</code>
<new>method boolean io.trino.spi.Node::isWorker()</new>
Copy link
Contributor

Choose a reason for hiding this comment

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

It's unnecessary to add it, since the worker means not coordinator. we already has isCoordinator, why not just reuse it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because a node can be a worker and coordinator at the same time using node-scheduler.include-coordinator as described here: https://trino.io/docs/current/installation/deployment.html#config-properties

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

Development

Successfully merging this pull request may close these issues.

2 participants