scwhittle commented on code in PR #35391:
URL: https://github.com/apache/beam/pull/35391#discussion_r2291731430
##########
sdks/python/apache_beam/runners/worker/sdk_worker_main.py:
##########
@@ -219,6 +224,14 @@ def main(unused_argv):
fn_log_handler.close()
+def flush_fn_log_handler():
+ """Flushes the FnApiLogRecordHandler if it exists."""
+ _LOGGER.error('The Sdk harness will be terminated now.')
+ if _FN_LOG_HANDLER:
+ _FN_LOG_HANDLER.close()
+ os._exit(1)
Review Comment:
not a python expert but it seems this doesn't call cleanup handlers, is that
something we want to do? do beam users install cleanup handlers?
@tvalentyn
##########
sdks/python/apache_beam/runners/worker/worker_status.py:
##########
@@ -252,12 +257,8 @@ def _log_lull_in_bundle_processor(self,
bundle_process_cache):
self._log_lull_sampler_info(info, instruction)
def _log_lull_sampler_info(self, sampler_info, instruction):
- if not self._passed_lull_timeout_since_last_log():
- return
- if (sampler_info and sampler_info.time_since_transition and
- sampler_info.time_since_transition > self.log_lull_timeout_ns):
+ if (sampler_info and sampler_info.time_since_transition):
Review Comment:
how about some quick checks to return from the function before the expensive
stack trace generation. We are going to be constructing the stack traces
pretty frequently after the removal of `if not
self._passed_lull_timeout_since_last_log():` and even previously we were
calculating them in cases we weren't actually logging them.
if (not sampler_info or not sampler_info.time_since_transition):
return
log_lull = self._passed_lull_timeout_since_last_log() and
sampler_info.time_since_transition > self.log_lull_timeout_ns
timeout_exceeded = self._element_processing_timeout_ns and
sampler_info.time_since_transition
> self._element_processing_timeout_ns
if (not log_lull or timeout_exceeded):
return
.. calculate the step/stack traces etc
if log_lull:
##########
sdks/python/apache_beam/runners/worker/sdk_worker_main.py:
##########
@@ -219,6 +224,14 @@ def main(unused_argv):
fn_log_handler.close()
+def flush_fn_log_handler():
Review Comment:
how abouut including the fact that this exits the process in the method name
so it is clearer at the callsites?
ie, flush_fn_log_handler_and_exit()
and update the method comment to say that it exits the process as well.
--
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
