On Tue, Dec 12, 2017 at 10:28 PM, Michael Lumish <[email protected]> wrote:
> Thank you for your input. > > Regarding the Protobuf.js changes, one of the primary goals of this API > change is to decouple gRPC from Protobuf.js as much as possible, because > the existing tight coupling has prevented us from upgrading the dependency > from Protobuf.js 5 to 6 without breaking the API. In addition, gRPC is > serialization format agnostic, so we do not want users to need to use the > service definition types from one specific serialization library. On top of > that, there is some complexity in transforming the information in a > Protobuf.js Service object into the information gRPC needs to run a client > or server, so it is simpler for gRPC to expose an API with a service > definition type that matches that information. > I knew that gRPC is serialization agnostic, I just never saw anyone using anything else than protobuf. Decoupling from protobufjs would also entail writing a gRPC IDL parser, I guess. > Adding "abortShutdown" and "unbind" functions is currently infeasible > because the underlying API that the library is implemented on doesn't have > that kind of granularity. However, this does indicate that the names of the > existing functions may need to be changed. The function "tryShutdown" is > really "unbind and stop accepting incoming calls, and notify when all > ongoing calls have terminated". So, "try" isn't really accurate, because it > doesn't actually fail. And "forceShutdown" is really "unbind and stop > accepting incoming calls, and terminate all ongoing calls immediately". > As far as I see, the Node.js socket API used by the pure JS version does not have it either. I would suggest something like shutdownImmediately() and shutdownGraceful() then. Alternatively, one could mirror Node's http2session.shutdown() method (but maybe return a promise instead of taking a callback). > Regarding the connection and disconnection hooks, having the API expose > calls and not connections is a deliberate design choice. We explicitly do > not support per-client state. Part of the reason for this is that the > semantics of gRPC do not correspond exactly to the semantics of HTTP/2, in > some ways that would cause unexpected outcomes for people trying to use > this kind of per-connection state. In particular, gRPC clients do automatic > reconnection, which means that the "same client" may be multiple > connections, and gRPC clients also do client-side load balancing, which > means that a single client may send different requests to different > servers. Basically, there are no guarantees that per-connection state would > persist for the lifetime of a client, or that any particular request would > be made to a server that already has that state. The solution to these is > to include that state in call metadata or in a database that can be > referenced using call metadata. > I see. I don't absolute need the hooks, it was just one of the first use case that occurred to me, but I understand that it is actually not a good idea. But having the number of currently connected sockets would still be beneficial as a metric for a load balancer. I think something like Node's server.getConnections() would suffice. > I am interested in the "API layering" suggestion, but I'm not exactly sure > what it would entail. Can you expand a bit on what this API that you are > suggesting might look like? I am particularly interested in what a unary > call and a bidirectional streaming call would look like. > I have written an rxjs adapter for the current API: https://github.com/ anfema/grpc-node-rxjs It is still just a proof of concept, but the tests contain an example for the client and server implementation. It also contains a plugin for my TypeScript definition generator: https://github.com/ anfema/grpc-code-generator The API currently looks something like this: // server handler // return types are wrapped in promises, so handler functions can be async interface Service { unaryCall(request: RequestType, call: Call): Promise<ResponseType> streamResponse(request: RequestType, call: Call): Promise<Observable<ResponseType>>; streamRequest(requestStream: Observable<RequestType>, call: Call): Promise<ResponseType>; streamBidi(requestStream: Observable<RequestType>, call: Call): Promise<Observable<ResponseType>>; } // the 'Call' object wraps the original call/stream and exposes metadata but _not_ IO related information interface Call { getPeer(): string; requestMetadata(): Metadata; sendResponseMetadata(responseMetadata: Metadata): void; } // client interface Client { unaryCall(request: RequestType): Observable<ResponseType>; streamResponse(request: RequestType): Observable<ResponseType>; streamRequest(requestStream: Observable<RequestType>): Observable<ResponseType>; streamBidi(requestStream: Observable<RequestType>): Observable<ResponseType>; } // unary call client.unaryCall({ request: "data"}, metadata, options) .subscribe( (response) => { /* */ }, (error) => { /* */ }, () => { /* stream will complete after receiving one response */ } ); // bidirectional streaming call const request = Observable.range(0, 3).map(n => { counter: n }); client.streamRequest(request) .subscribe( (response) => { /* */ }, (error) => { /* */ }, () => { /* */ } ); Best, André > On Mon, Dec 11, 2017 at 3:08 AM <[email protected]> wrote: > >> # Protobuf.js-related Changes >> >> - protobuf.js can already parse and reflect upon service definitions. >> Can't we just reuse this functionality and pass the `Service` reflection >> object to the main API? >> >> # Extend server API >> - Add a method `Server.abortShutdown()` to complement >> `Server.tryShutdown()` >> >> Reason: a server might decide it is overloaded and is not able to process >> more clients, but that state is transient and might be remedied in the >> future. >> >> `Server.tryShutdown()` should not unbind from the server port. There >> should be a complementary method `Server.unbind()` to `Server.bind()`. >> >> - Per client connect and disconnect hooks >> >> The server API should provide per client connection and disconnection >> hooks, that are called with a server side `Connection` object (naming up >> for discussion). >> >> Reason: A service might want to implement per connection state >> (authenticated, etc…), or simply keep track of connected clients (eg. for >> load statistics or connecting to per client backend services). >> >> - The server side `Connection` object should provide a method to send >> `GOAWAY` and `PING` frames (and maybe other advanced functionality). >> >> # API layering >> >> Since I do not expect the current callback/streams based API to change >> anytime soon, it should be modified a little bit to enable an easier >> layering of different call styles on top of it. I am thinking here of >> promises, generators and rxjs style APIs. >> >> For example, the object `ServerWriteableStream` conflates three distinct >> entities into one: >> >> - The `request` object from the client. >> - Metadata about the call (cancelled state, client metadata, peer info). >> - A `Writable` stream for sending responses and completing/aborting the >> call. >> >> If these were provided as separate objects to the handler function, >> different APIs could be layered more cleanly on top of it. For rxjs for >> example, a `ServerWriteableStream` could be driven from an `Observer` of an >> `Observable` provided from a higher level API, while the request and >> metadata objects could be passed up unmodified. >> >> Best, >> André >> >> -- You received this message because you are subscribed to the Google Groups "grpc.io" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at https://groups.google.com/group/grpc-io. To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/CA%2BFfjcd1izxV7X6KdjFO_Nqfwyi1rCxsvDbhPkveCKsPqr-%3D%3Dw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
