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

[7.4.1] Remove DownloadManager instance from RegistryFactoryImpl. #24228

Conversation

meteorcloudy
Copy link
Member

This PR removes the DownloadManager from the registry factory implementation. As the registry created by the factory is cached by the SkyFunction mechanism, the DownloadManager instance was living too long - it is supposed to be re-created for every command instantiation to respect changes in command line options, but for the registry, it ignored those changes.

Instead, the DownloadManager is set directly into the affected SkyFunctions that require access to it. This way, the per-command DownloadManager instance is correctly used.

This fixes #24166.

Note for reviewers: This is my first time touching code with SkyFunctions, so I don't really know what I'm doing.

Closes #24212.

PiperOrigin-RevId: 693644409
Change-Id: I7b16684e52673043615290d114f078ab7ab99fcf

This PR removes the DownloadManager from the registry factory implementation. As the registry created by the factory is cached by the SkyFunction mechanism, the DownloadManager instance was living too long - it is supposed to be re-created for every command instantiation to respect changes in command line options, but for the registry, it ignored those changes.

Instead, the DownloadManager is set directly into the affected SkyFunctions that require access to it. This way, the per-command DownloadManager instance is correctly used.

This fixes bazelbuild#24166.

Note for reviewers: This is my first time touching code with SkyFunctions, so I don't really know what I'm doing.

Closes bazelbuild#24212.

PiperOrigin-RevId: 693644409
Change-Id: I7b16684e52673043615290d114f078ab7ab99fcf
@meteorcloudy meteorcloudy requested a review from a team as a code owner November 6, 2024 15:30
@meteorcloudy meteorcloudy changed the title Remove DownloadManager instance from RegistryFactoryImpl. [7.4.1] Remove DownloadManager instance from RegistryFactoryImpl. Nov 6, 2024
@meteorcloudy meteorcloudy enabled auto-merge (squash) November 6, 2024 15:31
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. labels Nov 6, 2024
@meteorcloudy meteorcloudy merged commit 24caa1d into bazelbuild:release-7.4.1 Nov 6, 2024
50 of 51 checks passed
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants