From 801dd3462b995155fb02bd9fe48cb64177ee7ba6 Mon Sep 17 00:00:00 2001 From: Florian Weikert Date: Mon, 25 Nov 2024 17:07:19 +0100 Subject: [PATCH] Avoid unnecessary HTTP requests for latest/last_rc (#641) * Avoid unnecessary HTTP requests for latest/last_rc The refactoring in https://github.com/bazelbuild/bazelisk/pull/631 introduced a severe performance regression: With latest and last_rc the code traversed *all* GCS buckets for *every* existing Bazel version. Since we send one HTTP request per bucket, this behavior led to a significant increase in HTTP requests ( >140 instead of 2-3 requests). This commit restores the previous, correct behavior: Traversal will be stopped as soon as a matching version has been found. Moreover, this commit adds a test to prevent similar regressions in the future. Fixes https://github.com/bazelbuild/bazelisk/issues/640 Drive-by fix: Replaced \"%s\" with %q. * Apply minimal indentation principle --- bazelisk_version_test.go | 52 ++++++++++++++++++++++++++++++++++++++++ core/core.go | 2 +- core/repositories.go | 2 +- httputil/fake.go | 3 +++ platforms/platforms.go | 4 ++-- repositories/gcs.go | 6 ++++- versions/versions.go | 2 +- 7 files changed, 65 insertions(+), 6 deletions(-) diff --git a/bazelisk_version_test.go b/bazelisk_version_test.go index eb76793c..1aec8028 100644 --- a/bazelisk_version_test.go +++ b/bazelisk_version_test.go @@ -164,6 +164,58 @@ func TestResolveLatestVersion_ShouldOnlyReturnStableReleases(t *testing.T) { } } +func TestResolveLatestAvoidsUnnecessaryRequests(t *testing.T) { + tests := []struct{ + name string + specifiedVersion string + wantVersion string + } { + { + name: "Release", + specifiedVersion: "latest", + wantVersion: "7.0.0", + }, + { + name: "Candidate", + specifiedVersion: "last_rc", + wantVersion: "7.0.0rc1", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T){ + s := setUp(t) + s.AddVersion("4.0.0", true, []int{1}, nil) + s.AddVersion("5.0.0", true, []int{1}, nil) + s.AddVersion("6.0.0", true, []int{1}, nil) + s.AddVersion("7.0.0", true, []int{1}, nil) + s.AddVersion("8.0.0", false, nil, nil) + s.Finish() + + gcs := &repositories.GCSRepo{} + repos := core.CreateRepositories(gcs, nil, nil, nil, false) + gotVersion, _, err := repos.ResolveVersion(tmpDir, versions.BazelUpstream, test.specifiedVersion, config.Null()) + + if err != nil { + t.Fatalf("Version resolution failed unexpectedly: %v", err) + } + if gotVersion != test.wantVersion { + t.Errorf("Expected version %s, but got %s", test.wantVersion, gotVersion) + } + /* + We expect three requests: + - One to get a list of all Bazel version tracks + - One to check for the existence of the 8.* release (candidates), which returns no results + - One to check for the existence of the 7.* release (candidates), which returns a match + */ + wantRequests := 3 + if gotRequests := len(s.Transport.RequestedURLs); gotRequests != wantRequests { + t.Errorf("Expected exactly %d requests (one for the top-level, one for 8.0.0, one for 7.0.0), but got %d:\n%s", wantRequests, gotRequests, strings.Join(s.Transport.RequestedURLs, "\n")) + } + }) + } +} + func TestResolveLatestVersion_ShouldFailIfNotEnoughReleases(t *testing.T) { s := setUp(t) s.AddVersion("3.0.0", true, nil, nil) diff --git a/core/core.go b/core/core.go index 45536b8e..ee98bd80 100644 --- a/core/core.go +++ b/core/core.go @@ -332,7 +332,7 @@ func parseBazelForkAndVersion(bazelForkAndVersion string) (string, string, error } else if len(versionInfo) == 2 { bazelFork, bazelVersion = versionInfo[0], versionInfo[1] } else { - return "", "", fmt.Errorf("invalid version \"%s\", could not parse version with more than one slash", bazelForkAndVersion) + return "", "", fmt.Errorf("invalid version %q, could not parse version with more than one slash", bazelForkAndVersion) } return bazelFork, bazelVersion, nil diff --git a/core/repositories.go b/core/repositories.go index 8ba3f459..66dc6b92 100644 --- a/core/repositories.go +++ b/core/repositories.go @@ -196,7 +196,7 @@ func resolvePotentiallyRelativeVersion(bazeliskHome string, lister listVersionsF index := len(available) - 1 - vi.LatestOffset if index < 0 { - return "", fmt.Errorf("cannot resolve version \"%s\": There are not enough matching Bazel releases (%d)", vi.Value, len(available)) + return "", fmt.Errorf("cannot resolve version %q: There are not enough matching Bazel releases (%d)", vi.Value, len(available)) } sorted := versions.GetInAscendingOrder(available) return sorted[index], nil diff --git a/httputil/fake.go b/httputil/fake.go index 4b280a84..f870af6d 100644 --- a/httputil/fake.go +++ b/httputil/fake.go @@ -9,6 +9,8 @@ import ( // FakeTransport represents a fake http.Transport that returns prerecorded responses. type FakeTransport struct { responses map[string]*responseCollection + + RequestedURLs []string } // NewFakeTransport creates a new FakeTransport instance without any responses. @@ -38,6 +40,7 @@ func (ft *FakeTransport) AddError(url string, err error) { // RoundTrip returns a prerecorded response to the given request, if one exists. Otherwise its response indicates 404 - not found. func (ft *FakeTransport) RoundTrip(req *http.Request) (*http.Response, error) { + ft.RequestedURLs = append(ft.RequestedURLs, req.URL.String()) if responses, ok := ft.responses[req.URL.String()]; ok { return responses.Next() } diff --git a/platforms/platforms.go b/platforms/platforms.go index ebf60322..6f4c7e81 100644 --- a/platforms/platforms.go +++ b/platforms/platforms.go @@ -64,7 +64,7 @@ func DetermineArchitecture(osName, version string) (string, error) { case "arm64": machineName = "arm64" default: - return "", fmt.Errorf("unsupported machine architecture \"%s\", must be arm64 or x86_64", runtime.GOARCH) + return "", fmt.Errorf("unsupported machine architecture %q, must be arm64 or x86_64", runtime.GOARCH) } if osName == "darwin" { @@ -80,7 +80,7 @@ func DetermineOperatingSystem() (string, error) { case "darwin", "linux", "windows": return runtime.GOOS, nil default: - return "", fmt.Errorf("unsupported operating system \"%s\", must be Linux, macOS or Windows", runtime.GOOS) + return "", fmt.Errorf("unsupported operating system %q, must be Linux, macOS or Windows", runtime.GOOS) } } diff --git a/repositories/gcs.go b/repositories/gcs.go index b9bb3b2b..025b82f1 100644 --- a/repositories/gcs.go +++ b/repositories/gcs.go @@ -157,12 +157,16 @@ func (gcs *GCSRepo) matchingVersions(history []string, opts *core.FilterOpts) ([ // Ascending list of rc versions, followed by the release version (if it exists) and a rolling identifier (if there are rolling releases). versions := getVersionsFromGCSPrefixes(prefixes) - for vpos := len(versions) - 1; vpos >= 0 && len(descendingMatches) < opts.MaxResults; vpos-- { + for vpos := len(versions) - 1; vpos >= 0; vpos-- { curr := versions[vpos] if strings.Contains(curr, "rolling") || !opts.Filter(curr) { continue } + descendingMatches = append(descendingMatches, curr) + if len(descendingMatches) == opts.MaxResults { + return descendingMatches, nil + } } } return descendingMatches, nil diff --git a/versions/versions.go b/versions/versions.go index d775c346..c4decb54 100644 --- a/versions/versions.go +++ b/versions/versions.go @@ -61,7 +61,7 @@ func Parse(fork, version string) (*Info, error) { if m[1] != "" { offset, err := strconv.Atoi(m[1]) if err != nil { - return nil, fmt.Errorf("invalid version \"%s\", could not parse offset: %v", version, err) + return nil, fmt.Errorf("invalid version %q, could not parse offset: %v", version, err) } vi.LatestOffset = offset }