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

feat: add option to exit early if analysis cache is discarded #16805

Closed

Conversation

mattem
Copy link
Contributor

@mattem mattem commented Nov 19, 2022

Adds --fail_on_analysis_cache_discard option that allows the build to exit early if the analysis cache would have been discarded.

The flag name is of course open to bikeshedding etc.

fixes #16804

@mattem mattem force-pushed the feat/fail_on_analysis_cache_discard branch 4 times, most recently from f59a234 to a5c8298 Compare November 19, 2022 21:37
@mattem mattem marked this pull request as ready for review November 19, 2022 21:57
@sgowroji sgowroji added team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Nov 20, 2022
@acecilia
Copy link

👋 What is the status of this PR?

Context: I have a job that takes average 5min to run. The build was inadvertently discarding the analysis cache. When fixed, the average job time went down to 2min 😮

@gregestren
Copy link
Contributor

👋 What is the status of this PR?

Context: I have a job that takes average 5min to run. The build was inadvertently discarding the analysis cache. When fixed, the average job time went down to 2min 😮

We need to integrate feedback on exactly how the information is communicated. See #16805 (comment).

I'm not sure where this fits on @mattem 's current workload. I'll see if I can clarify the linked comment a bit more clearly, since it leaves room for interpretation.

gregestren added a commit to gregestren/bazel that referenced this pull request Mar 15, 2023
For bazelbuild#16805.

PiperOrigin-RevId: 516684564
Change-Id: I23024704ae2b35c1a5f9cd31b8839886b1bb8dc3
@gregestren
Copy link
Contributor

I'm not sure where this fits on @mattem 's current workload. I'll see if I can clarify the linked comment a bit more clearly, since it leaves room for interpretation.

