Skip to content
This repository has been archived by the owner on Jan 19, 2023. It is now read-only.

Create streaming interface for client streaming. #2504

Closed
wants to merge 5 commits into from
Closed

Create streaming interface for client streaming. #2504

wants to merge 5 commits into from

Conversation

wwitzel3
Copy link
Contributor

What this PR does / why we need it:
Extracts the current websocket streaming in to an interface that can be replaced.
This work was done by @mstergianis in #1905 this PR is just addressing the merge conflict.

Michael Stergianis and others added 4 commits May 28, 2021 12:12
@wwitzel3
Separate data streaming from websockets
f56372e 
Signed-off-by: Michael Stergianis <mstergianis@vmware.com>
@wwitzel3
Fix error when closing browser tab
7151e06 
There was some incorrect error handling in my last commit. When fixing
that I noticed that I slightly changed the semantics of the websocket
read pump when I moved unmarshaling to be a client.Recieve
responsibility. So I introduced a new concept that a stream client can
return a fatal error, and read pump can react to that appropriately.
This paves the way for read pump to become a function owned by the
streaming client manager instead of a method on websocket client.

Signed-off-by: Michael Stergianis <mstergianis@vmware.com>
@wwitzel3
Make handleStreamingMessage unexported
bc6b655 
- change for loop semantics in (*StreamingConnectionManager).Run

Signed-off-by: Michael Stergianis <mstergianis@vmware.com>
@wwitzel3
explict module dep
626e0c0 
Signed-off-by: Wayne Witzel III <wayne@riotousliving.com>
This was referenced May 28, 2021
Closed
Closed
GuessWhoSamFoo
GuessWhoSamFoo reviewed Jun 3, 2021
View reviewed changes
internal/api/streaming_connection_errors.go
@@ -0,0 +1,22 @@
package api

type StreamError struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could go under internal/errors

jamieklassen
jamieklassen suggested changes Jun 4, 2021
View reviewed changes
Copy link
Contributor

@jamieklassen jamieklassen left a comment

Choose a reason for hiding this comment

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

requesting one change so that library consumers can effectively use this handy feature!

pkg/dash/dash.go
@@ -215,15 +216,23 @@ func WithClusterClient(client cluster.ClientInterface) RunnerOption {
}
}

func WithStreamingClientFactory(factory api.StreamingClientFactory) RunnerOption {
Copy link
Contributor

@jamieklassen jamieklassen Jun 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

the package api in this context is an internal package -- for library consumers to use this option the StreamingClientFactory type will need to be moved into a non-internal package please!

EDIT: similarly for the default websocket implementation, please

@mstergianis
Copy link

@wwitzel3 happy to resolve the above two requests. I'll send you a patch

@mstergianis
Copy link

mstergianis commented Jun 4, 2021 •
edited
Loading

@wwitzel3 in going through the exercise of resolving the requests; I realized that StreamError should also be part of pkg as a custom implementation should have the option to return a fatal stream error. Wanted to get your thoughts on moving this much implementation out of internal before I spend more time on it.

@GuessWhoSamFoo
Copy link
Contributor

Making StreamError public is fine - realistically this is only going to be consumed internally by folks at VMware.

Eventually, internal/errors should be public as well and can have a similar API to pkg/store so third party usage or tools can add warnings/errors that will be visible by UI.

@mstergianis
Copy link

Okay, sounds good to me. I'll set aside some time to do this today and get back to you with some patches

jamieklassen
jamieklassen suggested changes Jun 9, 2021
View reviewed changes
Copy link
Contributor

@jamieklassen jamieklassen left a comment

Choose a reason for hiding this comment

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

I'd like to suggest that we move the generation of IDs into the connection factory itself -- it reduces the surface area of the connection factory interface and allows custom implementations (like the one I'd like to write) to manage their own IDs.

internal/api/streaming_connection_manager.go Outdated Show resolved Hide resolved
internal/api/streaming_connection_manager.go Outdated Show resolved Hide resolved
internal/api/streaming_connection_manager.go Outdated Show resolved Hide resolved
internal/api/websocket_connection_factory.go Outdated Show resolved Hide resolved
internal/api/websocket_connection_factory.go Outdated Show resolved Hide resolved
@mstergianis
Copy link

Apologies, I made the changes requested above and forgot to update this thread.

I've made a PR against @wwitzel3's branch. The diffs ended up being quite large so I thought that the easiest way to make the change.

@wwitzel3
Apply suggestions from code review
58a6283 
Co-authored-by: Jamie Klassen <jklassen@vmware.com>
@wwitzel3 wwitzel3 mentioned this pull request Jun 22, 2021
Merged
@wwitzel3
Copy link
Contributor Author

replaced by #2581

@wwitzel3 wwitzel3 closed this Jun 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@GuessWhoSamFoo GuessWhoSamFoo GuessWhoSamFoo left review comments

@jamieklassen jamieklassen jamieklassen requested changes

@lenriquez lenriquez Awaiting requested review from lenriquez

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wwitzel3 @mstergianis @GuessWhoSamFoo @jamieklassen