Skip to content

Commit

Permalink
Remove Validating cache (bazelbuild#141)
Browse files Browse the repository at this point in the history 
Since we are not using the mtime based validating cache anywhere and
dont have immediate plans of using it due to performance reasons, I'm
removing this code. This would make it a little bit simpler to update
the cache entry of the output files.

Test: Unit tests
  • Loading branch information
@gkousik
gkousik authored Jun 25, 2020
1 parent 17eef19 commit 6e74127
Show file tree
Hide file tree
Showing 6 changed files with 9 additions and 110 deletions.
2 changes: 2 additions & 0 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ github.com/golang/protobuf v1.3.1 h1:YF8+flBXS5eO826T4nzqPrxfhQThhXl0YzfuUPu4SBg
github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
github.com/golang/protobuf v1.3.2 h1:6nsPYzhq5kReh6QImI3k5qWzO4PEbvbIW2cwSfR/6xs=
github.com/golang/protobuf v1.3.2/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
github.com/golang/protobuf v1.3.3 h1:gyjaxf+svBWX08ZjK86iN9geUJF0H6gp2IRKX6Nf6/I=
github.com/golang/protobuf v1.3.3/go.mod h1:vzj43D7+SQXF/4pzW/hwtAqwc6iTitCiVSaWz5lYuqw=
github.com/golang/snappy v0.0.0-20180518054509-2e65f85255db h1:woRePGFeVFfLKN/pOkfl+p/TAqKOfFu+7KPlMVpok/w=
github.com/golang/snappy v0.0.0-20180518054509-2e65f85255db/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q=
Expand Down Expand Up @@ -370,6 +371,7 @@ google.golang.org/grpc v1.23.0/go.mod h1:Y5yQAOtifL1yxbo5wqy6BxZv8vAUGQwXBOALyac
google.golang.org/grpc v1.24.0 h1:vb/1TCsVn3DcJlQ0Gs1yB1pKI6Do2/QNwxdKqmc/b0s=
google.golang.org/grpc v1.24.0/go.mod h1:XDChyiUovWa60DnaeDeZmSW86xtLtjtZbwvSiRnRtcA=
google.golang.org/grpc v1.25.1/go.mod h1:c3i+UQWmh7LiEpx4sFZnkU36qjEYZ0imhYfXVyQciAY=
google.golang.org/grpc v1.29.1 h1:EC2SB8S04d2r73uptxphDSUG+kTKVgjRPF+N3xpxRB4=
google.golang.org/grpc v1.29.1/go.mod h1:itym6AZVZYACWQqET3MqgPpjcuV5QH3BxFS3IjizoKk=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
Expand Down
35 changes: 0 additions & 35 deletions go/pkg/filemetadata/cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@ package filemetadata

import (
"fmt"
"os"
"path/filepath"
"sync/atomic"
"time"

"github.com/bazelbuild/remote-apis-sdks/go/pkg/cache"
)
Expand All @@ -19,19 +17,13 @@ type fmCache struct {
Backend *cache.Cache
cacheHits uint64
cacheMisses uint64
Validate bool
}

// NewSingleFlightCache returns a singleton-backed in-memory cache, with no validation.
func NewSingleFlightCache() Cache {
return &fmCache{Backend: cache.GetInstance()}
}

// NewSingleFlightValidatedCache returns a singleton-backed in-memory cache, with validation.
func NewSingleFlightValidatedCache() Cache {
return &fmCache{Backend: cache.GetInstance(), Validate: true}
}

// Get retrieves the metadata of the file with the given filename, whether from cache or by
// computing the digest.
func (c *fmCache) Get(filename string) *Metadata {
Expand All @@ -46,25 +38,6 @@ func (c *fmCache) Get(filename string) *Metadata {
if err != nil {
return &Metadata{Err: err}
}
if !c.Validate {
c.updateMetrics(ch)
return md
}
valid, err := c.validate(abs, md.MTime)
if err != nil {
return &Metadata{Err: err}
}
if valid {
c.updateMetrics(ch)
return md
}
if err = c.Backend.Delete(namespace, abs); err != nil {
return &Metadata{Err: err}
}
md, ch, err = c.loadMetadata(abs)
if err != nil {
return &Metadata{Err: err}
}
c.updateMetrics(ch)
return md
}
Expand Down Expand Up @@ -103,14 +76,6 @@ func (c *fmCache) check() error {
return nil
}

func (c *fmCache) validate(filename string, mtime time.Time) (bool, error) {
file, err := os.Stat(filename)
if err != nil {
return false, err
}
return file.ModTime().Equal(mtime), nil
}

func (c *fmCache) loadMetadata(filename string) (*Metadata, bool, error) {
cacheHit := true
val, err := c.Backend.LoadOrStore(namespace, filename, func() (interface{}, error) {
Expand Down
3 changes: 1 addition & 2 deletions go/pkg/filemetadata/cache_posix_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"testing"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
)

func TestExecutableCacheLoad(t *testing.T) {
Expand All @@ -28,7 +27,7 @@ func TestExecutableCacheLoad(t *testing.T) {
Digest: wantDg,
IsExecutable: true,
}
if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(Metadata{}, "MTime")); diff != "" {
if diff := cmp.Diff(want, got); diff != "" {
t.Errorf("Get(%v) returned diff. (-want +got)\n%s", filename, diff)
}
}
60 changes: 5 additions & 55 deletions go/pkg/filemetadata/cache_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,9 @@ import (
"io/ioutil"
"os"
"testing"
"time"

"github.com/bazelbuild/remote-apis-sdks/go/pkg/cache"
"github.com/bazelbuild/remote-apis-sdks/go/pkg/digest"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
)

var (
Expand All @@ -34,12 +31,13 @@ func TestSimpleCacheLoad(t *testing.T) {
Digest: wantDg,
IsExecutable: false,
}
if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(Metadata{}, "MTime")); diff != "" {
if diff := cmp.Diff(want, got); diff != "" {
t.Errorf("Get(%v) returned diff. (-want +got)\n%s", filename, diff)
}
if c.GetCacheHits() != 0 {
t.Errorf("Cache has wrong num of CacheHits, want 0, got %v", c.GetCacheHits())
}

if c.GetCacheMisses() != 1 {
t.Errorf("Cache has wrong num of CacheMisses, want 1, got %v", c.GetCacheMisses())
}
Expand All @@ -63,7 +61,7 @@ func TestCacheOnceLoadMultiple(t *testing.T) {
if got.Err != nil {
t.Errorf("Get(%v) failed. Got error: %v", filename, got.Err)
}
if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(Metadata{}, "MTime")); diff != "" {
if diff := cmp.Diff(want, got); diff != "" {
t.Errorf("Get(%v) returned diff. (-want +got)\n%s", filename, diff)
}
}
Expand Down Expand Up @@ -92,13 +90,10 @@ func TestLoadAfterChangeWithoutValidation(t *testing.T) {
Digest: wantDg,
IsExecutable: false,
}
if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(Metadata{}, "MTime")); diff != "" {
if diff := cmp.Diff(want, got); diff != "" {
t.Fatalf("Get(%v) returned diff. (-want +got)\n%s", filename, diff)
}

// Sleep to avoid mtime not being updated between writes.
time.Sleep(time.Second)

change := []byte("change")
if err = ioutil.WriteFile(filename, change, os.ModeTemporary); err != nil {
t.Fatalf("Failed to write to tmp file for testing digests: %v", err)
Expand All @@ -107,7 +102,7 @@ func TestLoadAfterChangeWithoutValidation(t *testing.T) {
if got.Err != nil {
t.Errorf("Get(%v) failed. Got error: %v", filename, got.Err)
}
if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(Metadata{}, "MTime")); diff != "" {
if diff := cmp.Diff(want, got); diff != "" {
t.Errorf("Get(%v) returned diff. (-want +got)\n%s", filename, diff)
}
if c.GetCacheHits() != 1 {
Expand All @@ -117,48 +112,3 @@ func TestLoadAfterChangeWithoutValidation(t *testing.T) {
t.Errorf("Cache has wrong num of CacheMisses, want 1, got %v", c.GetCacheMisses())
}
}

func TestLoadAfterChange(t *testing.T) {
c := &fmCache{Backend: cache.GetInstance(), Validate: true}
filename, err := createFile(t, false, "")
if err != nil {
t.Fatalf("Failed to create tmp file for testing digests: %v", err)
}
if err = ioutil.WriteFile(filename, contents, os.ModeTemporary); err != nil {
t.Fatalf("Failed to write to tmp file for testing digests: %v", err)
}
got := c.Get(filename)
if got.Err != nil {
t.Fatalf("Get(%v) failed. Got error: %v", filename, got.Err)
}
want := &Metadata{
Digest: wantDg,
IsExecutable: false,
}
if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(Metadata{}, "MTime")); diff != "" {
t.Fatalf("Get(%v) returned diff. (-want +got)\n%s", filename, diff)
}

// Sleep to avoid mtime not being updated between writes.
time.Sleep(time.Second)

change := []byte("change")
digestAfterChange := digest.NewFromBlob(change)
if err = ioutil.WriteFile(filename, change, os.ModeTemporary); err != nil {
t.Fatalf("Failed to write to tmp file for testing digests: %v", err)
}
got = c.Get(filename)
if got.Err != nil {
t.Errorf("Get(%v) failed. Got error: %v", filename, got.Err)
}
want.Digest = digestAfterChange
if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(Metadata{}, "MTime")); diff != "" {
t.Errorf("Get(%v) returned diff. (-want +got)\n%s", filename, diff)
}
if c.GetCacheHits() != 0 {
t.Errorf("Cache has wrong num of CacheHits, want 0, got %v", c.GetCacheHits())
}
if c.GetCacheMisses() != 2 {
t.Errorf("Cache has wrong num of CacheMisses, want 2, got %v", c.GetCacheMisses())
}
}
3 changes: 0 additions & 3 deletions go/pkg/filemetadata/filemetadata.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ package filemetadata
import (
"fmt"
"os"
"time"

"github.com/bazelbuild/remote-apis-sdks/go/pkg/digest"
)
Expand All @@ -13,7 +12,6 @@ import (
type Metadata struct {
Digest digest.Digest
IsExecutable bool
MTime time.Time
Err error
}

Expand Down Expand Up @@ -54,7 +52,6 @@ func Compute(filename string) *Metadata {
md.Err = fe
return md
}
md.MTime = file.ModTime()
mode := file.Mode()
md.IsExecutable = (mode & 0100) != 0
if mode.IsDir() {
Expand Down
16 changes: 1 addition & 15 deletions go/pkg/filemetadata/filemetadata_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,9 @@ import (
"io/ioutil"
"os"
"testing"
"time"

"github.com/bazelbuild/remote-apis-sdks/go/pkg/digest"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
)

func TestCompute(t *testing.T) {
Expand All @@ -33,14 +31,11 @@ func TestCompute(t *testing.T) {
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
margin := time.Second
before := time.Now().Truncate(margin)
filename, err := createFile(t, tc.executable, tc.contents)
if err != nil {
t.Fatalf("Failed to create tmp file for testing digests: %v", err)
}
defer os.RemoveAll(filename)
after := time.Now().Truncate(margin).Add(margin)
got := Compute(filename)
if got.Err != nil {
t.Errorf("Compute(%v) failed. Got error: %v", filename, got.Err)
Expand All @@ -49,35 +44,26 @@ func TestCompute(t *testing.T) {
Digest: digest.NewFromBlob([]byte(tc.contents)),
IsExecutable: tc.executable,
}
if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(Metadata{}, "MTime")); diff != "" {
if diff := cmp.Diff(want, got); diff != "" {
t.Errorf("Compute(%v) returned diff. (-want +got)\n%s", filename, diff)
}
if got.MTime.Before(before) || got.MTime.After(after) {
t.Errorf("Compute(%v) returned MTime %v, want time in (%v, %v).", filename, got.MTime, before, after)
}
})
}
}

func TestComputeDirectory(t *testing.T) {
margin := time.Second
before := time.Now().Truncate(margin)
tmpDir, err := ioutil.TempDir("", "")
defer os.RemoveAll(tmpDir)
if err != nil {
t.Fatalf("Failed to create temp directory")
}
after := time.Now().Truncate(margin).Add(margin)
got := Compute(tmpDir)
if fe, ok := got.Err.(*FileError); !ok || !fe.IsDirectory {
t.Errorf("Compute(%v).Err = %v, want FileError{IsDirectory:true}", tmpDir, got.Err)
}
if got.Digest != digest.Empty {
t.Errorf("Compute(%v).Digest = %v, want %v", tmpDir, got.Digest, digest.Empty)
}
if got.MTime.Before(before) || got.MTime.After(after) {
t.Errorf("Compute(%v) returned MTime %v, want time in (%v, %v).", tmpDir, got.MTime, before, after)
}
}

func createFile(t *testing.T, executable bool, contents string) (string, error) {
Expand Down

0 comments on commit 6e74127

Please sign in to comment.