Separate data streaming from websockets #1905
Conversation
|
any plans to keep driving this forward? |
|
Yes sorry, I was focused on other things and forgot about this. I'll schedule some time this week to clean it up a bit and submit as a formal PR. Thanks |
mstergianis
force-pushed
the
grpc-hacking
branch
2 times, most recently
from
51708ba to
65f5f25
Compare
Signed-off-by: Michael Stergianis <mstergianis@vmware.com>
mstergianis
force-pushed
the
grpc-hacking
branch
from
65f5f25 to
8474e37
Compare
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>
|
Can we create an issue that describes the intended use case of this? Technically this looks fine, but what is the reason we want to implement this change. The general idea being, running Octant with some other streaming mechanism other than web sockets? I know we talked about it before, but have forget that context and forgot to capture it anywhere. @mstergianis @jamieklassen |
mstergianis
force-pushed
the
grpc-hacking
branch
2 times, most recently
from
b57f775 to
ecef710
Compare
- change for loop semantics in (*StreamingConnectionManager).Run Signed-off-by: Michael Stergianis <mstergianis@vmware.com>
mstergianis
force-pushed
the
grpc-hacking
branch
from
ecef710 to
be225f2
Compare
|
Sorry this has taken so long, I'm still thinking it over a bit and trying to understand the implications of this. Please do let me know if this is blocking anything on your end and I can make it a priority to spend some more time with it. I appreciate your patience and understand that having a PR sitting open and having to rebase to keep it up to date is frustrating and that isn't my intention here. |
|
All good. I'll hold off on rebasing until it is (maybe) approved. Not blocking anything at the moment |
|
Closing this out, but don't be scared, I rebased this and fixed up in #2504 so we can merge it! |
Signed-off-by: Michael Stergianis mstergianis@vmware.com
What this PR does / why we need it: Enable octant to swap out its websocket implementation for something else.
Which issue(s) this PR fixes
Special notes for your reviewer:
Release note: