diff options
author | jramsay <jcai@gitlab.com> | 2019-12-12 04:08:23 +0300 |
---|---|---|
committer | jramsay <jcai@gitlab.com> | 2019-12-12 21:33:30 +0300 |
commit | f8f80213af469cd328a3b4b20d34632788912b3d (patch) | |
tree | 46cabdd955556ea12cab45400471b40bcb835560 | |
parent | 31ca324da09b003c7a491edd25880e4e560faab7 (diff) |
Remove ruby script approach to GetAllLFSPointers
-rw-r--r-- | changelogs/unreleased/jc-remove-ruby-get-all-lfs.yml | 5 | ||||
-rw-r--r-- | internal/metadata/featureflag/feature_flags.go | 2 | ||||
-rw-r--r-- | internal/service/blob/lfs_pointers.go | 163 | ||||
-rw-r--r-- | internal/service/blob/lfs_pointers_test.go | 8 |
4 files changed, 5 insertions, 173 deletions
diff --git a/changelogs/unreleased/jc-remove-ruby-get-all-lfs.yml b/changelogs/unreleased/jc-remove-ruby-get-all-lfs.yml new file mode 100644 index 000000000..69aba10d1 --- /dev/null +++ b/changelogs/unreleased/jc-remove-ruby-get-all-lfs.yml @@ -0,0 +1,5 @@ +--- +title: Remove ruby script approach to GetAllLFSPointers +merge_request: 1695 +author: +type: other diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index f220bd5ac..3a4a33031 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -4,8 +4,6 @@ const ( // UploadPackFilter enables partial clones by sending uploadpack.allowFilter and uploadpack.allowAnySHA1InWant // to upload-pack UploadPackFilter = "upload_pack_filter" - // GetAllLFSPointersGo will cause the GetAllLFSPointers RPC to use the go implementation when set - GetAllLFSPointersGo = "get_all_lfs_pointers_go" // GetTagMessagesGo will cause the GetTagMessages RPC to use the go implementation when set GetTagMessagesGo = "get_tag_messages_go" // FilterShasWithSignaturesGo will cause the FilterShasWithSignatures RPC to use the go implementation when set diff --git a/internal/service/blob/lfs_pointers.go b/internal/service/blob/lfs_pointers.go index e904a5139..4dfbd27ab 100644 --- a/internal/service/blob/lfs_pointers.go +++ b/internal/service/blob/lfs_pointers.go @@ -1,22 +1,10 @@ package blob import ( - "bufio" - "bytes" "fmt" - "io" - "os" - "os/exec" - "strings" - "github.com/prometheus/client_golang/prometheus" - "gitlab.com/gitlab-org/gitaly/internal/command" - "gitlab.com/gitlab-org/gitaly/internal/config" "gitlab.com/gitlab-org/gitaly/internal/git" - "gitlab.com/gitlab-org/gitaly/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/internal/helper" - "gitlab.com/gitlab-org/gitaly/internal/helper/chunk" - "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/rubyserver" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" @@ -117,18 +105,6 @@ func (s *server) GetNewLFSPointers(in *gitalypb.GetNewLFSPointersRequest, stream }) } -var getAllLFSPointersRequests = prometheus.NewCounterVec( - prometheus.CounterOpts{ - Name: "gitaly_get_all_lfs_pointers_total", - Help: "Counter of go vs ruby implementation of GetAllLFSPointers", - }, - []string{"implementation"}, -) - -func init() { - prometheus.MustRegister(getAllLFSPointersRequests) -} - func (s *server) GetAllLFSPointers(in *gitalypb.GetAllLFSPointersRequest, stream gitalypb.BlobService_GetAllLFSPointersServer) error { ctx := stream.Context() @@ -136,18 +112,6 @@ func (s *server) GetAllLFSPointers(in *gitalypb.GetAllLFSPointersRequest, stream return helper.ErrInvalidArgument(err) } - if featureflag.IsEnabled(stream.Context(), featureflag.GetAllLFSPointersGo) { - getAllLFSPointersRequests.WithLabelValues("go").Inc() - - if err := getAllLFSPointersRubyScript(in.GetRepository(), stream); err != nil { - return helper.ErrInternal(err) - } - - return nil - } - - getAllLFSPointersRequests.WithLabelValues("ruby").Inc() - client, err := s.ruby.BlobServiceClient(ctx) if err != nil { return err @@ -181,130 +145,3 @@ func validateGetLfsPointersByRevisionRequest(in getLFSPointerByRevisionRequest) return git.ValidateRevision(in.GetRevision()) } - -type allLFSPointersSender struct { - stream gitalypb.BlobService_GetAllLFSPointersServer - lfsPointers []*gitalypb.LFSPointer -} - -func (s *allLFSPointersSender) Reset() { s.lfsPointers = nil } -func (s *allLFSPointersSender) Append(it chunk.Item) { - s.lfsPointers = append(s.lfsPointers, it.(*gitalypb.LFSPointer)) -} - -func (s *allLFSPointersSender) Send() error { - return s.stream.Send(&gitalypb.GetAllLFSPointersResponse{LfsPointers: s.lfsPointers}) -} - -func getAllLFSPointersRubyScript(repository *gitalypb.Repository, stream gitalypb.BlobService_GetAllLFSPointersServer) error { - repoPath, err := helper.GetRepoPath(repository) - if err != nil { - return err - } - - ctx := stream.Context() - - cmd := exec.Command( - "ruby", - "--", - "-", - config.Config.Git.BinPath, - repoPath, - fmt.Sprintf("%d", LfsPointerMinSize), - fmt.Sprintf("%d", LfsPointerMaxSize), - ) - cmd.Dir = config.Config.Ruby.Dir - ruby, err := command.New(ctx, cmd, strings.NewReader(rubyScript), nil, nil, os.Environ()...) - if err != nil { - return err - } - - if err := parseCatfileOut(ruby, stream); err != nil { - return err - } - - return ruby.Wait() -} - -func parseCatfileOut(_r io.Reader, stream gitalypb.BlobService_GetAllLFSPointersServer) error { - chunker := chunk.New(&allLFSPointersSender{stream: stream, lfsPointers: nil}) - - r := bufio.NewReader(_r) - buf := &bytes.Buffer{} - for { - _, err := r.Peek(1) - if err == io.EOF { - break - } - if err != nil { - return err - } - - info, err := catfile.ParseObjectInfo(r) - if err != nil { - return err - } - - buf.Reset() - _, err = io.CopyN(buf, r, info.Size) - if err != nil { - return err - } - delim, err := r.ReadByte() - if err != nil { - return err - } - if delim != '\n' { - return fmt.Errorf("unexpected character %x", delim) - } - - if !git.IsLFSPointer(buf.Bytes()) { - continue - } - - b := make([]byte, buf.Len()) - copy(b, buf.Bytes()) - if err := chunker.Send(&gitalypb.LFSPointer{ - Oid: info.Oid, - Size: info.Size, - Data: b, - }); err != nil { - return err - } - } - - return chunker.Flush() -} - -var rubyScript = ` - -def main(git_bin, git_dir, minSize, maxSize) - IO.popen(%W[#{git_bin} -C #{git_dir} rev-list --all --filter=blob:limit=#{maxSize+1} --in-commit-order --objects], 'r') do |rev_list| - # Loading bundler and rugged is slow. Let's do it while we wait for git rev-list. - require 'bundler/setup' - require 'rugged' - - # disable encoding conversion: we want to read and write data as-is - rev_list.binmode - $stdout.binmode - - repo = Rugged::Repository.new(git_dir) - - rev_list.each_line do |line| - oid = line.split(' ', 2).first - abort "bad rev-list line #{line.inspect}" unless oid - - header = repo.read_header(oid) - next unless header[:len] >= minSize && header[:len] <= maxSize && header[:type] == :blob - - puts "#{oid} blob #{header[:len]}" - $stdout.write(repo.lookup(oid).content) - puts # newline separator, just like git cat-file - end - end - - abort 'rev-list failed' unless $?.success? -end - -main(ARGV[0], ARGV[1], Integer(ARGV[2]), Integer(ARGV[3])) -` diff --git a/internal/service/blob/lfs_pointers_test.go b/internal/service/blob/lfs_pointers_test.go index 6b86ce7b1..5f40baa07 100644 --- a/internal/service/blob/lfs_pointers_test.go +++ b/internal/service/blob/lfs_pointers_test.go @@ -6,7 +6,6 @@ import ( "testing" "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" @@ -406,13 +405,6 @@ func TestSuccessfulGetAllLFSPointersRequest(t *testing.T) { require.NoError(t, err) require.ElementsMatch(t, expectedLFSPointers, getAllPointers(t, c)) - - // test with go implementation - // TODO: remove once feature flag is removed - c, err = client.GetAllLFSPointers(featureflag.ContextWithFeatureFlag(ctx, featureflag.GetAllLFSPointersGo), request) - require.NoError(t, err) - - require.ElementsMatch(t, expectedLFSPointers, getAllPointers(t, c)) } func getAllPointers(t *testing.T, c gitalypb.BlobService_GetAllLFSPointersClient) []*gitalypb.LFSPointer { |