jduo commented on pull request #8476: URL: https://github.com/apache/arrow/pull/8476#issuecomment-710777211
> > LGTM, one minor typo - and is it worth adding a test? (Though this is a very thin wrapper around gRPC.) > > Thanks. I'm looking to repurpose some existing tests that loop on isReady. - I've created a BackpressureStrategy interface and a simple callback-based implementation. - I've used this to repurpose the tests in TestBackPressure to test both polling (existing) and callback-based solutions. - New tests in TestBackpressure are marked Ignored since the tests they were based on were also ignored due to flakiness. I ran them locally though and they passed with similar performance. - I needed to change getStream() to run in a background thread. Currently it gets run in the onHalfClosed callback handler in gRPC. In the test implementations, getStream() is a synchronous implementation which prevented the new callback handler from running. In practice, I'd expect real world getStream() implementations to be written asynchronously though, so perhaps we should change our test implementations of getStream() to be asynchronous instead. This line of code eventually calls doGet, which must complete before onReady is called (thus, if doGet blocks until a notification from onReady, it never completes. But polling on isReady does): https://github.com/grpc/grpc-java/blob/0b6f29371bd96614fdbdcd3638d4bb6312258da3/stub/src/main/java/io/grpc/stub/ServerCalls.java#L182 ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
