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/06/21 19:48:14 UTC
[GitHub] [beam] davidyan74 opened a new pull request #12047: [BEAM-10291] Adding full thread dump upon lull detection
davidyan74 opened a new pull request #12047:
URL: https://github.com/apache/beam/pull/12047
Adding full thread dump upon lull detection to python fnapi sdk harness
R: @tvalentyn
------------------------
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
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](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] davidyan74 commented on pull request #12047: [BEAM-10291] Adding full thread dump upon lull detection
Posted by GitBox <gi...@apache.org>.
davidyan74 commented on pull request #12047:
URL: https://github.com/apache/beam/pull/12047#issuecomment-652022287
@tvalentyn Added a unit test. PTAL. Thank you!
----------------------------------------------------------------
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] aaltay commented on a change in pull request #12047: [BEAM-10291] Adding full thread dump upon lull detection
Posted by GitBox <gi...@apache.org>.
aaltay commented on a change in pull request #12047:
URL: https://github.com/apache/beam/pull/12047#discussion_r448524289
##########
File path: sdks/python/apache_beam/runners/worker/sdk_worker.py
##########
@@ -570,6 +576,23 @@ def _log_lull_in_bundle_processor(self, processor):
step_name_log,
stack_trace)
+ if self._should_log_full_thread_dump():
+ self._log_full_thread_dump()
+
+ def _should_log_full_thread_dump(self):
+ now = time.time()
+ if self._last_full_thread_dump_secs + LOG_LULL_FULL_THREAD_DUMP_S < now:
+ self._last_full_thread_dump_secs = now
+ return True
+ return False
+
+ def _log_full_thread_dump(self):
Review comment:
This is a nit. I think at this point, you can remove _log_full_thread_dump() and directly call thread_dump() instead.
----------------------------------------------------------------
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] davidyan74 commented on pull request #12047: [BEAM-10291] Adding full thread dump upon lull detection
Posted by GitBox <gi...@apache.org>.
davidyan74 commented on pull request #12047:
URL: https://github.com/apache/beam/pull/12047#issuecomment-648522264
The tests have been pending for a long time now. Is there anything wrong with it?
----------------------------------------------------------------
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] aaltay commented on a change in pull request #12047: [BEAM-10291] Adding full thread dump upon lull detection
Posted by GitBox <gi...@apache.org>.
aaltay commented on a change in pull request #12047:
URL: https://github.com/apache/beam/pull/12047#discussion_r448052626
##########
File path: sdks/python/apache_beam/runners/worker/sdk_worker.py
##########
@@ -570,6 +576,23 @@ def _log_lull_in_bundle_processor(self, processor):
step_name_log,
stack_trace)
+ if self._should_log_full_thread_dump():
+ self._log_full_thread_dump()
+
+ def _should_log_full_thread_dump(self):
+ now = time.time()
+ if self._last_full_thread_dump_secs + LOG_LULL_FULL_THREAD_DUMP_S < now:
+ self._last_full_thread_dump_secs = now
+ return True
+ return False
+
+ def _log_full_thread_dump(self):
Review comment:
Can we reuse thread_dump (from https://github.com/apache/beam/blob/master/sdks/python/apache_beam/runners/worker/worker_status.py#L37) ?
----------------------------------------------------------------
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] davidyan74 commented on pull request #12047: [BEAM-10291] Adding full thread dump upon lull detection
Posted by GitBox <gi...@apache.org>.
davidyan74 commented on pull request #12047:
URL: https://github.com/apache/beam/pull/12047#issuecomment-648522058
The original lull logging code does not have test coverage. I'll look into adding tests that cover both the original code and the added code in this PR. It will take me a while 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] tvalentyn commented on pull request #12047: [BEAM-10291] Adding full thread dump upon lull detection
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on pull request #12047:
URL: https://github.com/apache/beam/pull/12047#issuecomment-647765612
Thank you, @davidyan74 !
Does the added codepath have tests coverage? If not, could we add it?
Thank you.
----------------------------------------------------------------
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] davidyan74 commented on a change in pull request #12047: [BEAM-10291] Adding full thread dump upon lull detection
Posted by GitBox <gi...@apache.org>.
davidyan74 commented on a change in pull request #12047:
URL: https://github.com/apache/beam/pull/12047#discussion_r448058760
##########
File path: sdks/python/apache_beam/runners/worker/sdk_worker.py
##########
@@ -570,6 +576,23 @@ def _log_lull_in_bundle_processor(self, processor):
step_name_log,
stack_trace)
+ if self._should_log_full_thread_dump():
+ self._log_full_thread_dump()
+
+ def _should_log_full_thread_dump(self):
+ now = time.time()
+ if self._last_full_thread_dump_secs + LOG_LULL_FULL_THREAD_DUMP_S < now:
+ self._last_full_thread_dump_secs = now
+ return True
+ return False
+
+ def _log_full_thread_dump(self):
Review comment:
Thanks for the pointer. Done.
----------------------------------------------------------------
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] davidyan74 commented on a change in pull request #12047: [BEAM-10291] Adding full thread dump upon lull detection
Posted by GitBox <gi...@apache.org>.
davidyan74 commented on a change in pull request #12047:
URL: https://github.com/apache/beam/pull/12047#discussion_r448526068
##########
File path: sdks/python/apache_beam/runners/worker/sdk_worker.py
##########
@@ -570,6 +576,23 @@ def _log_lull_in_bundle_processor(self, processor):
step_name_log,
stack_trace)
+ if self._should_log_full_thread_dump():
+ self._log_full_thread_dump()
+
+ def _should_log_full_thread_dump(self):
+ now = time.time()
+ if self._last_full_thread_dump_secs + LOG_LULL_FULL_THREAD_DUMP_S < now:
+ self._last_full_thread_dump_secs = now
+ return True
+ return False
+
+ def _log_full_thread_dump(self):
Review comment:
Thanks! I'm using _log_full_thread_dump() as a mock in the unit test. I can mock thread_dump() instead there or leave it as is. Let me know your thoughts.
----------------------------------------------------------------
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] davidyan74 commented on pull request #12047: [BEAM-10291] Adding full thread dump upon lull detection
Posted by GitBox <gi...@apache.org>.
davidyan74 commented on pull request #12047:
URL: https://github.com/apache/beam/pull/12047#issuecomment-647699609
The 2 test failures in Python PreCommit do not seem to be related to the changes in this PR.
----------------------------------------------------------------
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] tvalentyn commented on pull request #12047: [BEAM-10291] Adding full thread dump upon lull detection
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on pull request #12047:
URL: https://github.com/apache/beam/pull/12047#issuecomment-652563755
Thank you, @davidyan74 , this is a very helpful addition.
----------------------------------------------------------------
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] tvalentyn commented on pull request #12047: [BEAM-10291] Adding full thread dump upon lull detection
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on pull request #12047:
URL: https://github.com/apache/beam/pull/12047#issuecomment-648526151
Sometimes Jenkins has glitches... not sure what is wrong in this case.
Thank you, tests would be helpful since postcommit tests will probably not exercise this codepath, since it requires a lull to happen. So if somebody adds a typo in that code, it may go undetected until we have to debug a slow customer job.
----------------------------------------------------------------
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] tvalentyn merged pull request #12047: [BEAM-10291] Adding full thread dump upon lull detection
Posted by GitBox <gi...@apache.org>.
tvalentyn merged pull request #12047:
URL: https://github.com/apache/beam/pull/12047
----------------------------------------------------------------
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