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

Proposed gRPC, Java server API for bidirectional stream widgets #4226

Merged
merged 86 commits into from
Aug 10, 2023

Conversation

niloc132
Copy link
Member

@niloc132 niloc132 commented Jul 24, 2023

Suggested continuation of #4086.

A lot of naming in this branch isn't ideal, some that I've changed and some that I've left as is - I'll take a first pass at reviewing it myself to leave some notes to other readers. Part of the problem with some of this is that it is deliberately generic - we know that the stream holds "data" and supports other "plugins" to stream their own kinds of data, but that's about all that we can clearly state about it.

I've tried to start by defining the gRPC interface a little more clearly though, and clearly describe what it does for the user, and what it doesn't. At least for reasons of handling gRPC metadata, it would not be a great choice for a gRPC transport mechanism, and will at best look like a "custom gRPC wiring for deephaven", rather than something more generic. Continuing in that theme, I made the (reversible) decision not to support half-close at the plugin level, assuming that if a plugin requires such a feature, that it can implement it itself.

Rather than a boolean for "is bidirectional", I've added an enum for this, in case we find that the protocol needs to evolve further in the future. It might make sense to include an enum in the proto as well, so that older clients can still talk to newer servers, if the protocol changes again - but for this revision, I'm deprecating FetchObject, and encouraging MessageStream to replace it.

Streamed exports have been changed slightly in the name of API ease of use:

  • Rather than let the export collector track objects over the lifetime of the stream (potentially leaking objects, requiring our stream protocol to understand releasing tickets), each single response from the plugin will have its own array of exports.
  • A plugin implementation need not request a Writer+ExportCollector pair, then progressively write the payload, and close/release both when finished, but can invoke a single method, stream.write(bytes, objects).

This change has a few other effects:

  • Plugin implementations that want to reference objects sent in past responses need to take care in their own format to describe which instances should the client not release (or perhaps better, indicate "if you still have a reference to X, you can release it now, here's a new export for it").
  • Rather than letting implementations specify allowUnknownType and forceNew, the above stream.write(bytes, objects) method will specify these. For now, I've specified allowUnknownType as true, and forceNew as true, leaving both as implementation details for the plugin itself ("be sure all required plugins are installed", and "if you want to de-dup references, do it in your own implementation first" respectively).

Naming strawman:

  • MessageStream is the "modernized" impl of FetchObject - it can fetch (that is, one connect request, one data response), but it can also support bidirectional streaming.
  • A StreamRequest (perhaps MessageRequest?) can of type Connect or Data. Leaning towards renaming StreamRequest.payload to be type? A StreamResponse can only be Data at this time.
  • A Data message contains payload bytes, and typed_export_ids typed tickets. We continue to qualify this with export, only export tickets should be used/allowed.
  • The MessageStream java interface currently has an onMessage method, but should probably be renamed to onData. Should we have two interfaces, to avoid that initial confusion that a gRPC-java user has, with StreamObserver being used to both send and receive? If not, should close() be renamed to onClose() to match?

@@ -22,5 +82,45 @@ message FetchObjectRequest {
message FetchObjectResponse {
string type = 1;
bytes data = 2;
repeated io.deephaven.proto.backplane.grpc.TypedTicket typed_export_id = 3;
repeated io.deephaven.proto.backplane.grpc.TypedTicket typed_export_ids = 3;
Copy link
Member Author

Choose a reason for hiding this comment

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

this is technically not a wire format change, but trying to fix a typo that we have. It break compiled code, where someone is consuming the protoc generated jars and building their own impl, but I think in this context we should be safe, as only js clients use this at this time.

chipkent
chipkent previously approved these changes Aug 9, 2023
Comment on lines +2 to +3
deephaven.registry.imageName=python:3.10
deephaven.registry.imageId=python@sha256:74cdd039dc36f6476dd5dfdbc187830e0c0f760a1bdfc73d186060ef4c4bd78f
Copy link
Member

Choose a reason for hiding this comment

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

I don't have a problem w/ this; assuming it was necessary for python client testing? I'm assuming this doesn't have anything to do w/ the server itself, b/c we have a min version of 3.8 there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, 3.7 is EOL, minimum requirement is 3.8, so we brought this up to 3.10, same as our server image uses:
https://github.com/deephaven/deephaven-server-docker/blob/1b31ee1ca7686a5e45a448a648714e3cef0fc1cd/server.hcl#L45-L47

Comment on lines +79 to +80
def is_fetch_only(self) -> bool:
return isinstance(self._user_object_type, FetchOnlyObjectType)
Copy link
Member

Choose a reason for hiding this comment

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

I don't know how important it is; but arguably, we could also check if it has a to_bytes method and it might work against existing plugins.

Copy link
Member Author

Choose a reason for hiding this comment

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

The fact that we also moved Callback, and also took away the default False values for the exporter methods makes me suspect that most existing plugins will need an update anyway.

devinrsmith
devinrsmith previously approved these changes Aug 10, 2023
Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

No blocking comments from me, happy to proceed.

chipkent
chipkent previously approved these changes Aug 10, 2023
rcaudy
rcaudy previously approved these changes Aug 10, 2023
@niloc132 niloc132 dismissed stale reviews from rcaudy, chipkent, and devinrsmith via 9fd19d9 August 10, 2023 18:54
rcaudy
rcaudy previously approved these changes Aug 10, 2023
@niloc132 niloc132 merged commit b6628d0 into deephaven:main Aug 10, 2023
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 10, 2023
@deephaven-internal
Copy link
Contributor

Labels indicate documentation is required. Issues for documentation have been opened:

How-to: https://github.com/deephaven/deephaven.io/issues/3026
Reference: https://github.com/deephaven/deephaven.io/issues/3025

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants