kennknowles commented on a change in pull request #12994:
URL: https://github.com/apache/beam/pull/12994#discussion_r498998483
##########
File path:
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/util/common/worker/MapTaskExecutor.java
##########
@@ -146,13 +165,17 @@ public boolean supportsRestart() {
@Override
public void abort() {
// Signal the read loop to abort on the next record.
- // TODO: Also interrupt the execution thread.
for (Operation op : operations) {
Preconditions.checkState(op instanceof ReadOperation || op instanceof
ReceivingOperation);
if (op instanceof ReadOperation) {
((ReadOperation) op).abortReadLoop();
}
}
+ synchronized (this) {
+ if (currentExecutorThread != null) {
+ currentExecutorThread.interrupt();
Review comment:
Discussed offline: "user" code can mean many things, and there are some
common Java errors that this change could trigger:
1. User's (who usually won't check the bit) doing catch-all error handling
without separating out InterruptedException.
2. IO libraries (which may often check the bit) doing incomplete or
incorrect cleanup and being left in an inconsistent state or leaking resources.
These would ideally both be noticed and handled by higher-level mechanisms
since in both cases something should be notably unhealthy about the thread,
process, or VM. For now just aborting without interrupting the thread is safer.
----------------------------------------------------------------
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]
