refactor: Cleanup DHC Service #171
Conversation
bmingles
commented
Nov 7, 2024
•
bmingles
commented
- Refactored DhcService to be more consistent with patterns used by DheService
- Psks are now stored in secret storage instead of keeping credentials in memory cache. They now behave similar to private keys in that one that has been saved will be automatically used when connecting to the server.
End-to-end Test Summary
Detailed Test Results
Failed Test SummaryNo failed tests ✨Flaky Test SummaryNo flaky tests detected. ✨ |
bmingles
force-pushed
the
151-cleanup-dhc-service
branch
from
1aa0986 to
ce62ad4
Compare
package.json
Outdated
| "command": "vscode-deephaven.createCoreAuthenticatedClient", | ||
| "title": "Create Authenticated Client" | ||
| }, | ||
| { | ||
| "command": "vscode-deephaven.createDHEAuthenticatedClient", | ||
| "title": "Create Authenticated Client" |
There was a problem hiding this comment.
Should these two actions have different titles?
There was a problem hiding this comment.
Technically, the titles never show in the UI, but I've update for clarity.
| export class CoreJsApiCache extends ByURLAsyncCache<typeof DhcType> { | ||
| constructor() { | ||
| super(async url => | ||
| initDhcApi(url, getTempDir({ subDirectory: urlToDirectoryName(url) })) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Something I thought of here - technically, we should cache the API by the engine configuration, rather than the URL... as two workers could have the same API/configuration, but would host the API on separate URLs.
Also I have a refactoring on DHE for using the WebClientData from a Core+ worker, and part of that is the CorePlusManager for getting connections/clients and having it cached: https://github.com/deephaven-ent/iris/pull/2404/files#diff-f19e170fbd46d5fe67fdfc7daae3a0f97de914ae3a87a1e70633e6d7150b27da
Might be something we publish a common package that's used in both projects.
There was a problem hiding this comment.
@mofojed Workers store the respective api by the worker URL. Am I misunderstanding?
There was a problem hiding this comment.
@bmingles Workers each have their own JS API URL, correct. However, in the case of DHE, you could have multiple workers that have the same worker kind configuration/engine; in which case you're just downloading the same JS API multiple times from different URLs. We optimize this in DHE by memoizing fetching the API on the worker kind/engine type: https://github.com/deephaven-ent/iris/blob/0fccc395d44974571e71085c762b68c151c7ca58/web/client-ui/src/main/AppMainContainer.tsx#L587
There was a problem hiding this comment.
oh gotcha. Mind if I handle that in a separate PR to keep things cleaner? That's technically not a new concept introduced by this one, and it might be better to do after the jsapi fetch swap coming next.
There was a problem hiding this comment.
@mofojed I've created Cache DHE Core+ jsapis by worker kind / engine type #174 to follow up on this.
