acrites commented on code in PR #40639:
URL: https://github.com/apache/beam/pull/40639#discussion_r4405423752
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingKeyedWorkItemSideInputParDoFn.java:
##########
@@ -231,18 +226,13 @@ protected void
onProcessWindowedValue(WindowedValue<KeyedWorkItem<K, InputT>> el
}
if (hasState) {
helpers.registerStateCleanup(
- (WindowingStrategy<?, W>) getDoFnInfo().getWindowingStrategy(),
windowsProcessed);
+ (WindowingStrategy<?, W>) helpers.fnInfo.getWindowingStrategy(),
windowsProcessed);
}
}
- /**
- * Returns the {@link DoFnInfo} currently being used by this {@link
SimpleParDoFn}.
- *
- * <p>May be null if no element has been processed yet, or if the {@link
SimpleParDoFn} has
- * finished.
- */
- @VisibleForTesting
- @Nullable DoFnInfo<?, ?> getDoFnInfo() {
- return helpers.fnInfo;
+ /** Returns the {@link DoFnInfo} used by this {@link
StreamingKeyedWorkItemSideInputParDoFn}. */
Review Comment:
Yeah, I think I see what you're saying. One option would be to have separate
methods for *all* of the things on the underlying DoFn that are immutable, e.g.
WindowingStrategy. But that seems like a lot of extra boilerplate. Also, for
the DoFnSignature stuff, I need an instance of the DoFn object itself.
So maybe we go with a combination of what you've suggested: change
`helpers.fnInfo` to `helpers.activeFnInfo` to make it more clear that this is a
dynamic object. And then have `getDoFnInfo` and `getActiveDoFnInfo` with better
comments. `getActiveDoFnInfo` doesn't need to be part of the generic `ParDoFn`
interface.
--
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]
