kennknowles commented on code in PR #40850:
URL: https://github.com/apache/beam/pull/40850#discussion_r4404362710
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/util/common/worker/WorkProgressUpdater.java:
##########
@@ -315,7 +316,11 @@ private void reportProgress() {
throw oom;
} catch (InterruptedException e) {
LOG.info("Cancelling workitem execution: {}", workString(), e);
- worker.abort();
+ // the batch worker reports progress while the executor is still being
created, before
+ // there is a worker to abort
+ if (Objects.nonNull(worker)) {
Review Comment:
`Object.nonNull()` is more of a check for a series, so just using `!= null`
is fine here. (not that it matters much)
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/BatchDataflowWorker.java:
##########
Review Comment:
It would also be ideal to remove this. Funny, I thought I had got this done
for the whole worker/ subdirectory, but that must have been in the old findbugs
days, before checkerframework (which is much more rigorous)
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/util/common/worker/WorkProgressUpdater.java:
##########
Review Comment:
Can you remove this and see how it goes? Because honestly `worker` being set
to `null` is not done except in initialization and I guess in subclasses. In
this class it is set to different things but not null. (unless we also make
those parameters nullable, which is maybe what we need to do?)
The reason I make this comment is because of the new use of this field.
Ideally we (or our coding agent) will have a mechanical check of whether
nullness is treated correctly.
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/BatchDataflowWorker.java:
##########
@@ -286,14 +296,30 @@ boolean doWork(WorkItem workItem, WorkItemStatusClient
workItemStatusClient) thr
}
workItemStatusClient.setWorker(worker, executionContext);
- DataflowWorkProgressUpdater progressUpdater =
- new DataflowWorkProgressUpdater(workItemStatusClient, workItem,
worker, options);
- executeWork(worker, progressUpdater);
+ if (Objects.isNull(progressUpdater)) {
Review Comment:
How could this be null here?
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/DataflowWorkProgressUpdater.java:
##########
Review Comment:
Here too
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/WorkItemStatusClient.java:
##########
Review Comment:
Perhaps here too.
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/DataflowWorkProgressUpdater.java:
##########
Review Comment:
If it is not thread safe... curious to find locks in it!
--
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]
