Skip to content

Conversation

@tvalentyn
Copy link
Contributor

Log when elements for an instruction are missing in data plane more than 5 minutes. This can help surface processing inefficiency between an SDK and a runner.

@github-actions
Copy link
Contributor

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @jrmccluskey added as fallback since no labels match configuration

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@github-actions
Copy link
Contributor

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @jrmccluskey added as fallback since no labels match configuration

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@tvalentyn tvalentyn force-pushed the logging_data_plane branch 2 times, most recently from 5d01350 to cdbe280 Compare November 11, 2023 02:08
@tvalentyn
Copy link
Contributor Author

Run Python_PVR_Flink PreCommit

@tvalentyn
Copy link
Contributor Author

Run Python_Coverage PreCommit

@tvalentyn
Copy link
Contributor Author

Run PythonDocker PreCommit

@tvalentyn
Copy link
Contributor Author

cc: @robertwb

@tvalentyn tvalentyn changed the title Add logging in dataplane code when processing is stuck. Add logging in data plane code when processing is stuck. Nov 14, 2023
@tvalentyn tvalentyn changed the title Add logging in data plane code when processing is stuck. Add logging in data plane code when processing is stuck waiting on data for an instruction. Nov 14, 2023
@codecov
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (c713425) 38.34% compared to head (545f44e) 38.33%.
Report is 23 commits behind head on master.

Files Patch % Lines
...ks/python/apache_beam/runners/worker/data_plane.py 55.55% 4 Missing ⚠️
sdks/python/apache_beam/transforms/external.py 0.00% 4 Missing ⚠️
sdks/python/apache_beam/dataframe/expressions.py 0.00% 3 Missing ⚠️
sdks/python/apache_beam/runners/worker/logger.py 0.00% 3 Missing ⚠️
...beam/transforms/fully_qualified_named_transform.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #29399      +/-   ##
==========================================
- Coverage   38.34%   38.33%   -0.01%     
==========================================
  Files         693      694       +1     
  Lines      102199   102265      +66     
==========================================
+ Hits        39185    39204      +19     
- Misses      61422    61469      +47     
  Partials     1592     1592              
Flag Coverage Δ
python 29.88% <22.72%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

'Waiting to receive elements in input queue '
'for instruction: %s for %.2f seconds.',
instruction_id,
current_time - start_time)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add log_interval_sec in the log and mention this frequency?
Was this tested with Dataflow jobs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yes I tested it on dataflow to make sure there were no typos; it was a modified version of the PR to trigger the branches without waiting for 5 min.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add log_interval_sec in the log and mention this frequency?

done

Copy link
Contributor

@jrmccluskey jrmccluskey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but +1 to @liferoad's question asking if this had been tested yet

@tvalentyn tvalentyn merged commit 427faae into apache:master Nov 15, 2023
@tvalentyn tvalentyn deleted the logging_data_plane branch November 15, 2023 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants