diff options
author | Pavlo Strokov <pstrokov@gitlab.com> | 2021-03-15 15:10:38 +0300 |
---|---|---|
committer | Pavlo Strokov <pstrokov@gitlab.com> | 2021-03-18 12:49:31 +0300 |
commit | 63c55949d54543fce205c0f750f8fc502ed9566a (patch) | |
tree | a26b220fde2aea0495789d584b35432bc6212751 /internal/gitaly/service | |
parent | 2093a05fa4214e69abc75eacea26e926266264b1 (diff) |
Linguist converted into an instantiation dependency
Implementation of the linguist package relies on the
package-private state: the use of the colorMap variable.
It is filled with a hook mechanism after validation of the
gitaly configuration. The problem is that this operation
invokes ruby code to get it done and this call is pretty
expensive. It also doesn't allow us to convert tests to
parallel as it causes race conditions in attempt to fill
the global variable.
The solution to this problem is creation of the struct
instance that encapsulates the state and can be instantiated
only when needed. The change also includes minification in
generating test configuration that does not require to make
a ruby call and uses a stub file for initialisation. The
behaviour could be returned back to the normal with usage of
the WithActualLinguist option for configuration creation.
The change also includes renames of the test functions for
the linguist package. The change speeds up execution of the
tests: before 0m43.907s and after 0m21.659s on my local.
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/2699
Diffstat (limited to 'internal/gitaly/service')
-rw-r--r-- | internal/gitaly/service/commit/languages.go | 5 | ||||
-rw-r--r-- | internal/gitaly/service/commit/languages_test.go | 7 | ||||
-rw-r--r-- | internal/gitaly/service/commit/server.go | 6 | ||||
-rw-r--r-- | internal/gitaly/service/commit/testhelper_test.go | 12 | ||||
-rw-r--r-- | internal/gitaly/service/operations/testhelper_test.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/register.go | 4 | ||||
-rw-r--r-- | internal/gitaly/service/repository/fetch_test.go | 2 |
7 files changed, 27 insertions, 11 deletions
diff --git a/internal/gitaly/service/commit/languages.go b/internal/gitaly/service/commit/languages.go index 894b5d118..4fd4cb164 100644 --- a/internal/gitaly/service/commit/languages.go +++ b/internal/gitaly/service/commit/languages.go @@ -10,7 +10,6 @@ import ( "strings" "gitlab.com/gitlab-org/gitaly/internal/git" - "gitlab.com/gitlab-org/gitaly/internal/gitaly/linguist" "gitlab.com/gitlab-org/gitaly/internal/gitaly/service/ref" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/helper/text" @@ -46,7 +45,7 @@ func (s *server) CommitLanguages(ctx context.Context, req *gitalypb.CommitLangua if err != nil { return nil, err } - stats, err := linguist.Stats(ctx, s.cfg, repoPath, commitID) + stats, err := s.linguist.Stats(ctx, s.cfg, repoPath, commitID) if err != nil { return nil, err } @@ -69,7 +68,7 @@ func (s *server) CommitLanguages(ctx context.Context, req *gitalypb.CommitLangua l := &gitalypb.CommitLanguagesResponse_Language{ Name: lang, Share: float32(100*count) / float32(total), - Color: linguist.Color(lang), + Color: s.linguist.Color(lang), Bytes: stats[lang], } resp.Languages = append(resp.Languages, l) diff --git a/internal/gitaly/service/commit/languages_test.go b/internal/gitaly/service/commit/languages_test.go index 7e09ba66b..54390a1de 100644 --- a/internal/gitaly/service/commit/languages_test.go +++ b/internal/gitaly/service/commit/languages_test.go @@ -5,12 +5,17 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/internal/testhelper" + "gitlab.com/gitlab-org/gitaly/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" ) func TestLanguages(t *testing.T) { - _, repo, _, client := setupCommitServiceWithRepo(t, true) + cfg, repo, _ := testcfg.BuildWithRepo(t, testcfg.WithRealLinguist()) + + serverSocketPath := startTestServices(t, cfg) + + client := newCommitServiceClient(t, serverSocketPath) request := &gitalypb.CommitLanguagesRequest{ Repository: repo, diff --git a/internal/gitaly/service/commit/server.go b/internal/gitaly/service/commit/server.go index 62c29b6b6..77d25709d 100644 --- a/internal/gitaly/service/commit/server.go +++ b/internal/gitaly/service/commit/server.go @@ -3,6 +3,7 @@ package commit import ( "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/linguist" "gitlab.com/gitlab-org/gitaly/internal/gitaly/service/ref" "gitlab.com/gitlab-org/gitaly/internal/storage" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -12,6 +13,7 @@ type server struct { cfg config.Cfg locator storage.Locator gitCmdFactory git.CommandFactory + linguist *linguist.Instance } var ( @@ -19,6 +21,6 @@ var ( ) // NewServer creates a new instance of a grpc CommitServiceServer -func NewServer(cfg config.Cfg, locator storage.Locator, gitCmdFactory git.CommandFactory) gitalypb.CommitServiceServer { - return &server{cfg: cfg, locator: locator, gitCmdFactory: gitCmdFactory} +func NewServer(cfg config.Cfg, locator storage.Locator, gitCmdFactory git.CommandFactory, ling *linguist.Instance) gitalypb.CommitServiceServer { + return &server{cfg: cfg, locator: locator, gitCmdFactory: gitCmdFactory, linguist: ling} } diff --git a/internal/gitaly/service/commit/testhelper_test.go b/internal/gitaly/service/commit/testhelper_test.go index f0815e916..91cf07f35 100644 --- a/internal/gitaly/service/commit/testhelper_test.go +++ b/internal/gitaly/service/commit/testhelper_test.go @@ -11,6 +11,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/linguist" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -28,6 +29,7 @@ func testMain(m *testing.M) int { return m.Run() } +// setupCommitService makes a basic configuration and starts the service with the client. func setupCommitService(t testing.TB) (config.Cfg, gitalypb.CommitServiceClient) { cfg, _, _, client := setupCommitServiceCreateRepo(t, func(tb testing.TB, cfg config.Cfg) (*gitalypb.Repository, string, testhelper.Cleanup) { return nil, "", func() {} @@ -35,7 +37,10 @@ func setupCommitService(t testing.TB) (config.Cfg, gitalypb.CommitServiceClient) return cfg, client } -func setupCommitServiceWithRepo(t testing.TB, bare bool) (config.Cfg, *gitalypb.Repository, string, gitalypb.CommitServiceClient) { +// setupCommitServiceWithRepo makes a basic configuration, creates a test repository and starts the service with the client. +func setupCommitServiceWithRepo( + t testing.TB, bare bool, +) (config.Cfg, *gitalypb.Repository, string, gitalypb.CommitServiceClient) { return setupCommitServiceCreateRepo(t, func(tb testing.TB, cfg config.Cfg) (*gitalypb.Repository, string, testhelper.Cleanup) { if bare { return gittest.CloneRepoAtStorage(tb, cfg.Storages[0], t.Name()) @@ -71,7 +76,10 @@ func startTestServices(t testing.TB, cfg config.Cfg) string { listener, err := net.Listen("unix", serverSocketPath) require.NoError(t, err) - gitalypb.RegisterCommitServiceServer(server, NewServer(cfg, config.NewLocator(cfg), git.NewExecCommandFactory(cfg))) + ling, err := linguist.New(cfg) + require.NoError(t, err) + + gitalypb.RegisterCommitServiceServer(server, NewServer(cfg, config.NewLocator(cfg), git.NewExecCommandFactory(cfg), ling)) go server.Serve(listener) return "unix://" + serverSocketPath diff --git a/internal/gitaly/service/operations/testhelper_test.go b/internal/gitaly/service/operations/testhelper_test.go index f0d48f4a2..626dbfe46 100644 --- a/internal/gitaly/service/operations/testhelper_test.go +++ b/internal/gitaly/service/operations/testhelper_test.go @@ -89,7 +89,7 @@ func runOperationServiceServer(t *testing.T) (string, func()) { gitalypb.RegisterHookServiceServer(srv.GrpcServer(), hook.NewServer(config.Config, hookManager, gitCmdFactory)) gitalypb.RegisterRepositoryServiceServer(srv.GrpcServer(), repository.NewServer(config.Config, RubyServer, locator, txManager, gitCmdFactory)) gitalypb.RegisterRefServiceServer(srv.GrpcServer(), ref.NewServer(config.Config, locator, gitCmdFactory)) - gitalypb.RegisterCommitServiceServer(srv.GrpcServer(), commit.NewServer(config.Config, locator, gitCmdFactory)) + gitalypb.RegisterCommitServiceServer(srv.GrpcServer(), commit.NewServer(config.Config, locator, gitCmdFactory, nil)) gitalypb.RegisterSSHServiceServer(srv.GrpcServer(), ssh.NewServer(config.Config, locator, gitCmdFactory)) reflection.Register(srv.GrpcServer()) diff --git a/internal/gitaly/service/register.go b/internal/gitaly/service/register.go index df898e5ca..89df6b041 100644 --- a/internal/gitaly/service/register.go +++ b/internal/gitaly/service/register.go @@ -8,6 +8,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" gitalyhook "gitlab.com/gitlab-org/gitaly/internal/gitaly/hook" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/linguist" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" "gitlab.com/gitlab-org/gitaly/internal/gitaly/service/blob" "gitlab.com/gitlab-org/gitaly/internal/gitaly/service/cleanup" @@ -68,10 +69,11 @@ func RegisterAll( locator storage.Locator, conns *client.Pool, gitCmdFactory git.CommandFactory, + ling *linguist.Instance, ) { gitalypb.RegisterBlobServiceServer(grpcServer, blob.NewServer(rubyServer, cfg, locator, gitCmdFactory)) gitalypb.RegisterCleanupServiceServer(grpcServer, cleanup.NewServer(cfg, gitCmdFactory)) - gitalypb.RegisterCommitServiceServer(grpcServer, commit.NewServer(cfg, locator, gitCmdFactory)) + gitalypb.RegisterCommitServiceServer(grpcServer, commit.NewServer(cfg, locator, gitCmdFactory, ling)) gitalypb.RegisterDiffServiceServer(grpcServer, diff.NewServer(cfg, locator, gitCmdFactory)) gitalypb.RegisterNamespaceServiceServer(grpcServer, namespace.NewServer(locator)) gitalypb.RegisterOperationServiceServer(grpcServer, operations.NewServer(cfg, rubyServer, hookManager, locator, conns, gitCmdFactory)) diff --git a/internal/gitaly/service/repository/fetch_test.go b/internal/gitaly/service/repository/fetch_test.go index d22cdb51b..c645059b0 100644 --- a/internal/gitaly/service/repository/fetch_test.go +++ b/internal/gitaly/service/repository/fetch_test.go @@ -351,7 +351,7 @@ func runFullSecureServer(t *testing.T, locator storage.Locator) (*grpc.Server, s require.NoError(t, err) listener, addr := testhelper.GetLocalhostListener(t) - service.RegisterAll(server, cfg, repository.RubyServer, hookManager, txManager, config.NewLocator(cfg), conns, gitCmdFactory) + service.RegisterAll(server, cfg, repository.RubyServer, hookManager, txManager, config.NewLocator(cfg), conns, gitCmdFactory, nil) errQ := make(chan error) // This creates a secondary GRPC server which isn't "secure". Reusing |