fix: Reduce redundant network calls in rill developer #4883

AdityaHegde · 2024-05-13T10:17:48Z

We have multiple redundant network calls in rill developer.

During initial load we make a call to all resource and all files names. Separately we also make a call to all resources, this can be filled in using all resources call.
WatchResourceClient receives the updated resource, but we still make another call to it.
We refresh list of all file names for every file write. We can only do this when a new file is added or file is deleted.
We make a call to ListResources with kind in addition to the ListResource with no params. We can filter from the full list instead. (Dashboard list is pending since it used in cloud where we might not always make a call to list resources)

This also attempts to move any metadata needed for these watch clients to those files instead of the global store FileArtifacts.

Add WatchFileClient to store any metadata needed only during watching files.
Add WatchResourceClient to store metadata needed only during watching resources.
Finally fix E2E and make sure there are no regressions.

ericpgreen2 · 2024-05-14T15:04:50Z

/review

rill-dev · 2024-05-14T15:04:58Z

Code Review Agent Run Status

AI Based Review: Successful

Code Review Overview

Summary: The PR introduces significant optimizations and refactoring aimed at reducing redundant network calls in the Rill Developer environment. Changes include the introduction of new utility classes for resource management, optimization of resource and file metadata handling, and removal of outdated methods and properties.
Code change type: Optimisation, Refactoring
Unit tests added: True
Estimated effort to review (1-5, lower is better): 3

>>See detailed code suggestions<<
The Bito AI Code Review Agent successfully reviewed 16 files and discovered 4 issues. Please review these issues along with suggested fixes in the Changed Files.

See other commands you can run

High-level Feedback

Ensure consistent use of resource management methods across all components to maintain efficiency. Review the changes for potential impacts on existing functionalities and compatibility with cloud environments. Consider adding more comprehensive tests to cover new changes and ensure that private constructors do not hinder unit testing.

rill-dev · 2024-05-14T15:08:43Z

web-local/tests/utils/ResourceWatcher.ts

