Add histogram parsing in runner v2 #34017
Conversation
|
Run Java_IOs_Direct PreCommit |
|
Run Java PreCommit |
|
Run Java_Pulsar_IO_Direct PreCommit |
|
Run Python_Runners PreCommit 3.12 |
|
Run Java PreCommit |
|
Run Java_Pulsar_IO_Direct PreCommit |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @Abacn for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
R: @kennknowles |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
| repeated int64 bucket_counts = 3; | ||
|
|
||
| // Statistics for the underflow and overflow bucket. | ||
| message OutlierStats { |
There was a problem hiding this comment.
nit: harder to read the fields that exist when nested message definitions are interspersed. Do all the nested messages first or all nested message last.
There was a problem hiding this comment.
Moved the nested messages to the end.
There was a problem hiding this comment.
I have removed the outlier stats, since we don't currently use them either for encoding or decoding, so it simplifies this a bit.
| } | ||
|
|
||
| // Describes the bucket boundaries used in the histogram. | ||
| optional BucketOptions bucket_options = 2; |
There was a problem hiding this comment.
Is there any issue where the options might be incompatible across two metrics reports?
There was a problem hiding this comment.
This is solely for encoding or decoding, and each message for a metric report (keyed by the name) is separate, for consuming metrics downstream, where if there are two different histograms types, it can be problematic, and would be on that client to determine how to handle incompatibilities or how to aggregate the two.
Naireen
force-pushed
the
new_hist_parse_logic
branch
from
13e8ce1 to
e532162
Compare
|
Run Go PreCommit |
|
Run Prism_Python PreCommit 3.9 |
|
Run Java_IOs_Direct PreCommit |
|
Run Go PreCommit |
Naireen
force-pushed
the
new_hist_parse_logic
branch
from
e532162 to
46ae1e3
Compare
|
Run Kotlin_Examples PreCommit |
|
Run Java PreCommit |
1 similar comment
|
Run Java PreCommit |
This is a forward fix for #33761, where this was added in
java:corewith a dependency on thedataflow-runner. This adds it back in without that dependence.Addresses #33093
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.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)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.