Add system.runtime.nodes information to web UI #11653

whutpencil · 2022-03-24T12:17:57Z

Description

The fact that you can't see a list of workers in the Web UI doesn't seem elegant. The goal of this PR is to improve it. The effect is shown in the figure below:

The value of active workers became a hyperlink.

When you click it, you will enter a new page with the contents as you run select * from system.runtime.nodes; the results obtained.

At the same time, each node ID and node IP supports jumping to the corresponding machine to view information.

This is the effect of having multiple nodes in a formal cluster:

Is this change a fix, improvement, new feature, refactoring, or other?
Improvement.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)
No.

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Web UI
* Add list of nodes to UI. ({issue}`11653`)

findepi · 2022-03-24T15:16:26Z

@dedep PTAL

whutpencil · 2022-03-31T04:00:10Z

@findepi Why hasn't anyone reviewed the code for so long? What happened to Trino community activity? If I can, I am willing to participate in the review process of assisting the community. LOL ^^

findepi · 2022-03-31T10:29:02Z

@whutpencil i am not a frontend dev person, so I wouldn't want to review myself, but let me try again find someone.

dedep · 2022-03-31T11:25:49Z

core/trino-main/src/main/java/io/trino/server/ui/WorkerResource.java

+            return state;
+        }
+    }
+


You have a checkstyle error here

dedep · 2022-03-31T11:30:00Z

core/trino-main/src/main/java/io/trino/server/ui/WorkerResource.java

+                @JsonProperty("coordinator") String coordinator,
+                @JsonProperty("state") String state)
+        {
+            this.nodeId = nodeId;


Use requireNonNull.

this.nodeId = requireNonNull(nodeId, "nodeId is null"); this.nodeIp = requireNonNull(nodeIp, "nodeIp is null"); ...

dedep · 2022-03-31T11:31:27Z

core/trino-main/src/main/java/io/trino/server/ui/WorkerResource.java

+        private final String nodeIp;
+        private final String nodeVersion;
+        private final String coordinator;
+        private final String state;


If state is either "active" or "inactive" then maybe it's better to have an enum for that?

dedep · 2022-03-31T11:35:11Z

core/trino-main/src/main/java/io/trino/server/ui/WorkerResource.java

+        private final String nodeId;
+        private final String nodeIp;
+        private final String nodeVersion;
+        private final String coordinator;


Imo, coordinator should be boolean. Why convert it to a String?

dedep · 2022-03-31T11:37:52Z

core/trino-main/src/main/resources/webapp/src/components/WorkerList.jsx

+
+    refreshLoop() {
+        clearTimeout(this.timeoutId); // to stop multiple series of refreshLoop from going on simultaneously
+        // const nodeId = getFirstParameter(window.location.search);


Can you remove the commented code line?

dedep · 2022-03-31T11:40:32Z

core/trino-main/src/main/resources/webapp/src/components/WorkerList.jsx

+                        initialized: true,
+                    });
+                });
+


Can you remove this empty line?

dedep · 2022-03-31T11:42:10Z

core/trino-main/src/main/resources/webapp/src/components/WorkerList.jsx

