gemini-code-assist[bot] commented on code in PR #35757:
URL: https://github.com/apache/beam/pull/35757#discussion_r2246251721
##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnApiDoFnRunner.java:
##########
@@ -1180,18 +1180,18 @@ private HandlesSplits.SplitResult
trySplitForElementAndRestriction(
splitResult.getWindowSplit(),
PTransformTranslation.SPLITTABLE_PROCESS_SIZED_ELEMENTS_AND_RESTRICTIONS_URN
+ "/GetSize");
+ Coder fullInputCoder = WindowedValues.getFullCoder(inputCoder,
windowCoder);
+ return constructSplitResult(
+ windowedSplitResult,
+ null,
+ fullInputCoder,
+ initialWatermark,
+ watermarkAndState,
+ pTransformId,
+ mainInputId,
+ pTransform.getOutputsMap().keySet(),
+ resumeDelay);
}
Review Comment:
Moving the `Coder fullInputCoder = WindowedValues.getFullCoder(inputCoder,
windowCoder);` and `constructSplitResult` calls inside the synchronized block
correctly fixes the race condition on `initialWatermark`. However, this
increases the time the lock is held, as `constructSplitResult` performs
encoding which can be CPU-intensive. This might impact performance if splits
are frequent. An alternative to minimize lock contention would be to copy
`initialWatermark` and other necessary parameters to local variables inside the
`synchronized` block and then call `constructSplitResult` outside the block.
This would fix the race condition without extending the lock duration as much.
Since this would require changes outside the current diff, feel free to keep
this as is if the performance impact is acceptable, or address it in a
follow-up.
--
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]
