You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/07/13 18:19:10 UTC

[GitHub] [beam] lukecwik opened a new pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

lukecwik opened a new pull request #12241:
URL: https://github.com/apache/beam/pull/12241


   This will also be important to have this lock when splitting/getting progress for window observing splittable DoFns.
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
    - [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
    - [ ] Update `CHANGES.md` with noteworthy changes.
    - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
   
   Post-Commit Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
   --- | --- | --- | --- | --- | --- | ---
   Go | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
   Java | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
   Python | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
   XLang | --- | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
   
   Pre-Commit Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   --- |Java | Python | Go | Website
   --- | --- | --- | --- | ---
   Non-portable | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
   Portable | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
   
   See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] lukecwik commented on pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #12241:
URL: https://github.com/apache/beam/pull/12241#issuecomment-658243038


   Run Python2_PVR_Flink PreCommit


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] boyuanzz commented on pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #12241:
URL: https://github.com/apache/beam/pull/12241#issuecomment-657960214


   Could you please explain more about the race condition here?


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] lukecwik commented on pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #12241:
URL: https://github.com/apache/beam/pull/12241#issuecomment-658269143


   Run Python2_PVR_Flink PreCommit


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] boyuanzz commented on a change in pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
boyuanzz commented on a change in pull request #12241:
URL: https://github.com/apache/beam/pull/12241#discussion_r454528596



##########
File path: sdks/python/apache_beam/runners/common.py
##########
@@ -842,29 +847,37 @@ def _invoke_process_per_window(self,
 
   def try_split(self, fraction):
     # type: (...) -> Optional[Tuple[SplitResultPrimary, SplitResultResidual]]
-    if self.threadsafe_restriction_tracker and self.current_windowed_value:
+    if not self.is_splittable:
+      return None
+
+    with self.splitting_lock:
+      # Make a local reference to member variables that change references during
+      # processing under lock before attempting to split so we have a consistent
+      # view of all the references.
+      current_windowed_value = self.current_windowed_value
+      threadsafe_restriction_tracker = self.threadsafe_restriction_tracker
+      threadsafe_watermark_estimator = self.threadsafe_watermark_estimator
+
+    if threadsafe_restriction_tracker:

Review comment:
       I would add a comment above to state the assumption. But it's minor though.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] boyuanzz commented on pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #12241:
URL: https://github.com/apache/beam/pull/12241#issuecomment-658295343


   > > Could you please explain more about the race condition here?
   > 
   > threadsafe_watermark_estimator can become None in the finally block after the `if threadsafe_restriction_tracker:` check has passed in `try_split` which will lead to a `NoneType doesn't have a get_estimator_state method` error.
   
   Thanks.


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] lukecwik commented on a change in pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #12241:
URL: https://github.com/apache/beam/pull/12241#discussion_r454532875



