ian-Liaozy commented on code in PR #36090:
URL: https://github.com/apache/beam/pull/36090#discussion_r2350096577
##########
sdks/python/apache_beam/transforms/core.py:
##########
@@ -1664,7 +1664,7 @@ def with_exception_handling(
subject to change.
"""
args, kwargs = self.raw_side_inputs
- return self.label >> _ExceptionHandlingWrapper(
+ wrapper = self.label >> _ExceptionHandlingWrapper(
Review Comment:
Hi @claudevdm , thanks for the suggestion on the way to handle resource hint
propagation with exception handling. I've considered this alternative (which
also proposed by the user who encountered the error), but I'm inclined to stick
with the current approach. My reasoning is based on the complexities introduced
by the with_exception_handling() transform:
1. Multiple `DoFn` Layers: The `with_exception_handling()` itself introduces
a new `DoFn` layer to wrap the original function.
2. Timeout Adds Another Layer: Furthermore, if the `timeout` parameter is
used within `with_exception_handling()`, this injects an additional `DoFn`
layer to manage the timeout logic.
3. Ordering Flexibility: The `_ExceptionHandlingWrapper` can be applied in
various orders relative to `with_resource_hints()`. The hints could be set on
the transform before exception handling is added, or on the result of
`with_exception_handling()`.
Given these factors, I want to make sure to propagate resource hint to the
actual execution function, regardless of how `with_exception_handling` and
`with_resource_hints` are combined. Also, my added unit test should be able to
cover both cases.
--
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]
