m-trieu commented on code in PR #32774:
URL: https://github.com/apache/beam/pull/32774#discussion_r1806018397
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/windmill/client/grpc/GrpcGetDataStream.java:
##########
@@ -302,38 +331,57 @@ public void appendSpecificHtml(PrintWriter writer) {
}
private <ResponseT> ResponseT issueRequest(QueuedRequest request,
ParseFn<ResponseT> parseFn) {
- while (true) {
+ while (!isShutdown()) {
request.resetResponseStream();
try {
queueRequestAndWait(request);
return parseFn.parse(request.getResponseStream());
- } catch (CancellationException e) {
- // Retry issuing the request since the response stream was cancelled.
- continue;
+ } catch (AppendableInputStream.InvalidInputStreamStateException
+ | VerifyException
Review Comment:
If it is thrown and we are shutdown, the verify exception is invalid because
any ongoing responses/requests are invalid. We clear the `pending` +
`requests` fields so some of the map operations are looking for things that are
no longer there.
An alternative is instead of just verify, we add a check for isShutdown
ontop of the boolean condition being checked and either return or throw if
isShutdown. The issue here is we start checking for isShutdown every vs
letting the exception just get thrown and handled here.
If the stream is not shutdown, the VerifyException will still be thrown. I
can add a test for this?
--
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.
To unsubscribe, e-mail: github-unsubscr...@beam.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