##########
File path: sdks/python/apache_beam/runners/common.py
##########
@@ -842,29 +847,37 @@ def _invoke_process_per_window(self,
 
   def try_split(self, fraction):
     # type: (...) -> Optional[Tuple[SplitResultPrimary, SplitResultResidual]]
-    if self.threadsafe_restriction_tracker and self.current_windowed_value:
+    if not self.is_splittable:
+      return None
+
+    with self.splitting_lock:
+      # Make a local reference to member variables that change references during
+      # processing under lock before attempting to split so we have a consistent
+      # view of all the references.
+      current_windowed_value = self.current_windowed_value
+      threadsafe_restriction_tracker = self.threadsafe_restriction_tracker
+      threadsafe_watermark_estimator = self.threadsafe_watermark_estimator
+
+    if threadsafe_restriction_tracker:

Review comment:
       I have a much larger rewrite here to support per window invocation where this will become a non-issue so I'll pass on this since we will need to hold the lock for the entire split call.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] boyuanzz commented on a change in pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
boyuanzz commented on a change in pull request #12241:
URL: https://github.com/apache/beam/pull/12241#discussion_r454510310



##########
File path: sdks/python/apache_beam/runners/common.py
##########
@@ -842,29 +847,37 @@ def _invoke_process_per_window(self,
 
   def try_split(self, fraction):
     # type: (...) -> Optional[Tuple[SplitResultPrimary, SplitResultResidual]]
-    if self.threadsafe_restriction_tracker and self.current_windowed_value:
+    if not self.is_splittable:
+      return None
+
+    with self.splitting_lock:
+      # Make a local reference to member variables that change references during
+      # processing under lock before attempting to split so we have a consistent
+      # view of all the references.
+      current_windowed_value = self.current_windowed_value

Review comment:
       I think I misunderstood the logic here, deep-copy is incorrect. So within the lock, why do we need to keep a local reference?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] lukecwik commented on a change in pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #12241:
URL: https://github.com/apache/beam/pull/12241#discussion_r454532100



##########
File path: sdks/python/apache_beam/runners/common.py
##########
@@ -842,29 +847,37 @@ def _invoke_process_per_window(self,
 
   def try_split(self, fraction):
     # type: (...) -> Optional[Tuple[SplitResultPrimary, SplitResultResidual]]
-    if self.threadsafe_restriction_tracker and self.current_windowed_value:
+    if not self.is_splittable:
+      return None
+
+    with self.splitting_lock:
+      # Make a local reference to member variables that change references during
+      # processing under lock before attempting to split so we have a consistent
+      # view of all the references.
+      current_windowed_value = self.current_windowed_value

Review comment:
       The lock ends before the `if` since the `if` indentation level is not at the same level as `with lock` statement.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] lukecwik edited a comment on pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
lukecwik edited a comment on pull request #12241:
URL: https://github.com/apache/beam/pull/12241#issuecomment-658244351






----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] lukecwik commented on a change in pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #12241:
URL: https://github.com/apache/beam/pull/12241#discussion_r454518717



##########
File path: sdks/python/apache_beam/runners/common.py
##########
@@ -842,29 +847,37 @@ def _invoke_process_per_window(self,
 
   def try_split(self, fraction):
     # type: (...) -> Optional[Tuple[SplitResultPrimary, SplitResultResidual]]
-    if self.threadsafe_restriction_tracker and self.current_windowed_value:
+    if not self.is_splittable:
+      return None
+
+    with self.splitting_lock:

Review comment:
       Not deep copying objects.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] lukecwik commented on pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #12241:
URL: https://github.com/apache/beam/pull/12241#issuecomment-658294246


   Run Python2_PVR_Flink PreCommit


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] boyuanzz commented on a change in pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
boyuanzz commented on a change in pull request #12241:
URL: https://github.com/apache/beam/pull/12241#discussion_r454525609



##########
File path: sdks/python/apache_beam/runners/common.py
##########
@@ -842,29 +847,37 @@ def _invoke_process_per_window(self,
 
   def try_split(self, fraction):
     # type: (...) -> Optional[Tuple[SplitResultPrimary, SplitResultResidual]]
-    if self.threadsafe_restriction_tracker and self.current_windowed_value:
+    if not self.is_splittable:
+      return None
+
+    with self.splitting_lock:
+      # Make a local reference to member variables that change references during
+      # processing under lock before attempting to split so we have a consistent
+      # view of all the references.
+      current_windowed_value = self.current_windowed_value

Review comment:
       Any reason that local references are need here, even within the lock?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] lukecwik commented on a change in pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #12241:
URL: https://github.com/apache/beam/pull/12241#discussion_r454518553



