From 34281badfc50dede0ea8e2140534951d501ead2f Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Mon, 23 Sep 2019 08:21:09 -0700 Subject: Revert "Merge branch 'po-fix-cache-invalidator-no-repo' into 'master'" This reverts commit d2dc9d4b87c5e0d3416a78b3d47321078e5f93f9, reversing changes made to 10d40838934326be0bc68e28ea3450be2d3b039c. --- .../unreleased/po-fix-cache-invalidator-no-repo.yml | 5 ----- internal/cache/cachedb_test.go | 5 ----- internal/cache/keyer.go | 10 +++++----- internal/middleware/cache/cache.go | 11 ++--------- internal/middleware/cache/cache_test.go | 17 +++-------------- internal/middleware/cache/export_test.go | 20 -------------------- 6 files changed, 10 insertions(+), 58 deletions(-) delete mode 100644 changelogs/unreleased/po-fix-cache-invalidator-no-repo.yml delete mode 100644 internal/middleware/cache/export_test.go diff --git a/changelogs/unreleased/po-fix-cache-invalidator-no-repo.yml b/changelogs/unreleased/po-fix-cache-invalidator-no-repo.yml deleted file mode 100644 index fef61d144..000000000 --- a/changelogs/unreleased/po-fix-cache-invalidator-no-repo.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix cache invalidator for Create* RPCs and health checks -merge_request: 1494 -author: -type: fixed diff --git a/internal/cache/cachedb_test.go b/internal/cache/cachedb_test.go index 1fe267fa8..1f006aedb 100644 --- a/internal/cache/cachedb_test.go +++ b/internal/cache/cachedb_test.go @@ -101,9 +101,4 @@ func TestStreamDBNaiveKeyer(t *testing.T) { // only completing/removing the pending generation file will allow access require.NoError(t, repo1Lease.EndLease(ctx)) expectGetMiss(req1) - - // creating a lease on a repo that doesn't exist yet should succeed - req1.Repository.RelativePath += "-does-not-exist" - _, err = keyer.StartLease(req1.Repository) - require.NoError(t, err) } diff --git a/internal/cache/keyer.go b/internal/cache/keyer.go index cdebd4fa9..ab60e7f63 100644 --- a/internal/cache/keyer.go +++ b/internal/cache/keyer.go @@ -76,7 +76,7 @@ func (l lease) EndLease(ctx context.Context) error { } func updateLatest(repo *gitalypb.Repository) (string, error) { - repoPath, err := helper.GetPath(repo) + repoPath, err := helper.GetRepoPath(repo) if err != nil { return "", err } @@ -141,7 +141,7 @@ func (LeaseKeyer) KeyPath(ctx context.Context, repo *gitalypb.Repository, req pr return "", err } - repoPath, err := helper.GetPath(repo) + repoPath, err := helper.GetRepoPath(repo) if err != nil { return "", err } @@ -190,7 +190,7 @@ func radixPath(root, key string) (string, error) { } func newPendingLease(repo *gitalypb.Repository) (string, error) { - repoPath, err := helper.GetPath(repo) + repoPath, err := helper.GetRepoPath(repo) if err != nil { return "", err } @@ -223,7 +223,7 @@ func cacheDir(repo *gitalypb.Repository) (string, error) { } func currentLeases(repo *gitalypb.Repository) ([]os.FileInfo, error) { - repoPath, err := helper.GetPath(repo) + repoPath, err := helper.GetRepoPath(repo) if err != nil { return nil, err } @@ -243,7 +243,7 @@ func currentLeases(repo *gitalypb.Repository) ([]os.FileInfo, error) { } func currentGenID(repo *gitalypb.Repository) (string, error) { - repoPath, err := helper.GetPath(repo) + repoPath, err := helper.GetRepoPath(repo) if err != nil { return "", err } diff --git a/internal/middleware/cache/cache.go b/internal/middleware/cache/cache.go index 3400f9b00..2a65c6447 100644 --- a/internal/middleware/cache/cache.go +++ b/internal/middleware/cache/cache.go @@ -3,7 +3,6 @@ package cache import ( "context" "fmt" - "strings" "sync" "github.com/golang/protobuf/proto" @@ -33,16 +32,11 @@ func methodErrLogger(method string) func(error) { } } -func shouldIgnore(fullMethod string) bool { - return strings.HasPrefix(fullMethod, "/grpc.health") -} - // StreamInvalidator will invalidate any mutating RPC that targets a // repository in a gRPC stream based RPC func StreamInvalidator(ci Invalidator, reg *protoregistry.Registry) grpc.StreamServerInterceptor { return func(srv interface{}, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error { - if !featureflag.IsEnabled(ss.Context(), FeatureFlag) || - shouldIgnore(info.FullMethod) { + if !featureflag.IsEnabled(ss.Context(), FeatureFlag) { return handler(srv, ss) } @@ -69,8 +63,7 @@ func StreamInvalidator(ci Invalidator, reg *protoregistry.Registry) grpc.StreamS // repository in a gRPC unary RPC func UnaryInvalidator(ci Invalidator, reg *protoregistry.Registry) grpc.UnaryServerInterceptor { return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) { - if !featureflag.IsEnabled(ctx, FeatureFlag) || - shouldIgnore(info.FullMethod) { + if !featureflag.IsEnabled(ctx, FeatureFlag) { return handler(ctx, req) } diff --git a/internal/middleware/cache/cache_test.go b/internal/middleware/cache/cache_test.go index 3f5f4065f..4296d2e4e 100644 --- a/internal/middleware/cache/cache_test.go +++ b/internal/middleware/cache/cache_test.go @@ -19,8 +19,6 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/praefect/protoregistry" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc" - "google.golang.org/grpc/health" - "google.golang.org/grpc/health/grpc_health_v1" "google.golang.org/grpc/metadata" ) @@ -51,7 +49,7 @@ func TestInvalidators(t *testing.T) { svc := &testSvc{} - cli, cc, cleanup := newTestSvc(t, ctx, srvr, svc) + cli, cleanup := newTestSvc(t, ctx, srvr, svc) defer cleanup() repo1 := &gitalypb.Repository{ @@ -120,12 +118,6 @@ func TestInvalidators(t *testing.T) { }) require.NoError(t, err) - // Health checks should NOT trigger cache invalidation - hcr := &grpc_health_v1.HealthCheckRequest{Service: "TestService"} - _, err = grpc_health_v1.NewHealthClient(cc).Check(ctx, hcr) - require.NoError(t, err) - require.Equal(t, 0, cache.MethodErrCount.Method["/grpc.health.v1.Health/Check"]) - require.Equal(t, expectedInvalidations, mCache.(*mockCache).invalidatedRepos) require.Equal(t, expectedSvcRequests, svc.repoRequests) require.Equal(t, 3, mCache.(*mockCache).endedLeases.count) @@ -175,10 +167,7 @@ func streamFileDesc(t testing.TB) *descriptor.FileDescriptorProto { return fdp } -func newTestSvc(t testing.TB, ctx context.Context, srvr *grpc.Server, svc testdata.TestServiceServer) (testdata.TestServiceClient, *grpc.ClientConn, func()) { - healthSrvr := health.NewServer() - grpc_health_v1.RegisterHealthServer(srvr, healthSrvr) - healthSrvr.SetServingStatus("TestService", grpc_health_v1.HealthCheckResponse_SERVING) +func newTestSvc(t testing.TB, ctx context.Context, srvr *grpc.Server, svc testdata.TestServiceServer) (testdata.TestServiceClient, func()) { testdata.RegisterTestServiceServer(srvr, svc) lis, err := net.Listen("tcp", ":0") @@ -203,7 +192,7 @@ func newTestSvc(t testing.TB, ctx context.Context, srvr *grpc.Server, svc testda ) require.NoError(t, err) - return testdata.NewTestServiceClient(cc), cc, cleanup + return testdata.NewTestServiceClient(cc), cleanup } type testSvc struct { diff --git a/internal/middleware/cache/export_test.go b/internal/middleware/cache/export_test.go deleted file mode 100644 index 48c1dab84..000000000 --- a/internal/middleware/cache/export_test.go +++ /dev/null @@ -1,20 +0,0 @@ -package cache - -import "sync" - -var MethodErrCount = struct { - sync.Mutex - Method map[string]int -}{ - Method: map[string]int{}, -} - -func init() { - // override prometheus counter to detect any errors logged for a specific - // method - countMethodErr = func(method string) { - MethodErrCount.Lock() - MethodErrCount.Method[method]++ - MethodErrCount.Unlock() - } -} -- cgit v1.2.3