Eliaaazzz commented on code in PR #39367:
URL: https://github.com/apache/beam/pull/39367#discussion_r3886140478
##########
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/artifact/ArtifactStagingService.java:
##########
@@ -433,6 +455,14 @@ public synchronized void
onNext(ArtifactApi.ArtifactResponseWrapper responseWrap
}
}
+ private void putChunk(ByteString chunk) throws Exception {
Review Comment:
Agreed on keeping the common path untouched; reverted both call sites to the
plain `put` and removed the helper.
While verifying the revert I found that the plain `put` alone leaves one
stall: if the client fills the 100-slot queue before the `StoreArtifact` task
fails, nothing drains the queue afterwards, and the producer blocks inside
`put` with the observer lock held. The new test hits this window when the
executor is slow. A 300ms delay at the top of `StoreArtifact.call` reproduces
it every time, with the stream thread parked in `ArrayBlockingQueue.put` inside
`onNext` and the test timing out.
So the guard is now a single line on the error path: `StoreArtifact`'s catch
clears the queue after setting the exception. A blocked producer gets freed and
its next `aquire` fails; the healthy path is untouched. With the same delay
probe in place the test then passes in well under a second. Happy to adjust if
you'd prefer another shape.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
