Support bidirectional streaming on websockets #1111
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
also has first draft of working binding to handle dispatch
Pruning dead code too
niloc132
force-pushed
the
834-websocket-bidi-streaming
branch
from
d2cfc8a
to
61791e7
Compare
niloc132
force-pushed
the
834-websocket-bidi-streaming
branch
from
61791e7
to
da4e412
Compare
nbauernfeind
requested changes
Aug 24, 2021
grpc-api/src/main/java/io/deephaven/grpc_api/browserstreaming/BrowserStreamInterceptor.java
Outdated
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/browserstreaming/BrowserStreamInterceptor.java
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/util/GrpcServiceOverrideBuilder.java
Outdated
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/util/GrpcServiceOverrideBuilder.java
Outdated
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/util/GrpcServiceOverrideBuilder.java
Outdated
Show resolved
Hide resolved
web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java
Outdated
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/browserstreaming/BrowserStream.java
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/browserstreaming/BrowserStream.java
Outdated
Show resolved
Hide resolved
web/client-api/src/main/java/io/deephaven/web/client/api/barrage/stream/BiDiStream.java
Outdated
Show resolved
Hide resolved
* lots of unsynchronized observers * grrrreat! * bad null check in browser stream * slim down some generics
nbauernfeind
requested changes
Aug 25, 2021
grpc-api/src/main/java/io/deephaven/grpc_api/console/ConsoleServiceGrpcImpl.java
Outdated
Show resolved
Hide resolved
niloc132
commented
Aug 25, 2021
grpc-api/src/main/java/io/deephaven/grpc_api/arrow/ArrowFlightUtil.java
Outdated
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/runner/DeephavenApiServerModule.java
Outdated
Show resolved
Hide resolved
|
James has pointed out that we need a mechanism to inform the client that individual requests for completion items have failed, without failing the stream. We also need a way to match requests and responses on the stream in case they race, and suppress CompletionCancelled errors from the log. |
JamesXNelson
requested changes
Aug 26, 2021
grpc-api/src/main/java/io/deephaven/grpc_api/browserstreaming/BrowserStream.java
Outdated
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/browserstreaming/BrowserStreamInterceptor.java
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/console/ConsoleServiceGrpcImpl.java
Show resolved
Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/console/ConsoleServiceGrpcImpl.java
Outdated
Show resolved
Hide resolved
web/client-api/src/main/java/io/deephaven/ide/shared/IdeSession.java
Outdated
Show resolved
Hide resolved
web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java
Outdated
Show resolved
Hide resolved
A helper tag that will allow outstanding branches to merge from main with respect to the large style guide code application
JamesXNelson
previously approved these changes
Aug 26, 2021
nbauernfeind
previously approved these changes
Aug 26, 2021
A helper tag that will allow outstanding branches to merge from main with respect to the large style guide code application
niloc132
dismissed stale reviews from nbauernfeind and JamesXNelson
via
6c21de1
nbauernfeind
approved these changes
Aug 26, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fetch/h2-based grpc-web clients do not support bidirectional streaming at this time, but the websocket client we are using from https://github.com/improbable-eng/grpc-web/ does. This lets us keep a single websocket open instead of making many calls. The client has support now to abstract how a bidirectional stream is created, either by a single websocket, or by a pair of fetch calls.
Additionally, autocomplete was seeing problems that appeared related to the server-side grpc handling payloads out of order. These methods have been consolidated into a single bidirectional stream to ensure ordering is sane. The server now has support to only implement a bidirectional stream once, and mark the "open"/"next" rpc methods as part of that, and automatically have their requests enqueued to be handled serially.
Other improvements got slipped into this work as well, such as only interacting with autocomplete from the gRPC service and never from DB, so that dependency was cut (as well as the dependency on gRPC utils, which only existed to get jsr305 annotations into various projects).
The client is incomplete in this patch, we plan on continuing this work as part of #730.
Fixes #929
Fixes #834
Fixes #1049