+    });
+  }
+
+  public async waitForResource(name: string) {


Issue: The method 'waitForResource' uses a hard-coded comparison string "RECONCILE_STATUS_IDLE", which could limit the flexibility of the ResourceWatcher class.
Fix: Consider making the status string a parameter of the 'waitForResource' method or part of the class configuration to increase flexibility and reusability of the 'ResourceWatcher' class.
Code Suggestion:

- return waitUntil(() => this.statuses.get(name) === "RECONCILE_STATUS_IDLE"); + return waitUntil(() => this.statuses.get(name) === this.idleStatus); // this.idleStatus should be configurable

It doesnt make sense to make this configurable.

rill-dev · 2024-05-14T15:08:44Z

web-common/src/features/entity-management/ResourceWatcher.svelte

@@ -1,13 +1,13 @@
 <script lang="ts">
  import { fileArtifacts } from "@rilldata/web-common/features/entity-management/file-artifacts";
-  import { createWatchFilesClient } from "@rilldata/web-common/features/entity-management/watch-files-client";
-  import { createWatchResourceClient } from "@rilldata/web-common/features/entity-management/watch-resources-client";
+  import { WatchFilesClient } from "@rilldata/web-common/features/entity-management/WatchFilesClient";


Issue: Import statements for 'WatchFilesClient' and 'WatchResourcesClient' use incorrect paths or naming conventions.
Fix: Correct the import statements to match the actual file names and paths in the project structure.
Code Suggestion:

- import { WatchFilesClient } from "@rilldata/web-common/features/entity-management/WatchFilesClient"; - import { WatchResourcesClient } from "@rilldata/web-common/features/entity-management/WatchResourcesClient"; + import { WatchFilesClient } from "@rilldata/web-common/features/entity-management/watch-files-client"; + import { WatchResourcesClient } from "@rilldata/web-common/features/entity-management/watch-resources-client";

This seems to be incorrect, did it not catch the file name rename? The file name should match the class name with camel case.

ericpgreen2

Some great cleanup in here!

ericpgreen2 · 2024-05-14T15:14:37Z

web-common/src/features/entity-management/WatchFilesClient.ts

+  public static New() {
+    const watcher = new WatchFilesClient();
+    return watcher.client;
+  }


This seems unconventional in Javascript. Can we just use the constructor to instantiate a new instance?

ericpgreen2 · 2024-05-14T15:15:32Z

web-common/src/features/entity-management/WatchFilesClient.ts

+  private invalidateAllFiles() {
+    // TODO: reset project parser errors
+    const instanceId = get(runtime).instanceId;
+    return queryClient.resetQueries({


Ideally we move from resetQueries to refetchQueries everywhere (but if that's risky, OK to keep out-of-scope of this PR). resetQueries typically leads to flashing.

ericpgreen2 · 2024-05-14T15:15:57Z

web-common/src/features/entity-management/WatchFilesClient.ts

+    const instanceId = get(runtime).instanceId;
+    const isNew = !this.seenFiles.has(res.path);
+
+    console.log(res.path, isNew);


Leftover console.log

ericpgreen2 · 2024-05-14T15:17:25Z

web-common/src/features/entity-management/WatchResourcesClient.ts

+  public static New() {
+    const watcher = new WatchResourcesClient();
+    return watcher.client;
+  }


Again, this seems unconventional in Javascript

ericpgreen2 · 2024-05-14T15:21:36Z

web-common/src/features/entity-management/resource-invalidations.ts

 export function refreshResource(
  queryClient: QueryClient,
  instanceId: string,
  res: V1Resource,
 ) {
-  return queryClient.resetQueries(
+  return queryClient.setQueryData(
    getRuntimeServiceGetResourceQueryKey(instanceId, {
      "name.name": res.meta?.name?.name,
      "name.kind": res.meta?.name?.kind,
    }),
+    {
+      resource: res,
+    },
  );
 }


Given this function is just a wrapper around setQueryData, can we remove the wrapper and use setQueryData directly? Else it requires a reader to mentally map "refreshResource" -> setQueryData, which seems unnecessary.

ericpgreen2 · 2024-05-14T15:25:24Z

web-local/tests/utils/ResourceWatcher.ts

+  }
+
+  public async waitForResource(name: string) {
+    return waitUntil(() => this.statuses.get(name) === "RECONCILE_STATUS_IDLE");


Our e2e tests should rely on user-visible indicators, not on internal state

True. I am iterating on this. That part is still TODO

Actually I am not sure if any user-visible indicators was reliable here. That is why we had waitForResource earlier.

ericpgreen2 · 2024-05-14T15:25:43Z

web-common/src/features/entity-management/resource-selectors.ts

+ * Fetches all resources and filters them client side.
+ * This is to improve network requests since we need the full list all the time as well.
+ */
+export function useClientFilteredResources(


ericpgreen2 · 2024-05-14T15:27:28Z

web-common/src/features/entity-management/WatchResourcesClient.ts

+    return watcher.client;
+  }
+
+  private handleWatchResourceResponse(res: V1WatchResourcesResponse) {


I like this code co-location!

AdityaHegde · 2024-05-14T15:54:55Z

Hey sorry this is not ready for review. Hence the bunch of console logs. Sorry if i gave the wrong impression. Also the AI seems wrong most of the time. It feels like spam most of the time.

Reduce redundant network calls in rill developer

34f1cda

AdityaHegde marked this pull request as draft May 13, 2024 10:17

AdityaHegde added 3 commits May 13, 2024 16:08

Add WatchFileClient

82785fd

Add WatchResourcesClient

2010aac

Update E2E

043725a

rill-dev reviewed May 14, 2024

View reviewed changes

rilldata deleted a comment from rill-dev May 14, 2024

ericpgreen2 reviewed May 14, 2024

View reviewed changes

ericpgreen2 assigned AdityaHegde May 14, 2024

Fixing some E2E

26d5cea

AdityaHegde force-pushed the adityahegde/rill-developer-network-improvements branch from 6468a84 to 26d5cea Compare May 15, 2024 07:35

Merge branch 'main' into adityahegde/rill-developer-network-improvements

d2189b1

AdityaHegde mentioned this pull request May 20, 2024

fix: Clearing a dashboard yaml stops updating errors #4923

Merged

AdityaHegde added 3 commits May 23, 2024 11:26

Merge branch 'main' into adityahegde/rill-developer-network-improvements

5439303

Merge branch 'main' into adityahegde/rill-developer-network-improvements

814b90b

Fix issues with 1st time resource

2e4aad5

AdityaHegde marked this pull request as ready for review May 23, 2024 16:37

AdityaHegde requested a review from ericpgreen2 May 23, 2024 16:37

rilldata deleted a comment from rill-dev May 23, 2024

ericpgreen2 approved these changes May 23, 2024

View reviewed changes

ericpgreen2 merged commit 905a798 into main May 23, 2024
4 checks passed

ericpgreen2 deleted the adityahegde/rill-developer-network-improvements branch May 23, 2024 23:43

ericpgreen2 added the patch label May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Reduce redundant network calls in rill developer #4883

fix: Reduce redundant network calls in rill developer #4883

AdityaHegde commented May 13, 2024 •

edited

ericpgreen2 commented May 14, 2024

rill-dev commented May 14, 2024 •

edited

rill-dev May 14, 2024

AdityaHegde May 14, 2024

rill-dev May 14, 2024

AdityaHegde May 14, 2024

ericpgreen2 left a comment

ericpgreen2 May 14, 2024 •

edited

ericpgreen2 May 14, 2024 •

edited

ericpgreen2 May 14, 2024

ericpgreen2 May 14, 2024

ericpgreen2 May 14, 2024

ericpgreen2 May 14, 2024

AdityaHegde May 14, 2024

AdityaHegde May 14, 2024

ericpgreen2 May 14, 2024

ericpgreen2 May 14, 2024

AdityaHegde commented May 14, 2024

fix: Reduce redundant network calls in rill developer #4883

fix: Reduce redundant network calls in rill developer #4883

Conversation

AdityaHegde commented May 13, 2024 • edited

ericpgreen2 commented May 14, 2024

rill-dev commented May 14, 2024 • edited

Code Review Agent Run Status

Code Review Overview

High-level Feedback

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

ericpgreen2 left a comment

Choose a reason for hiding this comment

ericpgreen2 May 14, 2024 • edited

Choose a reason for hiding this comment

ericpgreen2 May 14, 2024 • edited

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

AdityaHegde commented May 14, 2024

AdityaHegde commented May 13, 2024 •

edited

rill-dev commented May 14, 2024 •

edited

ericpgreen2 May 14, 2024 •

edited

ericpgreen2 May 14, 2024 •

edited