You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "clmccart (via GitHub)" <gi...@apache.org> on 2024/01/10 23:12:15 UTC
[PR] make getProcessingDistributionsForWorkId synchronized [beam]
clmccart opened a new pull request, #29979:
URL: https://github.com/apache/beam/pull/29979
make getProcessingDistributionsForWorkId synchronized to avoid race conditions with remove tracker
the race condition happens in getProcessingDistributionsForWorkId because we check for the existence of the work id in the activeTrackersByWorkId map before retrieving the value from the map. if another thread removes the tracker from the map in-between us checking for its existence and actually grabbing the object, we will get a null tracker that then throws a NullPointerException when we try to access data on it.
Was able to repro this with a chaos test locally and confirmed that synchronizing the getProcessingDistributionsForWorkId method solves the issue. Decided to not commit the chaos test since it's a bit messy and deviates from test patterns in this repo.
------------------------
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](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] make getProcessingDistributionsForWorkId synchronized [beam]
Posted by "clmccart (via GitHub)" <gi...@apache.org>.
clmccart commented on PR #29979:
URL: https://github.com/apache/beam/pull/29979#issuecomment-1887549068
#29987
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] make getProcessingDistributionsForWorkId synchronized [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #29979:
URL: https://github.com/apache/beam/pull/29979#issuecomment-1889446486
Thanks for explanation. Would you mind add a brief comment in code to indicate the need of synchronized?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] make getProcessingDistributionsForWorkId threadsafe [beam]
Posted by "lostluck (via GitHub)" <gi...@apache.org>.
lostluck merged PR #29979:
URL: https://github.com/apache/beam/pull/29979
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] make getProcessingDistributionsForWorkId synchronized [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #29979:
URL: https://github.com/apache/beam/pull/29979#discussion_r1450923209
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/DataflowExecutionStateSampler.java:
##########
@@ -117,7 +117,8 @@ public Optional<ActiveMessageMetadata> getActiveMessageMetadataForWorkId(String
return Optional.ofNullable(null);
}
- public Map<String, IntSummaryStatistics> getProcessingDistributionsForWorkId(String workId) {
+ public synchronized Map<String, IntSummaryStatistics> getProcessingDistributionsForWorkId(
+ String workId) {
if (!activeTrackersByWorkId.containsKey(workId)) {
Review Comment:
```suggestion
// synchronized to avoid race condition of accessing activeTrackersByWorkId
if (!activeTrackersByWorkId.containsKey(workId)) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] make getProcessingDistributionsForWorkId threadsafe [beam]
Posted by "clmccart (via GitHub)" <gi...@apache.org>.
clmccart commented on code in PR #29979:
URL: https://github.com/apache/beam/pull/29979#discussion_r1470031258
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/DataflowExecutionStateSampler.java:
##########
@@ -117,7 +117,8 @@ public Optional<ActiveMessageMetadata> getActiveMessageMetadataForWorkId(String
return Optional.ofNullable(null);
}
- public Map<String, IntSummaryStatistics> getProcessingDistributionsForWorkId(String workId) {
+ public synchronized Map<String, IntSummaryStatistics> getProcessingDistributionsForWorkId(
Review Comment:
yeah i like that better. done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] make getProcessingDistributionsForWorkId synchronized [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #29979:
URL: https://github.com/apache/beam/pull/29979#discussion_r1450923209
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/DataflowExecutionStateSampler.java:
##########
@@ -117,7 +117,8 @@ public Optional<ActiveMessageMetadata> getActiveMessageMetadataForWorkId(String
return Optional.ofNullable(null);
}
- public Map<String, IntSummaryStatistics> getProcessingDistributionsForWorkId(String workId) {
+ public synchronized Map<String, IntSummaryStatistics> getProcessingDistributionsForWorkId(
+ String workId) {
if (!activeTrackersByWorkId.containsKey(workId)) {
Review Comment:
```suggestion
// synchronized to avoid race condition of accessing activeTrackersByWorkId
if (!activeTrackersByWorkId.containsKey(workId)) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] make getProcessingDistributionsForWorkId synchronized [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #29979:
URL: https://github.com/apache/beam/pull/29979#issuecomment-1885924985
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @Abacn 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] make getProcessingDistributionsForWorkId synchronized [beam]
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle commented on code in PR #29979:
URL: https://github.com/apache/beam/pull/29979#discussion_r1457952149
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/DataflowExecutionStateSampler.java:
##########
@@ -117,7 +117,8 @@ public Optional<ActiveMessageMetadata> getActiveMessageMetadataForWorkId(String
return Optional.ofNullable(null);
}
- public Map<String, IntSummaryStatistics> getProcessingDistributionsForWorkId(String workId) {
+ public synchronized Map<String, IntSummaryStatistics> getProcessingDistributionsForWorkId(
Review Comment:
since removeTracker isn't synchronized it still seems like the race is possible.
An alternative since the activeTrackersByWorkId is a concurrent map would be to just call get() first and replace the containsKey check with comparing if null is returned.
similarly the completedProcessingMetrics.containsKey check could be replaced by using getOrDefault
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] make getProcessingDistributionsForWorkId synchronized [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #29979:
URL: https://github.com/apache/beam/pull/29979#issuecomment-1908758730
CC: @clmccart @scwhittle this issue is marked as release blocker. Any update?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org