feat: DH-19901: Use grpc-java in JS API#7878
Conversation
niloc132
added
jsapi
grpc
NoDocumentationNeeded
NoReleaseNotesNeeded
No docs changes detected for 485deb9 |
|
Marked as a draft for now, I have a few outstanding work items to complete, but all integration tests pass and local websocket use seems to function.
|
| ref.setTicket(op.getSource().makeTicket()); | ||
| } else { | ||
| // every subsequent pb op references the entry before it | ||
| ref.setBatchOffset(send.length + internalOffset); |
There was a problem hiding this comment.
You've done testing to make this work, but rather than spend more minutes puzzling over why we don't need to include the size of the list in our batch offset anymore I figured I could just ask.
There was a problem hiding this comment.
So it works out that a lot of this machinery is never actually used - the client sends batches when appropriate, but doesn't actually use the offset feature at all via BatchBuilder, but specifies tickets for each node in the DAG. That lets the client explicitly retain exports and rebuild partway as it believes it is appropriate, rather than rely on memoization and refcount/gc.
| aggColumns.forEach((p0, p1) -> { | ||
| aggColumns.forEach((p0) -> { | ||
| String colName = p0.split("=")[0].trim(); | ||
| customColumns.push(colName + "= `` + " + colName); |
There was a problem hiding this comment.
Why are we toStringing the columns here? I know it isn't new, but I am certainly suspicious.
There was a problem hiding this comment.
It looks like I'm throwing Ryan under the bus here - but this is just for the post-agg display. Now that we can support arrays of just about any kind, we should be able to remove that extra step.
| return workerConnection.newState((c, state, metadata) -> { | ||
| AsOfJoinTablesRequest request = new AsOfJoinTablesRequest(); | ||
| return workerConnection.newState((c, state) -> { | ||
| AsOfJoinTablesRequest.Builder request = AsOfJoinTablesRequest.newBuilder(); |
There was a problem hiding this comment.
Maybe not a terrible time to do https://deephaven.atlassian.net/browse/DH-19188
If not in this PR as a simple follow-on?
There was a problem hiding this comment.
Reopened, will do this and the agg cleanup(s) in a followup.
| DomGlobal.fetch(options.url, init).then(response -> { | ||
| if (!abortController.signal.aborted) { | ||
| options.onHeaders.onHeaders(readHeaders(response.headers), response.status); | ||
| assert response.body != null : "Browser should always include a response body"; |
There was a problem hiding this comment.
This assert seems to contradict the response.body comment on line 66
There was a problem hiding this comment.
Replaced with better comments and an if just in case browsers do decide to someday follow the spec.
This is in part a backport from DHE, bringing grpc-java and protobuf-java support to the JS API. Removes the need to load dh-internal.js by compiling the Java protos and grpc services to JS, allowing easier code reuse and sharing, supporting more gRPC features, and removing the need to work around limitations around longs, headers, etc. Some of these changes will allow us to simplify DHE to better share code after this merges and is released.
Some grpc-java semantics are not exactly the same as in a JVM - Context's threadlocals won't play nicely with browser Promises for example, so must be explicitly propagated when used.
More cleanup is possible than has been done here - BiDiStream should no longer be necessary, but can just be a normal pair of StreamObservers. All of web/client-backplane can be deleted, but that would substantially bloat this patch, so I've left it out for now.