From a84764ca65c0eda04b2404fea1777544a3572769 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Mon, 7 Nov 2022 17:28:39 +0100 Subject: catfile: Use `ObjectReader` behind the feature flag Now that we finally have `ObjectReader` ready to be utilized, we use this behind the `CatfileBatchCommand` featureflag in `catfile/cache.go`. We also add the required featuresets for tests and a top level flip of the featureflag in `testhelper.go`, since this code is used by most of the RPCs. --- internal/git/catfile/cache.go | 41 ++++++++++++++++++++++++++++---------- internal/git/catfile/cache_test.go | 15 ++++++++++++-- internal/git/catfile/tag_test.go | 8 +++++++- internal/testhelper/testhelper.go | 3 +++ 4 files changed, 54 insertions(+), 13 deletions(-) diff --git a/internal/git/catfile/cache.go b/internal/git/catfile/cache.go index 493e7b9d3..cde7957f9 100644 --- a/internal/git/catfile/cache.go +++ b/internal/git/catfile/cache.go @@ -14,6 +14,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/metadata" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/labkit/correlation" ) @@ -171,16 +172,26 @@ func (c *ProcessCache) Stop() { // ObjectReader creates a new ObjectReader process for the given repository. func (c *ProcessCache) ObjectReader(ctx context.Context, repo git.RepositoryExecutor) (ObjectContentReader, func(), error) { - cacheable, cancel, err := c.getOrCreateProcess(ctx, repo, &c.objectReaders, func(ctx context.Context) (cacheable, error) { - return newObjectContentReader(ctx, repo, c.catfileLookupCounter) - }, "catfile.ObjectReader") + var cached cacheable + var err error + var cancel func() + + if featureflag.CatfileBatchCommand.IsEnabled(ctx) { + cached, cancel, err = c.getOrCreateProcess(ctx, repo, &c.objectReaders, func(ctx context.Context) (cacheable, error) { + return newObjectReader(ctx, repo, c.catfileLookupCounter) + }, "catfile.ObjectReader") + } else { + cached, cancel, err = c.getOrCreateProcess(ctx, repo, &c.objectReaders, func(ctx context.Context) (cacheable, error) { + return newObjectContentReader(ctx, repo, c.catfileLookupCounter) + }, "catfile.ObjectContentReader") + } if err != nil { return nil, nil, err } - objectReader, ok := cacheable.(ObjectContentReader) + objectReader, ok := cached.(ObjectContentReader) if !ok { - return nil, nil, fmt.Errorf("expected object reader, got %T", cacheable) + return nil, nil, fmt.Errorf("expected object reader, got %T", cached) } return objectReader, cancel, nil @@ -188,16 +199,26 @@ func (c *ProcessCache) ObjectReader(ctx context.Context, repo git.RepositoryExec // ObjectInfoReader creates a new ObjectInfoReader process for the given repository. func (c *ProcessCache) ObjectInfoReader(ctx context.Context, repo git.RepositoryExecutor) (ObjectInfoReader, func(), error) { - cacheable, cancel, err := c.getOrCreateProcess(ctx, repo, &c.objectInfoReaders, func(ctx context.Context) (cacheable, error) { - return newObjectInfoReader(ctx, repo, c.catfileLookupCounter) - }, "catfile.ObjectInfoReader") + var cached cacheable + var err error + var cancel func() + + if featureflag.CatfileBatchCommand.IsEnabled(ctx) { + cached, cancel, err = c.getOrCreateProcess(ctx, repo, &c.objectInfoReaders, func(ctx context.Context) (cacheable, error) { + return newObjectReader(ctx, repo, c.catfileLookupCounter) + }, "catfile.ObjectReader") + } else { + cached, cancel, err = c.getOrCreateProcess(ctx, repo, &c.objectInfoReaders, func(ctx context.Context) (cacheable, error) { + return newObjectInfoReader(ctx, repo, c.catfileLookupCounter) + }, "catfile.ObjectInfoReader") + } if err != nil { return nil, nil, err } - objectInfoReader, ok := cacheable.(ObjectInfoReader) + objectInfoReader, ok := cached.(ObjectInfoReader) if !ok { - return nil, nil, fmt.Errorf("expected object info reader, got %T", cacheable) + return nil, nil, fmt.Errorf("expected object info reader, got %T", cached) } return objectInfoReader, cancel, nil diff --git a/internal/git/catfile/cache_test.go b/internal/git/catfile/cache_test.go index e2ca83332..1a20f37c5 100644 --- a/internal/git/catfile/cache_test.go +++ b/internal/git/catfile/cache_test.go @@ -13,6 +13,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/git/repository" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v15/internal/helper" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" "gitlab.com/gitlab-org/labkit/correlation" @@ -203,7 +204,12 @@ func TestCache_autoExpiry(t *testing.T) { } func TestCache_ObjectReader(t *testing.T) { - ctx := testhelper.Context(t) + t.Parallel() + + testhelper.NewFeatureSets(featureflag.CatfileBatchCommand).Run(t, testCacheObjectReader) +} + +func testCacheObjectReader(t *testing.T, ctx context.Context) { cfg := testcfg.Build(t) repo, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ @@ -303,7 +309,12 @@ func TestCache_ObjectReader(t *testing.T) { } func TestCache_ObjectInfoReader(t *testing.T) { - ctx := testhelper.Context(t) + t.Parallel() + + testhelper.NewFeatureSets(featureflag.CatfileBatchCommand).Run(t, testCacheObjectInfoReader) +} + +func testCacheObjectInfoReader(t *testing.T, ctx context.Context) { cfg := testcfg.Build(t) repo, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ diff --git a/internal/git/catfile/tag_test.go b/internal/git/catfile/tag_test.go index efc54e20c..fa9da5763 100644 --- a/internal/git/catfile/tag_test.go +++ b/internal/git/catfile/tag_test.go @@ -1,6 +1,7 @@ package catfile import ( + "context" "fmt" "strings" "testing" @@ -9,13 +10,18 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v15/internal/helper" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" ) func TestGetTag(t *testing.T) { - ctx := testhelper.Context(t) + t.Parallel() + testhelper.NewFeatureSets(featureflag.CatfileBatchCommand).Run(t, testGetTag) +} + +func testGetTag(t *testing.T, ctx context.Context) { cfg, objectReader, _, repoPath := setupObjectReader(t, ctx) commitID := gittest.WriteCommit(t, cfg, repoPath) diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index dfcb757fe..3596d02e5 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -201,6 +201,9 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context { ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.NodeErrorCancelsVoter, rnd.Int()%2 == 0) ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.GitV238, rnd.Int()%2 == 0) + // CatfileBatchCommand affects many tests since most of them rely on catfile for content/info + // information about objects. + ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.CatfileBatchCommand, rnd.Int()%2 == 0) for _, opt := range opts { ctx = opt(ctx) -- cgit v1.2.3