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

Preserve tombstones for workunits which complete while they still have children #15088

Merged
merged 4 commits into from Apr 11, 2022

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented Apr 10, 2022

#15080 was caused by two factors:

  1. Add global counters, and deprecate per-workunit counters #14541 made counters global via a new API, and attached them (as deprecated) to "the root workunit" (with "has no parent id" as the heuristic that something was the root workunit).
  2. Store workunits as a DAG rather than a tree #14856 moved to calculating the parent(s) of a node based on a running graph of workunits (to allow Workunits should form a DAG rather than a tree #14680 to eventually add multiple parents), which meant that when nodes completed out of order, we might not have any parents for them.

Together: this meant that when workunits completed asynchronously, we might not have parents for them, and because of the deprecation, we would attach the counters to multiple workunits. A consumer which was aggregating the counters would end up with an inaccurate total.

Fixes #15080.

@stuhood stuhood added needs-cherrypick category:bugfix Bug fixes for released features labels Apr 10, 2022
@stuhood stuhood added this to the 2.11.x milestone Apr 10, 2022
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@stuhood stuhood force-pushed the stuhood/workunit-completed-parents branch from 133be8c to 806c260 Compare April 10, 2022 22:42
@stuhood stuhood marked this pull request as ready for review April 11, 2022 01:12
Copy link
Contributor

@tdyas tdyas left a comment

Choose a reason for hiding this comment

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

lgtm

@stuhood stuhood merged commit db26132 into pantsbuild:main Apr 11, 2022
@stuhood stuhood deleted the stuhood/workunit-completed-parents branch April 11, 2022 17:45
stuhood added a commit to stuhood/pants that referenced this pull request Apr 11, 2022
…e children (pantsbuild#15088)

1. pantsbuild#14541 made counters global via a new API, and attached them (as deprecated) to "the root workunit" (with "has no parent id" as the heuristic that something was the root workunit).
2. pantsbuild#14856 moved to calculating the parent(s) of a node based on a running graph of workunits (to allow pantsbuild#14680 to eventually add multiple parents), which meant that when nodes completed out of order, we might not have any parents for them.

Together: this meant that when workunits completed asynchronously, we might not have parents for them, and because of the deprecation, we would attach the counters to multiple workunits. A consumer which was aggregating the counters would end up with an inaccurate total.

Fixes pantsbuild#15080.
stuhood added a commit that referenced this pull request Apr 11, 2022
…e children (cherrypick of #15088) (#15103)

#15080 was caused by two factors:

1. #14541 made counters global via a new API, and attached them (as deprecated) to "the root workunit" (with "has no parent id" as the heuristic that something was the root workunit).
2. #14856 moved to calculating the parent(s) of a node based on a running graph of workunits (to allow #14680 to eventually add multiple parents), which meant that when nodes completed out of order, we might not have any parents for them.

Together: this meant that when workunits completed asynchronously, we might not have parents for them, and because of the deprecation, we would attach the counters to multiple workunits. A consumer which was aggregating the counters would end up with an inaccurate total.

Fixes #15080.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inaccurate "remote cache time saved" total
2 participants