You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/03/23 20:26:10 UTC
[GitHub] [beam] ayberk commented on a change in pull request #17162: [BEAM-14157] Don't request work on a closed windmill GetWorkStream
ayberk commented on a change in pull request #17162:
URL: https://github.com/apache/beam/pull/17162#discussion_r833692179
##########
File path: runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/windmill/GrpcWindmillServer.java
##########
@@ -936,7 +936,11 @@ protected void onResponse(StreamingGetWorkResponseChunk chunk) {
.execute(
() -> {
try {
- send(extension);
+ synchronized (this) {
+ if (!clientClosed.get()) {
Review comment:
+1 to throwing `IllegalStateException` if the stream is closed. Otherwise we risk having regressions by changing the behavior. It's safer to throw the exception for the purposes of yoru change.
##########
File path: runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/windmill/GrpcWindmillServer.java
##########
@@ -936,7 +936,11 @@ protected void onResponse(StreamingGetWorkResponseChunk chunk) {
.execute(
() -> {
try {
- send(extension);
+ synchronized (this) {
+ if (!clientClosed.get()) {
Review comment:
+1 to throwing `IllegalStateException` if the stream is closed. Otherwise we risk having regressions by changing the behavior. It's safer to throw the exception for the purposes of your change.
--
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-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org