+    }
+
+    refreshLoop() {
+        clearTimeout(this.timeoutId); // to stop multiple series of refreshLoop from going on simultaneously


What's this.timeoutId? I can't find where you set it.
I suppose you wanted to send a request in the loop, but you didn't add setTimeout anywhere.

dedep · 2022-03-31T12:05:38Z

core/trino-main/src/main/resources/webapp/workers.html

+    <div id="worker-list">
+        <div class="loader">Loading...</div>
+    </div>
+    <!--<div id="worker-threads">


Please remove the commented code

dedep · 2022-03-31T12:14:04Z

core/trino-main/src/main/resources/webapp/src/components/WorkerList.jsx

+            })
+            for (var index in workerInfo) {
+                this.setState({
+                    workerId: addToHistory(workerInfo[index].nodeId, this.state.workerId),


addToHistory function signature is:
export function addToHistory (value: number, valuesArray: number[]): number[] {

You don't pass numbers, but strings.
I don't think you should use this function here.

dedep · 2022-03-31T12:14:41Z

core/trino-main/src/main/resources/webapp/src/components/WorkerList.jsx

+                workerInfo: workerInfo
+            })
+            for (var index in workerInfo) {
+                this.setState({


I think that you didn't design the state well. Instead of

{ workerId: string[], workerIp: string[], workerVersion: string[], ... }

I would do (typescript notation):

{ workers: [ workerId: string, workerIp: string, workerVersion: string, ... ] }

This way, the model is more concise and easier to work on

whutpencil · 2022-04-01T12:53:46Z

@dedep Thank you very much for your detailed review. I'm not a full-time front-end staff, so I didn't use the corresponding data structure well in the implementation process. I learned a lot from your comments and have made changes according to your modification opinions. Please review again.

dedep · 2022-04-01T14:23:00Z

core/trino-main/src/main/java/io/trino/server/ui/WorkerResource.java

+            this.nodeId = requireNonNull(nodeId, "nodeId is null");
+            this.nodeIp = requireNonNull(nodeIp, "nodeIp is null");
+            this.nodeVersion = requireNonNull(nodeVersion, "nodeVersion is null");
+            this.coordinator = requireNonNull(coordinator, "coordinator is null");


There is no need to do the requireNonNull check for a primitive (boolean) type

dedep · 2022-04-01T14:28:15Z

core/trino-main/src/main/resources/webapp/src/components/WorkerList.jsx

+            workerInfo: null,
+            initialized: false,
+            ended: false,
+            workers: [{


workerId: String, workerIp: String, workerVersion: String, coordinator: Boolean, state: String

is not a correct JavaScript code. The code snippet I posted before was written in Typescript to show you the types.

So, I think it should be just: workers: []

dedep · 2022-04-01T14:30:40Z

core/trino-main/src/main/resources/webapp/src/components/WorkerList.jsx

+
+    refreshLoop() {
+        clearTimeout(this.timeoutId);
+        $.get('/ui/api/worker', function (workerInfo) {


I would rename workerInfo to workers to signal that this is an array.

dedep · 2022-04-01T14:34:29Z

core/trino-main/src/main/resources/webapp/src/components/WorkerList.jsx

+        this.state = {
+            workerInfo: null,
+            initialized: false,
+            ended: false,


I think that ended property is not used anywhere. IMO, you can remove this line

whutpencil · 2022-04-06T02:11:14Z

@dedep Excuse me, do you have any further amendments? Could this PR be merged into the master?

electrum · 2022-04-07T17:34:04Z

Thanks for your contribution!

mosabua · 2022-04-14T16:02:03Z

Hey @whutpencil .. I would like to feature this PR in the Trino Community Broadcast next week. Would you be interested in joining me a guest in the episode?

mosabua · 2022-04-20T06:43:07Z

I am going ahead with demoing this in Trino Community Broadcast 35 on Thursday ..

whutpencil · 2022-04-21T03:08:26Z

font{ line-height: 1.6; } ul,ol{ padding-left: 20px; list-style-position: inside; } Hello, I'm very sorry that I'm busy recently due to my family. I didn't pay attention to the email and reply to your message in time. I am honored to contribute to Trino community. Thank you very much for your invitation. Do I need to prepare anything? 137497191 ***@***.*** 签名由网易邮箱大师定制 On 04/20/2022 14:43，Manfred ***@***.***> wrote： I am going ahead with demoing this in Trino Community Broadcast 35 on Thursday .. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

mosabua · 2022-04-26T02:48:26Z

You can watch the demo and such now.. https://trino.io/episodes/35.html

Add system.runtime.nodes information to web UI

fcc2f21

cla-bot bot added the cla-signed label Mar 24, 2022

whutpencil requested review from electrum March 24, 2022 12:19

findepi added enhancement New feature or request ui Web UI labels Mar 24, 2022

findepi assigned raghavsethi and unassigned raghavsethi Mar 24, 2022

findepi requested a review from raghavsethi March 24, 2022 15:16

dedep reviewed Mar 31, 2022

View reviewed changes

modify by review

2d745d8

dedep reviewed Apr 1, 2022

View reviewed changes

modify by review again

a2d5865

whutpencil requested a review from dedep April 2, 2022 02:07

electrum merged commit 1214c7f into trinodb:master Apr 7, 2022

github-actions bot added this to the 376 milestone Apr 7, 2022

mosabua mentioned this pull request Apr 7, 2022

Add Trino 376 release notes #11691

Merged

whutpencil deleted the worker_ui branch April 11, 2022 01:22

maltesander mentioned this pull request Apr 12, 2022

Provide and test trino-server-377 stackabletech/trino-operator#187

Closed

7 tasks

Heltman mentioned this pull request Sep 8, 2022

Add endpoint to get worker information #14049

Open

Add system.runtime.nodes information to web UI #11653

Add system.runtime.nodes information to web UI #11653

Conversation

whutpencil commented Mar 24, 2022 • edited by electrum Loading

Description

Related issues, pull requests, and links

Documentation

Release notes

findepi commented Mar 24, 2022

whutpencil commented Mar 31, 2022

findepi commented Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whutpencil commented Apr 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whutpencil commented Apr 6, 2022

electrum commented Apr 7, 2022

mosabua commented Apr 14, 2022

mosabua commented Apr 20, 2022

whutpencil commented Apr 21, 2022 via email

mosabua commented Apr 26, 2022

whutpencil commented Mar 24, 2022 •

edited by electrum

Loading