Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BEAM-14557] Read and Seek Runner Capabilities in Go SDK #17821

Merged
merged 6 commits into from
Jun 14, 2022

Conversation

riteshghorse
Copy link
Contributor

@riteshghorse riteshghorse commented Jun 2, 2022

Closes #21807
This PR provides parity with Java and Python SDK to read the runner capabilities and use it to optimize/support additional features/utilities. This PR focuses on supporting monitoring info with short ID.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Add a link to the appropriate issue in your description, 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.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@asf-ci
Copy link

asf-ci commented Jun 2, 2022

Can one of the admins verify this patch?

2 similar comments
@asf-ci
Copy link

asf-ci commented Jun 2, 2022

Can one of the admins verify this patch?

@asf-ci
Copy link

asf-ci commented Jun 2, 2022

Can one of the admins verify this patch?

@github-actions github-actions bot added the go label Jun 2, 2022
@riteshghorse
Copy link
Contributor Author

R: @lostluck

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2022

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #17821 (e943b6b) into master (56755f5) will increase coverage by 0.00%.
The diff coverage is 2.43%.

@@           Coverage Diff           @@
##           master   #17821   +/-   ##
=======================================
  Coverage   74.09%   74.09%           
=======================================
  Files         697      697           
  Lines       91986    92068   +82     
=======================================
+ Hits        68154    68216   +62     
- Misses      22583    22601   +18     
- Partials     1249     1251    +2     
Flag Coverage Δ
go 50.90% <2.43%> (+0.07%) ⬆️

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

Impacted Files Coverage Δ
sdks/go/pkg/beam/core/runtime/harness/harness.go 10.28% <0.00%> (-0.18%) ⬇️
...dks/go/pkg/beam/core/runtime/harness/monitoring.go 23.12% <0.00%> (-1.22%) ⬇️
sdks/go/pkg/beam/core/runtime/graphx/translate.go 43.19% <100.00%> (+0.05%) ⬆️
sdks/go/pkg/beam/testing/passert/sum.go 100.00% <0.00%> (ø)
...ks/go/pkg/beam/runners/dataflow/dataflowlib/job.go 21.55% <0.00%> (ø)
...o/pkg/beam/runners/dataflow/dataflowlib/execute.go 0.00% <0.00%> (ø)
sdks/go/pkg/beam/runners/dataflow/dataflow.go 58.65% <0.00%> (+0.03%) ⬆️
sdks/go/pkg/beam/testing/passert/passert.go 81.11% <0.00%> (+2.36%) ⬆️
sdks/go/pkg/beam/testing/passert/count.go 79.16% <0.00%> (+2.97%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56755f5...e943b6b. Read the comment docs.

Copy link
Contributor

@lostluck lostluck left a comment

Choose a reason for hiding this comment

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

Woohoo! LGTM

@lostluck
Copy link
Contributor

lostluck commented Jun 2, 2022

Run Go PostCommit

@lostluck lostluck changed the title Remove monitoring infos [BEAM-14557] Remove monitoring infos Jun 2, 2022
@riteshghorse
Copy link
Contributor Author

Okay, it looks like the universal runner extracts metrics from MonitoringInfos for producing the pipeline result. Need to change that as well.

@riteshghorse
Copy link
Contributor Author

The portable and flink runners are not requesting req.MonitoringInfos(). That's the reason they are not getting any monitoring infos with the new approach.

@github-actions github-actions bot added the docker label Jun 7, 2022
@lostluck
Copy link
Contributor

lostluck commented Jun 9, 2022

As discussed, we can't remove the monitoring infos due to other runners, but I have found the correct URNs we need to look out for and probably provide as a capability:

"beam:protocol:monitoring_info_short_ids:v1"

MONITORING_INFO_SHORT_IDS = 0 [(beam_urn) = "beam:protocol:monitoring_info_short_ids:v1"];

@lostluck
Copy link
Contributor

Since the scope of the PR has changed, could the Title and PR Description be updated accordingly?

@riteshghorse riteshghorse changed the title [BEAM-14557] Remove monitoring infos [BEAM-14557] Read and Seek Runner Capabilities in Go SDK Jun 10, 2022
@lostluck
Copy link
Contributor

retest this please

@lostluck
Copy link
Contributor

https://ci-beam.apache.org/job/beam_PostCommit_Go_PR/438/ passed! Merging this.

Copy link
Contributor

@lostluck lostluck left a comment

Choose a reason for hiding this comment

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

LGTM

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.

[Task]: Go SDK should use runner capabilities to decide whether to send monitoring info with short ID
3 participants