feat: Connect ProjectStore with streaming updates #8834
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
master/internal/api/handler.go
Outdated
| func bypasscheckorigin(r *http.Request) bool { | ||
| return true | ||
| } | ||
|
|
||
| var upgrader = websocket.Upgrader{ | ||
| CheckOrigin: bypasscheckorigin, | ||
| } |
There was a problem hiding this comment.
I didn't know about this header.
This is a fine solution for this draft PR but we'll need to fix the server to handle this better in the near future.
| let same = true; | ||
| forEach(entity, (val, k) => { | ||
| if (!this.#curEntity?.[k as Streamable].spec.equals(val.spec)) same = false; | ||
| }); | ||
| return same; |
There was a problem hiding this comment.
This feels like a case for Array.prototype.every(), for the early exit behavior:
return Object.entries(entity).every(
([k, val]) => this.#curEntity![k as Streamable].spec.equals(val.spec)
)
Wait, but even then... this isn't quite right, because you really need to compare two sparse dictionaries.
If your curEntity was:
{
projects: pspec1
trials: tspec1
}
and your entity was:
{
projects: pspec1
}
then this check would say "true" because it checks that all keys in entity match the same keys on curEntity, but it doesn't check for extra keys in curEntity.
There was a problem hiding this comment.
I named the function poorly, but the intention was that, if I'm currently monitoring
{
projects: pspec1
trials: tspec1
}
and I subscribe to
{
projects: pspec1
}
then I should skip sending the subscription to the stream because what I want for projects has not changed. Thinking about this, I wonder if backend can do something similar, suppose we want to keep the fact that there is only one live subscription, can backend parse new subscription and only make changes if necessary?
For example, given the current subscription being
{
projects: pspec1
trials: tspec1
}
when receiving a new subscription
{
projects: pspec2
trials: tspec1
}
can we only make changes to projects, and keep everything trials untouched?
There was a problem hiding this comment.
can backend parse new subscription and only make changes if necessary?
No. That was the initial design, and it was way too hard to implement.
Plus, the option to allow that design created some extremely confusing corner cases in the client, like if:
- client starts subscribed to trials 1 and 2
- client receives offline messages until seqno S1
- client resubscribes to just trial 1
- server sees update for trial 2 with seqno S2
- server sees update for trial 1 with seqno S3
- client sees update to trial 1 with seqno S3, saves S3 as max_seqno
- client resubscribes to trials 1 and 2, listing both 1 and 2 as known entities and report max_seqno as S3. This is a problem because with 1 and 2 in known and max_seqno of S3, trial 2's update with seqno S2 will never be sent.
But I do think that having multiple independent subscriptions (each one immutable) is a simple and effective alternative for solving this problem; that's why I made a ticket for that.
There was a problem hiding this comment.
Can we make KeyCache sync with the current subscription on client side? So when client resubscribes to just trial 1 (unsubscribe from trial 2), the client remove trial 2 from keyCache (either generating a new keyCache, or passively processing trials_deleted), and when client resubscribes to trials 1 and 2, trial 2 should not be listed as known
There was a problem hiding this comment.
Hey, this looks pretty good. It would be really nice to see some unit tests in there too.
That caused me to see if I wrote unit tests for mine, and it looks like I did, and it also looks like I forgot to git add them. But that's fixed now, if you want to see my unit tests.
But I think unit tests are going to be very important to keeping this streaming updates part of our system bug free, much more so than with the REST API.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8834 +/- ##
==========================================
+ Coverage 47.35% 47.52% +0.17%
==========================================
Files 1162 1168 +6
Lines 176133 176705 +572
Branches 2237 2354 +117
==========================================
+ Hits 83402 83975 +573
+ Misses 92573 92572 -1
Partials 158 158
Flags with carried forward coverage won't be shown. Click here to find out more.
|
gt2345
force-pushed
the
gt/ts-streaming-client-1
branch
from
beb72ca
to
e96c967
Compare
Description
Connects workspace details page to ProjectStore, and ProjectStore monitors projects changes through streaming updates after the initial fetch.
Test Plan
Commentary (optional)
Note that if viewing as list, the changes of description is not immediately reflected due to how the Input is setup.
Checklist
docs/release-notes/.See Release Note for details.
Ticket
MD-265