{"action":"created","issue":{"url":"https://api.github.com/repos/grpc/grpc/issues/6327","repository_url":"https://api.github.com/repos/grpc/grpc","labels_url":"https://api.github.com/repos/grpc/grpc/issues/6327/labels{/name}","comments_url":"https://api.github.com/repos/grpc/grpc/issues/6327/comments","events_url":"https://api.github.com/repos/grpc/grpc/issues/6327/events","html_url":"https://github.com/grpc/grpc/pull/6327","id":151528953,"number":6327,"title":"gRPC Python GA API (Draft)","user":{"login":"nathanielmanistaatgoogle","id":10131044,"avatar_url":"https://avatars.githubusercontent.com/u/10131044?v=3","gravatar_id":"","url":"https://api.github.com/users/nathanielmanistaatgoogle","html_url":"https://github.com/nathanielmanistaatgoogle","followers_url":"https://api.github.com/users/nathanielmanistaatgoogle/followers","following_url":"https://api.github.com/users/nathanielmanistaatgoogle/following{/other_user}","gists_url":"https://api.github.com/users/nathanielmanistaatgoogle/gists{/gist_id}","starred_url":"https://api.github.com/users/nathanielmanistaatgoogle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nathanielmanistaatgoogle/subscriptions","organizations_url":"https://api.github.com/users/nathanielmanistaatgoogle/orgs","repos_url":"https://api.github.com/users/nathanielmanistaatgoogle/repos","events_url":"https://api.github.com/users/nathanielmanistaatgoogle/events{/privacy}","received_events_url":"https://api.github.com/users/nathanielmanistaatgoogle/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/grpc/grpc/labels/cla:%20yes","name":"cla: yes","color":"009800"},{"url":"https://api.github.com/repos/grpc/grpc/labels/DO%20NOT%20MERGE","name":"DO NOT MERGE","color":"e11d21"},{"url":"https://api.github.com/repos/grpc/grpc/labels/python","name":"python","color":"2619ff"}],"state":"open","locked":false,"assignee":{"login":"ctiller","id":10120821,"avatar_url":"https://avatars.githubusercontent.com/u/10120821?v=3","gravatar_id":"","url":"https://api.github.com/users/ctiller","html_url":"https://github.com/ctiller","followers_url":"https://api.github.com/users/ctiller/followers","following_url":"https://api.github.com/users/ctiller/following{/other_user}","gists_url":"https://api.github.com/users/ctiller/gists{/gist_id}","starred_url":"https://api.github.com/users/ctiller/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ctiller/subscriptions","organizations_url":"https://api.github.com/users/ctiller/orgs","repos_url":"https://api.github.com/users/ctiller/repos","events_url":"https://api.github.com/users/ctiller/events{/privacy}","received_events_url":"https://api.github.com/users/ctiller/received_events","type":"User","site_admin":false},"milestone":null,"comments":3,"created_at":"2016-04-28T01:31:30Z","updated_at":"2016-05-16T13:09:09Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/grpc/grpc/pulls/6327","html_url":"https://github.com/grpc/grpc/pull/6327","diff_url":"https://github.com/grpc/grpc/pull/6327.diff","patch_url":"https://github.com/grpc/grpc/pull/6327.patch"},"body":"Lots of minutia to discuss here, but of course the most important question for this is whether or not it has all the growth surfaces that are needed and whether or not future changes are going to be as disruptive as past feature growth was to the alpha and beta APIs.\r\n\r\n(Security (mostly) not part of this code review.)\r\n\r\nChanges from Beta:\r\n* Channel construction now takes a single string rather than separate host and port.\r\n* RPC timeout now optional.\r\n* Server-side timeout removed - if applications don't trust the RPC caller and want to time out according to their own schedule, they'll have to set their own alarm clock.\r\n\r\nQuestions:\r\n* What about [Future](https://github.com/grpc/grpc/blob/master/src/python/grpcio/grpc/framework/foundation/future.py)? The problems with `concurrent.futures.Future` that motivated `grpc.framework.foundation.future.Future` haven't been solved. \"Maximizing compatibility\" with `concurrent.futures.Future` would mean compounding its design flaw _and_ violating the public API of `concurrent.futures` - please no? Is it a problem putting our `Future` interface where it is currently drafted in this API? Should `Future` appear as a code element of `grpc`, or should only code elements that we expect to be used in application code be code elements of `grpc`?\r\n* The MultiCallable interfaces are how we support asynchronous invocation on the same code element as synchronous invocation, but with the current semantics they're only needed for response-unary RPCs. Should we keep all four anyway with future growth in mind? If we want to [support `async`/`await`](https://github.com/grpc/grpc/issues/6046), they'd be the right place, right?\r\n* Should the `Channel` have the RPC invocation convenience methods depicted here or is it just enough to have the MultiCallable methods? How much do we expect users to make direct use of `Channel`s? How hard (and/or weird) are we making their lives if we make them write `my_response = my_channel.unary_unary(my_method_name)(my_request)` instead of `my_response = my_channel.blocking_unary_unary(my_method_name, my_request)`?\r\n* How should this API's documentation refer to connectivity and status code integers? Do we just say \"go look at connectivity_state.h and status.h to see what the integer returned to you means\"?\r\n* Is it too clever making the raised `RpcError` also a `Call` object? Should the `RpcError` have a `Call` object instead?\r\n* Should the required `Channel` construction parameter be named \"target\" or \"address\" or... something else? What exact wording would we like describing it?\r\n* What about \"channel args\"? How do we document them? At least one of them is only appropriate for use in tests - are we worried about it being passed via the non-test API?\r\n* Servicing RPCs requires a thread pool (right?). Should the server's thread pool be required of the application, optional, or entirely obscured? Should the application be allowed to pass thread pool size?\r\n* How do we want to support generic service of arbitrary RPCs? (This was present in beta; the question isn't coming out of nowhere.) By accepting a function that looks like a stream-stream handler, but that takes the method name as well and operates on `bytes` in and `bytes` out?\r\n* Where in the API do we want to allow applications to pass native strings as opposed to byte strings? (If we want to support `my_response = my_stub.my_method(my_request)` at the read-eval-print loop, we're going to have to support native strings in at least one place.)\r\n* Can we do away with the `metadata_transformer` present in alpha and beta and make security entirely done with the plug-in API? (I suspect yes?)\r\n* The `host` is a stub option in the beta API; should it remain a stub option or become a channel option or per-call option?\r\n* Where in the documentation do we want to say \"metadata\" and where do we want to say \"headers\"? What three terms do we want to say where the beta API said \"invocation metadata\", \"initial metadata\", and \"terminal metadata\"?\r\n* In the beta API there is a `CallOptions` class that can be passed on RPC invocation and that encapsulates disable_compression, subcall_of, and per-call credentials. Do we want to do away with this aggregate object and just have those values as keyword arguments on each RPC invocation (as is drafted here)? If so, is there any need to worry about every future feature being added as yet another keyword argument and eventually having an unmanageable number?\r\n* Are \"normal termination of the RPC\" and \"abnormal termination of the RPC\" the right terms to use?\r\n\r\n@soltanmm and @jtattermusch please also look? Thanks much."},"comment":{"url":"https://api.github.com/repos/grpc/grpc/issues/comments/219420661","html_url":"https://github.com/grpc/grpc/pull/6327#issuecomment-219420661","issue_url":"https://api.github.com/repos/grpc/grpc/issues/6327","id":219420661,"user":{"login":"nathanielmanistaatgoogle","id":10131044,"avatar_url":"https://avatars.githubusercontent.com/u/10131044?v=3","gravatar_id":"","url":"https://api.github.com/users/nathanielmanistaatgoogle","html_url":"https://github.com/nathanielmanistaatgoogle","followers_url":"https://api.github.com/users/nathanielmanistaatgoogle/followers","following_url":"https://api.github.com/users/nathanielmanistaatgoogle/following{/other_user}","gists_url":"https://api.github.com/users/nathanielmanistaatgoogle/gists{/gist_id}","starred_url":"https://api.github.com/users/nathanielmanistaatgoogle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nathanielmanistaatgoogle/subscriptions","organizations_url":"https://api.github.com/users/nathanielmanistaatgoogle/orgs","repos_url":"https://api.github.com/users/nathanielmanistaatgoogle/repos","events_url":"https://api.github.com/users/nathanielmanistaatgoogle/events{/privacy}","received_events_url":"https://api.github.com/users/nathanielmanistaatgoogle/received_events","type":"User","site_admin":false},"created_at":"2016-05-16T13:09:09Z","updated_at":"2016-05-16T13:09:09Z","body":"Lots more here to discuss and review now.\r\n\r\nHighlights:\r\n* Blocking unary-unary invocation is single-threaded and single-batch which should address all the glaring, obvious performance improvements for the use case and further gains would have to be measurement-based.\r\n* Asynchronous invocation uses a single shared-channel-wide completion queue so N asynchronously-invoked RPCs spawn one thread rather than N (or N times a constant) threads.\r\n* A started server runs one thread. Servicing RPCs consumes one thread per active RPC, taken from a thread pool supplied by the application.\r\n* I think the tests cover a lot.\r\n  * Details is neither used nor verified.\r\n  * Custom status codes are neither used nor verified.\r\n  * Metadata is used but not yet verified.\r\n  * I could stand to write some stream-length-zero tests in addition to the stream-length-many tests\r\n  * But other than that...\r\n\r\nQuestions:\r\n* Do we (gRPC Python) **have** to make a call to Core for every call that the application invokes on us? In the current draft if the application invokes a request-unary RPC and passes a request object that fails to serialize we fake having called ([and we conform to status code semantics for the case](https://github.com/grpc/grpc/blob/master/doc/statuscodes.md)) without ever touching Core. Is this legitimate?\r\n* The current draft of the server is one-completion-queue-per-server-plus-one-completion-queue-per-active-RPC. I had wanted to go full-on one-completion-queue-per-server, but after the discussion in [issue 6597](https://github.com/grpc/grpc/issues/6597) I now believe it's impossible to write a multithreaded server with a single completion queue (I have an analysis written up elsewhere if it's not obvious why). Am I wrong? If I'm not wrong, am I the only one who cares? If I'm the only one who cares I'll just get over myself.\r\n* Is there too much code here (@jtattermusch rightly brought this question up with me about the beta implementation a while back)? The `__init__.py` file can be as long as it needs to be because it's mostly documentation, but are seven hundred lines for `_channel.py` and six hundred lines for `_server.py` in line with the weight of the features being added? Are there obvious opportunities for reduction that I've overlooked?\r\n* `GenericHandler` and `ParticularHandler` are terrible names, but for the moment they get the point across, and we don't have anything more proper listed in either glossary. `RpcHandler` and `MethodHandler`, maybe?\r\n* In the server code I saw some memory leaks clear up when I moved the polling on the per-RPC completion queue inside the per-RPC lock. Why was that? Shouldn't any thread be able to poll on any completion queue? (I'm also open to being wrong about what I saw.)\r\n* Four of my test methods still leak memory, according to what the Core prints at the end of the test run - the ones that use asynchronous invocation (and thus on the client side a completion queue on a different thread) and complete with non-OK status. What could be going on there?\r\n* My apologies that at the moment the implementation comments are sparse and the specification comments are absent, but what's unexpected in the code? Where have I gotten \"creative\" in ways that are dangerous or defective?"}}