aaltay commented on a change in pull request #15003:
URL: https://github.com/apache/beam/pull/15003#discussion_r652024252



##########
File path: CHANGES.md
##########
@@ -103,10 +103,13 @@
   it was ignored if provided.
 * `TopCombineFn` disallow `compare` as its argument (Python) 
([BEAM-7372](https://issues.apache.org/jira/browse/BEAM-7372)).
 * Drop support for Flink 1.10 
([BEAM-12281](https://issues.apache.org/jira/browse/BEAM-12281)).
+* Custom implementations of `TriggerFn` now require that `may_lose_data` be 
implemented ([BEAM-9487](https://issues.apache.org/jira/browse/BEAM-9487)).

Review comment:
       Same backward compatibility question applies. Would this break any 
existing TriggerFn's out there. 
   
   And this is also a bit more severe then just adding a flag to workaround, 
because it will require code changes. Often times that is very hard and 
sometimes not possible. (E.g. An existing template where users do not have the 
sources easily accessible.) This type of change might be better waiting for 
Beam 3.0 (breaking changes) or happen very slowly.

##########
File path: sdks/python/apache_beam/transforms/ptransform_test.py
##########
@@ -476,6 +477,7 @@ def test_group_by_key(self):
       result = pcoll | 'Group' >> beam.GroupByKey() | _SortLists
       assert_that(result, equal_to([(1, [1, 2, 3]), (2, [1, 2]), (3, [1])]))
 
+  @skip('Behavior not available until Beam 2.33')

Review comment:
       Can you set the flag manually here to avoid skipping?




-- 
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.

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to