See my just-posted inline comment (https://github.com/bazelbuild/bazel/pull/16805/files#r1136437606).

@mattem
Copy link
Contributor Author

mattem commented Mar 15, 2023

Sorry, this is stalled on me. I'm juggling quite a bit currently, but I'll see if I can work on this later this week. @gregestren Those comments above make sense to me on a quick glance. Did you want the addition to the finished event to land separately?

@gregestren
Copy link
Contributor

I support the finished approach as the sole solution to this problem. i.e. an iteration of the code I shared with a new type vs. CONFLICTING_CONFIGURATIONS. I think this addresses concerns from all sides.

@mattem mattem force-pushed the feat/fail_on_analysis_cache_discard branch from a5c8298 to 8313dd0 Compare March 25, 2023 23:13
@mattem mattem requested a review from a team as a code owner March 25, 2023 23:13
@mattem mattem requested review from aranguyen and removed request for a team March 25, 2023 23:13
@mattem mattem force-pushed the feat/fail_on_analysis_cache_discard branch from 8313dd0 to bb30682 Compare March 25, 2023 23:16
@mattem mattem requested review from gregestren and removed request for aranguyen March 27, 2023 17:01
@gregestren
Copy link
Contributor

Just back from vacation last week. Looking forward to reviewing the updates...

Copy link
Contributor

@gregestren gregestren left a comment

Choose a reason for hiding this comment

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

Looking good to me. Just a few more small-scale comments.

@gregestren
Copy link
Contributor

gregestren commented Apr 4, 2023

Since we're getting close to submission, I want to signal boost @mattem 's old comment about flag naming.

The current name is --fail_on_analysis_cache_discard. @mattem noted an alternative suggestion is --disallow_analysis_cache_discards. Anyone else with preferences before we lock on the current naming?

@brentleyjones
Copy link
Contributor

I like --disallow_analysis_cache_discards.

Copy link
Contributor

@gregestren gregestren left a comment

Choose a reason for hiding this comment

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

Thanks for pulling this all the way through, @mattem .

@gregestren gregestren added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jul 27, 2023
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jul 31, 2023
@brentleyjones
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jul 31, 2023
@iancha1992
Copy link
Member

@bazel-io fork 6.4.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 3, 2023
@iancha1992
Copy link
Member

iancha1992 commented Aug 3, 2023

@gregestren @mattem
When cherry-pick was attempted to release-6.4.0, there were merge conflicts:

  1. src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
    We may need another commit that introduced the lines below:
skyframeBuildView.setConfiguration(
    eventHandler, topLevelConfig, viewOptions.maxConfigChangesToShow);
  1. src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
    We many need another prior commit that has previously affected the setConfiguration function and changed the function name from setConfigurations to setConfiguration

  2. src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
    Need a commit that changed the variable name from setConfigurationsForTesting to setConfigurationForTesting

cc: @bazelbuild/triage

@gregestren
Copy link
Contributor

gregestren commented Aug 7, 2023

I thought we had a similar challenge with the original merge somewhere? If so, it should be straightforward to resolve.

I'll get to this as soon as I can. What's the deadline for 6.4.0?

@brentleyjones
Copy link
Contributor

What's the deeadline for 6.4.0?

#19035

Expected first release candidate date: 2023-09-18

@mattem mattem deleted the feat/fail_on_analysis_cache_discard branch August 18, 2023 13:53
@iancha1992
Copy link
Member

Hi, @gregestren @brentleyjones. I just wanted to check up on this. Are we still pushing this to be part of 6.4.0? The expected first release candidate date is next Monday (9/18/2023), just FYI.

@gregestren
Copy link
Contributor

Yes. I was away for two weeks, just back today. I'll try to prep up a new patch...

gregestren pushed a commit to gregestren/bazel that referenced this pull request Sep 12, 2023
Adds `--allow_analysis_cache_discard` option that allows the build to exit early if the analysis cache would have been discarded.

The flag name is of course open to bikeshedding etc.

fixes bazelbuild#16804

Closes bazelbuild#16805.

PiperOrigin-RevId: 552575951
Change-Id: Ia336eb3a5b7d7e41665fd0e0adf3edc03ed50f18
gregestren pushed a commit to gregestren/bazel that referenced this pull request Sep 12, 2023
Adds `--allow_analysis_cache_discard` option that allows the build to exit early if the analysis cache would have been discarded.

The flag name is of course open to bikeshedding etc.

fixes bazelbuild#16804

Closes bazelbuild#16805.

PiperOrigin-RevId: 552575951
Change-Id: Ia336eb3a5b7d7e41665fd0e0adf3edc03ed50f18
iancha1992 pushed a commit that referenced this pull request Sep 12, 2023
…#19503)

Forked from #16805.

Manually merged conflicts observed at
#16805 (comment).

Fixes #19162.

---------

Co-authored-by: Matt Mackay <mattem@gmail.com>
SalmaSamy pushed a commit that referenced this pull request Sep 13, 2023
…#19503)

Forked from #16805.

Manually merged conflicts observed at
#16805 (comment).

Fixes #19162.

---------

Co-authored-by: Matt Mackay <mattem@gmail.com>
@iancha1992
Copy link
Member

The changes in this PR have been included in Bazel 6.4.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!

sluongng added a commit to buildbuddy-io/buildbuddy that referenced this pull request Oct 27, 2023
These proto files are taken from commit
bazelbuild/bazel@76117b4

Highlight:

- Inclusion of ActionCacheStatistics in build_event_stream
  bazelbuild/bazel#17615

- Explicit Execution phase timing (in prepare for Skymeld)
  bazelbuild/bazel@be63eee

- If `--noallow_analysis_cache_discard` is set, BuildFinished may
  contains FailureDetails -> CONFIGURATION_DISCARDED_ANALYSIS_CACHE.
  bazelbuild/bazel#16805

- ExecRequest event is included for `bazel run`
  bazelbuild/bazel@9a047de

- TestProgress event is included for TestRunner action
  bazelbuild/bazel@d8b8ab0

- ActionExecuted now includes start/end time and strategy information
  bazelbuild/bazel@2ddacab
alexeagle added a commit to alexeagle/bazel that referenced this pull request Oct 30, 2023
Mention the new flag from bazelbuild#16805 that can help prevent analysis cache discards
copybara-service bot pushed a commit that referenced this pull request Nov 1, 2023
Mention the new flag from #16805 that can help prevent analysis cache discards

Closes #19989.

PiperOrigin-RevId: 578673354
Change-Id: I062274f8059bbfeef5fb49b9c6c26db9887efde9
@tkoeppe
Copy link
Contributor

tkoeppe commented Feb 26, 2024

Many thanks for this really helpful feature! (Also on behalf of many satisfied users.)

I was wondering if it would make sense to extend this logic to "server restared because startup options changed". Restarting the server of course also invalidates, so it wouldn't be stretch to say that the flag should also prevent that, would it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] Add a way to disallow analysis cache discards