-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Report Outstanding Bundles and Bytes #29041
Conversation
…efactor metric unit tests
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
...orker/src/test/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorkerTest.java
Outdated
Show resolved
Hide resolved
...orker/src/test/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorkerTest.java
Outdated
Show resolved
Hide resolved
...orker/src/test/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorkerTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General nit: can you add a bit more color in the description about:
- What is a bundle. Why is it important
- What is this bytes referring to. Is it backlog, queued, etc?
...va/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorker.java
Show resolved
Hide resolved
Run Java PreCommit |
ptal: @bvolpato |
assign set of reviewers |
Assigning reviewers. If you would like to opt out of this review, comment R: @AnandInguva added as fallback since no labels match configuration Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Hi, what is the status of this PR? @edman124 @m-trieu @twang126 It was noted in #29065 (comment) that this PR will fix a flaky test of Java PreCommit that seen quite frequently. We may need this before release -- current Java PreCommit is too flaky |
This flakiness is recent, right? Was there a change that caused it to become flaky? (could be fine but we might want to check) |
It is OK to rollback a change that went in that does not have reliable tests, too. |
Yes. I suggest if this was not made into release cut we should revert #28513 |
I think this PR is ready, @m-trieu can you double check and give a LGTM if everything looks good. I think the PR should be good to submit after approval |
LGTM |
thanks, as long as there is no merging conflict rebase is not needed, or is there any recent commit to master branch would affect this PR? Otherwise I will just merge this PR |
I don't think there are any conflicts |
@Abacn or I will merge it once tests are green. |
These metrics are needed as a part of an internal project for thread scaling.
The outstanding bundles and bytes will provide signals for how we want to autoscale the workers. The outstanding bundles tracks how many bundles of work are currently being processed (currently very similar to number of active threads). The outstanding bytes tracks how many bytes are currently being processed. These two can be compared to the respective limits to see if the pipeline needs to be scaled up or down.
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.md
with 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.