Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add streaming updates core functionality and project streaming #8669

Merged
merged 21 commits into from
Feb 9, 2024

Conversation

corban-beaird
Copy link
Contributor

@corban-beaird corban-beaird commented Jan 9, 2024

Description

Add Streaming Updates core functionality with project streaming & testing. This also provides the framework for adding additional streamable types as time goes on. It also provides a master/stream.py demo that can be used to test out the feature.

Offline & Out of Scope testing will need to be added back in due to those test not using streamable types that will be included in this initial merge.

Test Plan

  • Automated testing (stream/system_intg_test.go)

Commentary (optional)

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

DET-10056

@corban-beaird corban-beaird requested a review from a team as a code owner January 9, 2024 18:31
@cla-bot cla-bot bot added the cla-signed label Jan 9, 2024
Copy link

netlify bot commented Jan 9, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 880d884
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/65c68cf3b6353c0008d60218

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 333 lines in your changes are missing coverage. Please review.

Comparison is base (cfffe96) 47.79% compared to head (880d884) 47.82%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8669      +/-   ##
==========================================
+ Coverage   47.79%   47.82%   +0.03%     
==========================================
  Files        1050     1061      +11     
  Lines      167359   168109     +750     
  Branches     2240     2239       -1     
==========================================
+ Hits        79982    80394     +412     
- Misses      87219    87557     +338     
  Partials      158      158              
Flag Coverage Δ
backend 43.85% <55.71%> (+0.24%) ⬆️
harness 64.32% <ø> (-0.01%) ⬇️
web 42.52% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
master/internal/db/migrations.go 59.57% <100.00%> (ø)
master/internal/db/postgres.go 53.21% <100.00%> (ø)
master/pkg/model/task.go 15.59% <ø> (ø)
master/internal/stream/authz_basic_impl.go 75.00% <75.00%> (ø)
master/pkg/syncx/errgroupx/errgroupx.go 63.63% <0.00%> (-6.37%) ⬇️
master/internal/stream/subscription.go 92.68% <92.68%> (ø)
master/internal/core.go 2.46% <0.00%> (-0.02%) ⬇️
master/internal/stream/socketlike.go 33.33% <33.33%> (ø)
master/internal/stream/messages.go 0.00% <0.00%> (ø)
master/internal/stream/util.go 67.74% <67.74%> (ø)
... and 6 more

... and 3 files with indirect coverage changes

master/plan Outdated Show resolved Hide resolved
Copy link
Contributor

@eecsliu eecsliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! My only question is whether or not to include stuff in this PR - comments and extra files meant for developing

master/internal/stream/system.go Outdated Show resolved Hide resolved
master/manytrials.sh Outdated Show resolved Hide resolved
master/pkg/stream/stream.go Outdated Show resolved Hide resolved
master/plan Outdated Show resolved Hide resolved
master/stream.py Outdated Show resolved Hide resolved
@corban-beaird corban-beaird force-pushed the corban-beaird-streaming-updates-core-func branch 3 times, most recently from f68d968 to b08e286 Compare January 22, 2024 17:43
Copy link
Contributor

@eecsliu eecsliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to @rb-determined-ai, but I'm ok with this change. I feel like it'd be rare to bump up against the 64 bit int overflow to begin with (but maybe in a few years I'll be changing my tune...)

master/internal/stream/DESIGN.md Outdated Show resolved Hide resolved
master/internal/stream/projects.go Show resolved Hide resolved
master/pkg/stream/stream_test.go Show resolved Hide resolved
@corban-beaird corban-beaird force-pushed the corban-beaird-streaming-updates-core-func branch from 378c144 to b877a88 Compare January 24, 2024 16:52
@erikwilson erikwilson self-requested a review January 26, 2024 16:17
// Return Values:
// - `gone` is a range-encoded string, suitable for returning over the REST API.
// - `new` is a list of ints, which are PKs that will need hydrating from the database.
func ProcessKnown(known string, exist []int64) (string, []int64, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProcessKnown might not be entirely correct if known and/or exist is not sorted, while it is highly possible for exist to be unsorted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add in the assumption to the comment.

known is created by the streaming client, which keeps track of the known keys and builds the known string, and exist is populate by a query for each streamable type (currently just projects).

The project select includes an ORDER <PK> ASC; so we should be be fine in terms of making sure that exists is sorted.

As long as the streaming client does not build an improper known string and any additional streamable types are also sorting the results of their queries properly, this shouldn't be an issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since exist is basically a concatenation of oldEventsQuery scan and newEventsQuery scan, both queries sorted does not guarantee exist also sorted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

Copy link
Contributor

@erikwilson erikwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just leaving a comment here so that I can be notified properly.
From a slack conversation with Corban it sounds like the code is being re-worked to deal with the character limit.
If the code is actually ready now, or whenever it does become ready, please re-request a review.
Thanks!

@corban-beaird corban-beaird force-pushed the corban-beaird-streaming-updates-core-func branch from 80dc1ee to 1c35290 Compare February 8, 2024 22:59
@corban-beaird
Copy link
Contributor Author

corban-beaird commented Feb 8, 2024

Just leaving a comment here so that I can be notified properly. From a slack conversation with Corban it sounds like the code is being re-worked to deal with the character limit. If the code is actually ready now, or whenever it does become ready, please re-request a review. Thanks!

@erikwilson
In an effort to get the core functionality available for the transition with the re-org, the transition over to only communicating using filterable columns will be considered a separate unit of work

Copy link
Member

@rb-determined-ai rb-determined-ai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work! It's been a long haul

@corban-beaird corban-beaird merged commit 6206bde into main Feb 9, 2024
71 of 87 checks passed
@corban-beaird corban-beaird deleted the corban-beaird-streaming-updates-core-func branch February 9, 2024 20:54
maxrussell pushed a commit that referenced this pull request Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants