Merge branch 'pks-lfs-pointers-drop-bitmap-experiment' into 'master'

blob: Drop LFS pointer bitmap experiment

See merge request gitlab-org/gitaly!3385

4 files changed, 27 insertions, 87 deletions

diff --git a/changelogs/unreleased/pks-lfs-pointers-drop-bitmap-experiment.yml b/changelogs/unreleased/pks-lfs-pointers-drop-bitmap-experiment.yml
new file mode 100644
index 000000000..f7ec3d562
--- /dev/null
+++ b/changelogs/unreleased/pks-lfs-pointers-drop-bitmap-experiment.yml
@@ -0,0 +1,5 @@
+---
+title: 'blob: Drop LFS pointer bitmap experiment'
+merge_request: 3385
+author:
+type: performance
diff --git a/internal/gitaly/service/blob/lfs_pointers.go b/internal/gitaly/service/blob/lfs_pointers.go
index 76d6000a3..b8e30188e 100644
--- a/internal/gitaly/service/blob/lfs_pointers.go
+++ b/internal/gitaly/service/blob/lfs_pointers.go
@@ -13,7 +13,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"golang.org/x/text/transform"
 	"google.golang.org/grpc/codes"
@@ -256,23 +255,18 @@ func findLFSPointersByRevisions(
 		}
 	}
 
