UI to interact with Relay server #3484

MVarshini · 2023-07-07T10:21:27Z

PBENCH-1141

Develop UI to interact with Relay server 'pull' API

dbutenhof

This is great; you seem to have some duplicate code to clean up, plus we need to finalize the header in the dialog.

dashboard/src/App.js

dashboard/src/modules/components/RelayUIComponent/index.jsx

dbutenhof · 2023-07-07T11:59:20Z

dashboard/src/actions/relayActions.js

+      uri: fileURI,
+    });
+    const response = await API.post(uri, null, null);
+    if (response.status >= 200 && response.status < 300) {


That's interesting. Does that mean that the axios response object doesn't have an ok attribute or method? Too bad.

This is probably fine, although the server will only return 201 (CREATED) or 200 (OK) on success. But anything else will be >= 400, so this will work.

In axios, there is ok attribute in the response object

I'm curious why you didn't use if (response.ok) here, then? It's not a problem, and I think this will work just fine.

I think this will work just fine.

Yes, but ok would be better.... 🙂

dashboard/src/actions/relayActions.js

dashboard/src/modules/components/RelayUIComponent/index.jsx

dbutenhof · 2023-07-07T15:55:32Z

dashboard/src/actions/relayActions.js

+      uri: fileURI,
+    });
+    const response = await API.post(uri, null, null);
+    if (response.status >= 200 && response.status < 300) {


I'm curious why you didn't use if (response.ok) here, then? It's not a problem, and I think this will work just fine.

webbnh

This looks good (and so I'm approving), but I've pointed out some opportunities for polishing if you feel inclined.

webbnh · 2023-07-10T15:01:31Z

dashboard/src/modules/components/RelayUIComponent/index.jsx

+  };
+  const handleClose = () => {
+    dispatch(handleInputChange(""));
+    dispatch(toggleRelayModal(false));


Not a huge deal, but I think that toggleRelayModal() is misnamed -- "toggle" generally refers to inverting an existing state, like switching from "true" to "false" and "false" to "true", and, therefore, a toggle function would not require an parameter, because its behavior would be based on the existing state -- so this function really should be named something more like "setRelayModalState()".

Renamed the function

webbnh · 2023-07-10T15:20:29Z

dashboard/src/modules/components/RelayUIComponent/index.jsx

+  const pullServerData = () => {
+    dispatch(uploadFile());
+  };


It's not obvious what the benefit of wrapping this dispatch() call in a function is. I would in-line its body in the place where it is called and omit this definition (like what you did at line 97). Failing that, you know my bias toward expressions, so, if you don't in-line it, I would express this as:

const pullServerData = () => dispatch(uploadFile());

which is simpler here, but I don't know if it will make a difference at the invocation site (i.e., my code would evaluate to a Promise while I think yours throws away the Promise and evaluates to undefined).

webbnh · 2023-07-10T15:23:59Z

dashboard/src/modules/components/RelayUIComponent/index.jsx

+    <p>
+      When a firewall prevents this direct access, the Pbench Agent can store a
+      dataset tarball and a manifest file on a file relay server which can be
+      reached by both the Pbench Agent and the Pbench Server.{" "}


I don't think we want or need the trailing {" "}.

webbnh · 2023-07-10T15:25:15Z

dashboard/src/modules/components/RelayUIComponent/index.jsx

+      Enter the relay server URI reported by the Pbench Agent and press Submit
+      to begin the import.


Do we like "import"? I would recommend "upload". 🤷

webbnh · 2023-07-10T15:26:41Z

dashboard/src/modules/components/RelayUIComponent/index.jsx

+const PopoverComponent = () => {
+  return (


Using a code blob which does nothing but invoke a return statement is pointless -- just remove the brances and the return statement and let the expression to be returned stand as the value of the arrow function.

webbnh · 2023-07-10T15:44:33Z

dashboard/src/actions/relayActions.js

+    const uri = uriTemplate(endpoints, "relay", {
+      uri: fileURI,
+    });


If you remove the trailing comma from line 16, I bet Prettier will combine these three lines into one, which would be nicer.

webbnh · 2023-07-10T15:48:26Z

dashboard/src/actions/relayActions.js

+      uri: fileURI,
+    });
+    const response = await API.post(uri, null, null);
+    if (response.status >= 200 && response.status < 300) {


I think this will work just fine.

Yes, but ok would be better.... 🙂

webbnh · 2023-07-10T15:49:53Z

dashboard/src/actions/relayActions.js

+      if (response.status === 201) {
+        // need to remove once response returns the uploaded dataset
+        dispatch(getDatasets());
+      }


I don't understand the code comment at line 24. (Should this instead be applied to line 21, which removes the modal dialog display?)

@MVarshini : despite this comment, it's probably best to leave the code alone for now and come back later.

However, for what it's worth, if you want to think about your refresh optimization ideas, I've updated the staging server with the latest code from main. I restarted the relay and pushed a dataset, and now (omitting the internal hostnames from GitHub comments):

POST https://<staging-server>/api/v1/relay/http://<relay>:8002/RELAY/4bcada96aff3b1d9a62540962204c833c12cd9266b10d747e1ef71b278d23a09

will return:

{ "message": "File successfully uploaded", "name": "pbench-user-benchmark_newrelay_2023.07.11T12.04.37", "resource_id": "a9b1daaa5549c4063c00e1d62115c9e5" }

webbnh · 2023-07-10T15:56:26Z

dashboard/src/actions/relayActions.js

+    );
+    dispatch({ type: TYPES.NETWORK_ERROR });


What does the NETWORK_ERROR action do? (I don't see a reducer which actually handles it.)

I'm not sure I understand that, either: I haven't noticed anything that sets a payload value, but the actions here and in the visualizer PR both use this unconditionally when errors occur, but I don't see any implementation of that type code.

Create UItointeract with relay server pull API

dbutenhof · 2023-07-11T12:11:51Z

dashboard/src/actions/relayActions.js

+      if (response.status === 201) {
+        // need to remove once response returns the uploaded dataset
+        dispatch(getDatasets());
+      }


@MVarshini : despite this comment, it's probably best to leave the code alone for now and come back later.

However, for what it's worth, if you want to think about your refresh optimization ideas, I've updated the staging server with the latest code from main. I restarted the relay and pushed a dataset, and now (omitting the internal hostnames from GitHub comments):

POST https://<staging-server>/api/v1/relay/http://<relay>:8002/RELAY/4bcada96aff3b1d9a62540962204c833c12cd9266b10d747e1ef71b278d23a09

will return:

{ "message": "File successfully uploaded", "name": "pbench-user-benchmark_newrelay_2023.07.11T12.04.37", "resource_id": "a9b1daaa5549c4063c00e1d62115c9e5" }

dbutenhof · 2023-07-11T12:24:03Z

dashboard/src/actions/relayActions.js

+    );
+    dispatch({ type: TYPES.NETWORK_ERROR });


I'm not sure I understand that, either: I haven't noticed anything that sets a payload value, but the actions here and in the visualizer PR both use this unconditionally when errors occur, but I don't see any implementation of that type code.

MVarshini added the Dashboard Of and relating to the Dashboard GUI label Jul 7, 2023

MVarshini added this to the v0.73 milestone Jul 7, 2023

MVarshini requested a review from dbutenhof July 7, 2023 10:21

MVarshini self-assigned this Jul 7, 2023

MVarshini added this to In progress in v0.72 via automation Jul 7, 2023

dbutenhof requested changes Jul 7, 2023

View reviewed changes

v0.72 automation moved this from In progress to Review in progress Jul 7, 2023

dbutenhof requested review from ndokos and webbnh July 7, 2023 12:08

dbutenhof previously approved these changes Jul 7, 2023

View reviewed changes

v0.72 automation moved this from Review in progress to Reviewer approved Jul 7, 2023

MVarshini force-pushed the PBENCH-1141 branch from bbab4d5 to 644f3c7 Compare July 10, 2023 07:34

webbnh previously approved these changes Jul 10, 2023

View reviewed changes

MVarshini added 7 commits July 11, 2023 16:08

PBENCH-1141

17e09b7

Create UItointeract with relay server pull API

UI update

f578efc

relay modal ui

f5bbe47

prettier

59d1c1e

review comments

5062821

aria-label for relay modal

23eb4ea

review comments

7d7a222

MVarshini dismissed stale reviews from webbnh and dbutenhof via 7d7a222 July 11, 2023 10:39

MVarshini force-pushed the PBENCH-1141 branch from 644f3c7 to 7d7a222 Compare July 11, 2023 10:39

v0.72 automation moved this from Reviewer approved to Review in progress Jul 11, 2023

dbutenhof approved these changes Jul 11, 2023

View reviewed changes

v0.72 automation moved this from Review in progress to Reviewer approved Jul 11, 2023

dbutenhof merged commit 32c6c5c into distributed-system-analysis:main Jul 11, 2023
4 checks passed

v0.72 automation moved this from Reviewer approved to Done Jul 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI to interact with Relay server #3484

UI to interact with Relay server #3484

MVarshini commented Jul 7, 2023

dbutenhof left a comment

dbutenhof Jul 7, 2023

MVarshini Jul 7, 2023

dbutenhof Jul 7, 2023

webbnh Jul 10, 2023

dbutenhof Jul 7, 2023

webbnh left a comment

webbnh Jul 10, 2023

MVarshini Jul 11, 2023

webbnh Jul 10, 2023

webbnh Jul 10, 2023

MVarshini Jul 11, 2023

webbnh Jul 10, 2023

MVarshini Jul 11, 2023

webbnh Jul 10, 2023

MVarshini Jul 11, 2023

webbnh Jul 10, 2023

MVarshini Jul 11, 2023

webbnh Jul 10, 2023

webbnh Jul 10, 2023

dbutenhof Jul 11, 2023

webbnh Jul 10, 2023

dbutenhof Jul 11, 2023

dbutenhof Jul 11, 2023

dbutenhof Jul 11, 2023

		Enter the relay server URI reported by the Pbench Agent and press Submit
		to begin the import.

UI to interact with Relay server #3484

UI to interact with Relay server #3484

Conversation

MVarshini commented Jul 7, 2023

dbutenhof left a 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

webbnh left a 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