Re: [PR] [yaml] propogate error_handling to windowing composite config [beam]

[via GitHub]

robertwb merged PR #33443:
URL: https://github.com/apache/beam/pull/33443


-- 
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: github-unsubscr...@beam.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [yaml] propogate error_handling to windowing composite config [beam]

[via GitHub]

Polber commented on code in PR #33443:
URL: https://github.com/apache/beam/pull/33443#discussion_r1896113490


##
sdks/python/apache_beam/yaml/yaml_transform.py:
##
@@ -771,6 +771,9 @@ def preprocess_windowing(spec):
 'transforms': [modified_spec] + windowing_transforms,
 'input': spec['input'],
 'output': modified_spec['__uuid__'],
+'config': {
+'error_handling': spec.get('config').get('error_handling', None)

Review Comment:
   Agreed with both



-- 
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: github-unsubscr...@beam.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [yaml] propogate error_handling to windowing composite config [beam]

[via GitHub]

github-actions[bot] commented on PR #33443:
URL: https://github.com/apache/beam/pull/33443#issuecomment-2560271866

   Stopping reviewer notifications for this pull request: review requested by 
someone other than the bot, ceding control. If you'd like to restart, comment 
`assign set of reviewers`


-- 
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: github-unsubscr...@beam.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [yaml] propogate error_handling to windowing composite config [beam]

[via GitHub]

Polber commented on code in PR #33443:
URL: https://github.com/apache/beam/pull/33443#discussion_r1896111354


##
sdks/python/apache_beam/yaml/yaml_transform.py:
##
@@ -771,6 +771,9 @@ def preprocess_windowing(spec):
 'transforms': [modified_spec] + windowing_transforms,
 'input': spec['input'],
 'output': modified_spec['__uuid__'],
+'config': {
+'error_handling': spec.get('config').get('error_handling', None)
+} or {},

Review Comment:
   Good point



-- 
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: github-unsubscr...@beam.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [yaml] propogate error_handling to windowing composite config [beam]

[via GitHub]

robertwb commented on code in PR #33443:
URL: https://github.com/apache/beam/pull/33443#discussion_r1896108315


##
sdks/python/apache_beam/yaml/yaml_transform.py:
##
@@ -771,6 +771,9 @@ def preprocess_windowing(spec):
 'transforms': [modified_spec] + windowing_transforms,
 'input': spec['input'],
 'output': modified_spec['__uuid__'],
+'config': {
+'error_handling': spec.get('config').get('error_handling', None)
+} or {},

Review Comment:
   I don't think the `or {}` will ever trigger, as this will always result in a 
key with a single dictionary entry. However, setting letting 
error_handling=None should be fine. 



##
sdks/python/apache_beam/yaml/yaml_transform.py:
##
@@ -805,6 +808,9 @@ def preprocess_windowing(spec):
 'name': spec.get('name', None) or spec['type'],
 'transforms': [modified_spec] + windowing_transforms,
 'output': windowed_outputs,
+'config': {

Review Comment:
   Same as above.



##
sdks/python/apache_beam/yaml/yaml_transform.py:
##
@@ -771,6 +771,9 @@ def preprocess_windowing(spec):
 'transforms': [modified_spec] + windowing_transforms,
 'input': spec['input'],
 'output': modified_spec['__uuid__'],
+'config': {
+'error_handling': spec.get('config').get('error_handling', None)

Review Comment:
   Nit, let's use `spec['config'].get('error_handling', None)` for consistency 
(unless we're worried the spec might not have a config, in which case we'd want 
`spec.get('config', {})` (which I think we'd need as it happens before the 
`ensure_config` phase).
   
   Also, should it be `get('error_handling', {})` rather than 
`get('error_handling', None)`



-- 
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: github-unsubscr...@beam.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [yaml] propogate error_handling to windowing composite config [beam]

[via GitHub]

Polber commented on PR #33443:
URL: https://github.com/apache/beam/pull/33443#issuecomment-2560270656

   R: @robertwb 


-- 
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: github-unsubscr...@beam.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org