-	flags := []git.Option{
-		git.Flag{Name: "--in-commit-order"},
-		git.Flag{Name: "--objects"},
-		git.Flag{Name: "--no-object-names"},
-		git.Flag{Name: fmt.Sprintf("--filter=blob:limit=%d", lfsPointerMaxSize)},
-	}
-	if featureflag.IsEnabled(ctx, featureflag.LFSPointersUseBitmapIndex) {
-		flags = append(flags, git.Flag{Name: "--use-bitmap-index"})
-	}
-
 	// git-rev-list(1) currently does not have any way to list all reachable objects of a
 	// certain type.
 	var revListStderr bytes.Buffer
 	revlist, err := repo.Exec(ctx, git.SubCmd{
-		Name:  "rev-list",
-		Flags: flags,
-		Args:  revisions,
+		Name: "rev-list",
+		Flags: []git.Option{
+			git.Flag{Name: "--in-commit-order"},
+			git.Flag{Name: "--objects"},
+			git.Flag{Name: "--no-object-names"},
+			git.Flag{Name: fmt.Sprintf("--filter=blob:limit=%d", lfsPointerMaxSize)},
+		},
+		Args: revisions,
 	}, git.WithStderr(&revListStderr))
 	if err != nil {
 		return nil, fmt.Errorf("could not execute rev-list: %w", err)
diff --git a/internal/gitaly/service/blob/lfs_pointers_test.go b/internal/gitaly/service/blob/lfs_pointers_test.go
index c89747b85..09404f330 100644
--- a/internal/gitaly/service/blob/lfs_pointers_test.go
+++ b/internal/gitaly/service/blob/lfs_pointers_test.go
@@ -2,7 +2,6 @@ package blob
 
 import (
 	"bytes"
-	"context"
 	"errors"
 	"fmt"
 	"io"
@@ -17,7 +16,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -70,16 +68,11 @@ var (
 )
 
 func TestListLFSPointers(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.LFSPointersUseBitmapIndex,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		testListLFSPointers(t, ctx)
-	})
-}
-
-func testListLFSPointers(t *testing.T, ctx context.Context) {
 	_, repo, _, client := setup(t)
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	for _, tc := range []struct {
 		desc             string
 		revs             []string
@@ -184,14 +177,6 @@ func testListLFSPointers(t *testing.T, ctx context.Context) {
 }
 
 func TestListAllLFSPointers(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.LFSPointersUseBitmapIndex,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		testListAllLFSPointers(t, ctx)
-	})
-}
-
-func testListAllLFSPointers(t *testing.T, ctx context.Context) {
 	receivePointers := func(t *testing.T, stream gitalypb.BlobService_ListAllLFSPointersClient) []*gitalypb.LFSPointer {
 		t.Helper()
 
@@ -306,14 +291,6 @@ size 12345`
 }
 
 func TestSuccessfulGetLFSPointersRequest(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.LFSPointersUseBitmapIndex,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		testSuccessfulGetLFSPointersRequestFeatured(t)
-	})
-}
-
-func testSuccessfulGetLFSPointersRequestFeatured(t *testing.T) {
 	_, repo, _, client := setup(t)
 
 	ctx, cancel := testhelper.Context()
@@ -359,14 +336,11 @@ func testSuccessfulGetLFSPointersRequestFeatured(t *testing.T) {
 }
 
 func TestFailedGetLFSPointersRequestDueToValidations(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.LFSPointersUseBitmapIndex,
-	}).Run(t, testFailedGetLFSPointersRequestDueToValidations)
-}
-
-func testFailedGetLFSPointersRequestDueToValidations(t *testing.T, ctx context.Context) {
 	_, repo, _, client := setup(t)
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	testCases := []struct {
 		desc    string
 		request *gitalypb.GetLFSPointersRequest
@@ -403,14 +377,6 @@ func testFailedGetLFSPointersRequestDueToValidations(t *testing.T, ctx context.C
 }
 
 func TestSuccessfulGetNewLFSPointersRequest(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.LFSPointersUseBitmapIndex,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		testSuccessfulGetNewLFSPointersRequestFeatured(t)
-	})
-}
-
-func testSuccessfulGetNewLFSPointersRequestFeatured(t *testing.T) {
 	cfg, _, _, client := setup(t)
 
 	ctx, cancel := testhelper.Context()
@@ -548,14 +514,11 @@ func testSuccessfulGetNewLFSPointersRequestFeatured(t *testing.T) {
 }
 
 func TestFailedGetNewLFSPointersRequestDueToValidations(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.LFSPointersUseBitmapIndex,
-	}).Run(t, testFailedGetNewLFSPointersRequestDueToValidations)
-}
-
-func testFailedGetNewLFSPointersRequestDueToValidations(t *testing.T, ctx context.Context) {
 	_, repo, _, client := setup(t)
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	testCases := []struct {
 		desc       string
 		repository *gitalypb.Repository
@@ -605,14 +568,6 @@ func drainNewPointers(c gitalypb.BlobService_GetNewLFSPointersClient) error {
 }
 
 func TestSuccessfulGetAllLFSPointersRequest(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.LFSPointersUseBitmapIndex,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		testSuccessfulGetAllLFSPointersRequestFeatured(t)
-	})
-}
-
-func testSuccessfulGetAllLFSPointersRequestFeatured(t *testing.T) {
 	_, repo, _, client := setup(t)
 
 	ctx, cancel := testhelper.Context()
@@ -653,14 +608,11 @@ func getAllPointers(t *testing.T, c gitalypb.BlobService_GetAllLFSPointersClient
 }
 
 func TestFailedGetAllLFSPointersRequestDueToValidations(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.LFSPointersUseBitmapIndex,
-	}).Run(t, testFailedGetAllLFSPointersRequestDueToValidations)
-}
-
-func testFailedGetAllLFSPointersRequestDueToValidations(t *testing.T, ctx context.Context) {
 	_, _, _, client := setup(t)
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	testCases := []struct {
 		desc       string
 		repository *gitalypb.Repository
@@ -696,17 +648,9 @@ func drainAllPointers(c gitalypb.BlobService_GetAllLFSPointersClient) error {
 	}
 }
 
-func TestGetAllLFSPointersVerifyScope(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.LFSPointersUseBitmapIndex,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		testGetAllLFSPointersVerifyScopeFeatured(t)
-	})
-}
-
-// testGetAllLFSPointersVerifyScope verifies that this RPC returns all LFS
+// TestGetAllLFSPointersVerifyScope verifies that this RPC returns all LFS
 // pointers in a repository, not only ones reachable from the default branch
-func testGetAllLFSPointersVerifyScopeFeatured(t *testing.T) {
+func TestGetAllLFSPointersVerifyScope(t *testing.T) {
 	_, repo, repoPath, client := setup(t)
 
 	ctx, cancel := testhelper.Context()
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index d4e8ea860..f13a5f38b 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -24,8 +24,6 @@ var (
 	GoUserUpdateSubmodule = FeatureFlag{Name: "go_user_update_submodule", OnByDefault: true}
 	// GoUserRevert enables the Go implementation of UserRevert
 	GoUserRevert = FeatureFlag{Name: "go_user_revert", OnByDefault: false}
-	// LFSPointersUseBitmapIndex enables the use of bitmap indices when searching LFS pointers.
-	LFSPointersUseBitmapIndex = FeatureFlag{Name: "lfs_pointers_use_bitmap_index", OnByDefault: false}
 	// GoUpdateRemoteMirror enables the Go implementation of UpdateRemoteMirror
 	GoUpdateRemoteMirror = FeatureFlag{Name: "go_update_remote_mirror", OnByDefault: false}
 	// ConnectionMultiplexing enables the use of multiplexed connection from Praefect to Gitaly.
@@ -46,7 +44,6 @@ var All = []FeatureFlag{
 	GoUserUpdateSubmodule,
 	GoUserRevert,
 	GrpcTreeEntryNotFound,
-	LFSPointersUseBitmapIndex,
 	GoUpdateRemoteMirror,
 	ConnectionMultiplexing,
 	BackchannelVoting,