##########
File path: sdks/python/apache_beam/runners/common.py
##########
@@ -842,29 +847,37 @@ def _invoke_process_per_window(self,
 
   def try_split(self, fraction):
     # type: (...) -> Optional[Tuple[SplitResultPrimary, SplitResultResidual]]
-    if self.threadsafe_restriction_tracker and self.current_windowed_value:
+    if not self.is_splittable:
+      return None
+
+    with self.splitting_lock:
+      # Make a local reference to member variables that change references during
+      # processing under lock before attempting to split so we have a consistent
+      # view of all the references.
+      current_windowed_value = self.current_windowed_value
+      threadsafe_restriction_tracker = self.threadsafe_restriction_tracker
+      threadsafe_watermark_estimator = self.threadsafe_watermark_estimator
+
+    if threadsafe_restriction_tracker:

Review comment:
       There is an assumption that if one is set then the other is set. Similarly, if one is unset then the other is unset.

##########
File path: sdks/python/apache_beam/runners/common.py
##########
@@ -842,29 +847,37 @@ def _invoke_process_per_window(self,
 
   def try_split(self, fraction):
     # type: (...) -> Optional[Tuple[SplitResultPrimary, SplitResultResidual]]
-    if self.threadsafe_restriction_tracker and self.current_windowed_value:
+    if not self.is_splittable:
+      return None
+
+    with self.splitting_lock:
+      # Make a local reference to member variables that change references during
+      # processing under lock before attempting to split so we have a consistent
+      # view of all the references.
+      current_windowed_value = self.current_windowed_value

Review comment:
       The purpose of the lock is to get a consistent point in time copy of the references to the current objects.
   
   We don't need a deep copy. Splitting will fail if the restriction becomes finished and the main processing loop moves onto the next element.
   




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] lukecwik commented on pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #12241:
URL: https://github.com/apache/beam/pull/12241#issuecomment-658244351


   > Could you please explain more about the race condition here?
   
   threadsafe_watermark_estimator can become None in the finally block after the `if threadsafe_restriction_tracker:` check has passed which will lead to a `NoneType doesn't have a get_estimator_state` method.


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] boyuanzz commented on a change in pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
boyuanzz commented on a change in pull request #12241:
URL: https://github.com/apache/beam/pull/12241#discussion_r454504904



##########
File path: sdks/python/apache_beam/runners/common.py
##########
@@ -842,29 +847,37 @@ def _invoke_process_per_window(self,
 
   def try_split(self, fraction):
     # type: (...) -> Optional[Tuple[SplitResultPrimary, SplitResultResidual]]
-    if self.threadsafe_restriction_tracker and self.current_windowed_value:
+    if not self.is_splittable:
+      return None
+
+    with self.splitting_lock:
+      # Make a local reference to member variables that change references during
+      # processing under lock before attempting to split so we have a consistent
+      # view of all the references.
+      current_windowed_value = self.current_windowed_value

Review comment:
       Are you trying to deep-copy `current_windowed_value`, `threadsafe_restriction_tracker ` and `threadsafe_watermark_estimator `? If so, we need to do it explicitly `copy.deepcopy`. 

##########
File path: sdks/python/apache_beam/runners/common.py
##########
@@ -842,29 +847,37 @@ def _invoke_process_per_window(self,
 
   def try_split(self, fraction):
     # type: (...) -> Optional[Tuple[SplitResultPrimary, SplitResultResidual]]
-    if self.threadsafe_restriction_tracker and self.current_windowed_value:
+    if not self.is_splittable:
+      return None
+
+    with self.splitting_lock:

Review comment:
       If we are deep copying objects, it seems like we can use the lock to guard the copying logic only, instead of the entire split logic.

##########
File path: sdks/python/apache_beam/runners/common.py
##########
@@ -842,29 +847,37 @@ def _invoke_process_per_window(self,
 
   def try_split(self, fraction):
     # type: (...) -> Optional[Tuple[SplitResultPrimary, SplitResultResidual]]
-    if self.threadsafe_restriction_tracker and self.current_windowed_value:
+    if not self.is_splittable:
+      return None
+
+    with self.splitting_lock:
+      # Make a local reference to member variables that change references during
+      # processing under lock before attempting to split so we have a consistent
+      # view of all the references.
+      current_windowed_value = self.current_windowed_value
+      threadsafe_restriction_tracker = self.threadsafe_restriction_tracker
+      threadsafe_watermark_estimator = self.threadsafe_watermark_estimator
+
+    if threadsafe_restriction_tracker:

Review comment:
       Would it be better to both check `threadsafe_restriction_tracker ` and `threadsafe_watermark_estimator` for easy reading?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] lukecwik merged pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
lukecwik merged pull request #12241:
URL: https://github.com/apache/beam/pull/12241


   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] lukecwik commented on pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #12241:
URL: https://github.com/apache/beam/pull/12241#issuecomment-657716085


   R: @boyuanzz 


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [beam] lukecwik commented on pull request #12241: [BEAM-10420] Fix minor race condition related to splitting that will cause None has no method 'yyy'

Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #12241:
URL: https://github.com/apache/beam/pull/12241#issuecomment-658393808


   Run Python2_PVR_Flink PreCommit


----------------------------------------------------------------
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:
users@infra.apache.org