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.

Sign up for GitHub

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

Jump to bottom

Report total active threads #28513

Merged
merged 16 commits into from
Oct 2, 2023
Merged

Report total active threads #28513

merged 16 commits into from
Oct 2, 2023

Conversation

edman124
Copy link
Contributor

These metrics are needed as a part of an internal project for thread scaling.
Total number of active threads will be joined with worker cpu utilization to see if we are bounded by the # of parallel work items, ultimately to reduce worker idleness.

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

  • Mention the appropriate issue in your description (for example: 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, comment fixes #<ISSUE NUMBER> instead.
  • 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
Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

@edman124 edman124 marked this pull request as draft September 18, 2023 23:36
@edman124 edman124 marked this pull request as ready for review September 18, 2023 23:37
@edman124
Copy link
Contributor Author

ptal @twang126

twang126
twang126 reviewed Sep 18, 2023
View reviewed changes
Copy link

@twang126 twang126 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

pls take a quick look and undo the changes that are just formatting, etc. (there's more than just the parts I commented at)

...va/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorker.java Outdated Show resolved Hide resolved
...va/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorker.java Outdated Show resolved Hide resolved
...va/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorker.java Outdated Show resolved Hide resolved
...va/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorker.java Outdated Show resolved Hide resolved
...va/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorker.java Outdated Show resolved Hide resolved
...orker/src/test/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorkerTest.java Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

@edman124
Copy link
Contributor Author

Run Java PreCommit

@github-actions
Copy link
Contributor

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

R: @damccorm 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).

@edman124
Copy link
Contributor Author

ptal @bvolpato

bvolpato
bvolpato suggested changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@bvolpato bvolpato left a comment

Choose a reason for hiding this comment

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

In general LGTM, shared suggestion to make max active threads more useful

@edman124
Copy link
Contributor Author

ptal @damccorm should be ready to merge.

bvolpato
bvolpato approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@bvolpato bvolpato left a comment

Choose a reason for hiding this comment

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

LGTM, just get rid of some debugging / personal information if possible, please

@bvolpato
Copy link
Contributor

Please ./gradlew spotlessApply

damccorm
damccorm reviewed Sep 29, 2023
View reviewed changes
...orker/src/test/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorkerTest.java Outdated
ShardedKey key1Shard1 = ShardedKey.create(ByteString.copyFromUtf8("key1"), 1);

// real work
MockActiveWork m1 =
Copy link
Contributor

Choose a reason for hiding this comment

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

The test is now failing to build, please address the precommit failures (e.g. https://github.com/apache/beam/actions/runs/6344373926/job/17234456430?pr=28513)

@damccorm
Copy link
Contributor

damccorm commented Oct 2, 2023

Run Java PreCommit

damccorm
damccorm approved these changes Oct 2, 2023
View reviewed changes
Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

Thanks

@damccorm damccorm merged commit bf1e829 into apache:master Oct 2, 2023
17 checks passed
@Abacn Abacn mentioned this pull request Oct 19, 2023
Merged
3 tasks
Abacn
Abacn reviewed Oct 19, 2023
View reviewed changes
...orker/src/test/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorkerTest.java
executor.execute(m2, m2.getWorkItem().getSerializedSize());
this.wait();
// Seems current executor executes the initial work item twice
this.wait();
Copy link
Contributor

Choose a reason for hiding this comment

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

this may have caused #28957

Copy link
Contributor Author

@edman124 edman124 Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It could be the first thread started runs before the synchronized block starts causing a deadlock. I have a fix in #29041 that uses countdown latches.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix. While #29041 is in progress, would you mind lgtm #29065 so the current stuck test won't affect the whole test suite so that it can still finish?

This was referenced Oct 27, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrmccluskey jrmccluskey jrmccluskey left review comments

@Abacn Abacn Abacn left review comments

@damccorm damccorm damccorm approved these changes

@twang126 twang126 twang126 approved these changes

@bvolpato bvolpato bvolpato approved these changes

Assignees
No one assigned
Labels
dataflow Next Action: Reviewers runners
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@edman124 @bvolpato @damccorm @Abacn @twang126 @jrmccluskey