Redesign cygrpc.Operation#13852
Conversation
|
This PR is against v1.8.x, not master. Is that intentional? (I'm asking since this sounds like a big change and I imagine 1.9 will be cut pretty soon too) |
|
Yes, it's intentional; I'd much rather have it upmerged than have to backport it. :-) It's not as large as the numbers suggest; a lot of it is reconfiguration rather than new behavior. (Of course, ideally there's zero new non-test behavior in this commit...) |
|
I agree that upmerge is preferable to backport, but my suggestion was to leave it out of 1.8.x entirely, especially since 1.8 to 1.9 timespan will likely be shorter than usual—if there's no clear bug-fix benefit to 1.8.x users, there's always a risk of some destabilization in a point release, but it's ultimately your call. |
|
I'd like it on |
There was a problem hiding this comment.
Default signedness of char type is implementation-defined in C (I'm assuming Cython is the same). Please make unsignedness explicit i.e. ctypedef unsigned char uint8_t (unless Cython disallows it for some reason?)
There was a problem hiding this comment.
Done; thanks for the catch.
It is now a family of classes conforming to an interface rather than a single class no single instance of which makes use of all behavior scoped to the class. It also now only uses gRPC Core memory for the time of a single batch rather than for the entire lifetime of the instance.
nathanielmanistaatgoogle
force-pushed
the
12531
branch
from
6389a60 to
81edf5f
Compare
|
(Reminder for adding the appropriate entry to pony's saddlebag that might be necessary for the new cython file when upmerged to master) |
2 participants
Further progress toward issue 12531.