Repository: buchgr/bazel-remote
Pull request #471 (2021-09-13 06:30:27)

[disk] refactor to use the options pattern
The dir and max_size settings remain as regular arguments since they're always required.


Patch:
diff --git a/cache/disk/BUILD.bazel b/cache/disk/BUILD.bazel
index 3d3fb8f..0257c78 100644
--- a/cache/disk/BUILD.bazel
+++ b/cache/disk/BUILD.bazel
@@ -6,6 +6,7 @@ go_library(
         "disk.go",
         "findmissing.go",
         "lru.go",
+        "options.go",
     ],
     importpath = "github.com/buchgr/bazel-remote/cache/disk",
     visibility = ["//visibility:public"],
diff --git a/cache/disk/disk.go b/cache/disk/disk.go
index d3963ca..750cd5a 100644
--- a/cache/disk/disk.go
+++ b/cache/disk/disk.go
@@ -8,6 +8,7 @@ import (
 	"io"
 	"io/ioutil"
 	"log"
+	"math"
 	"net/http"
 	"os"
 	"path"
@@ -101,9 +102,35 @@ func badReqErr(format string, a ...interface{}) *cache.Error {
 }
 
 // New returns a new instance of a filesystem-based cache rooted at `dir`,
-// with a maximum size of `maxSizeBytes` bytes, maximum logical blob size
-// `maxBlobSize` and an optional backend `proxy`.
-func New(dir string, maxSizeBytes int64, maxBlobSize int64, storageMode string, proxy cache.Proxy, accessLogger *log.Logger) (*Cache, error) {
+// with a maximum size of `maxSizeBytes` bytes and `opts` Options set.
+func New(dir string, maxSizeBytes int64, opts ...Option) (*Cache, error) {
+
+	err := os.MkdirAll(dir, os.ModePerm)
+	if err != nil {
+		return nil, err
+	}
+
+	dir, err = filepath.EvalSymlinks(dir)
+	if err != nil {
+		return nil, err
+	}
+
+	c := &Cache{
+		dir: dir,
+
+		// Not using config here, to avoid test import cycles.
+		storageMode: casblob.Zstandard,
+		maxBlobSize: math.MaxInt64,
+	}
+
+	// Apply options.
+	for _, o := range opts {
+		err = o(c)
+		if err != nil {
+			return nil, err
+		}
+	}
+
 	// Create the directory structure.
 	hexLetters := []byte("0123456789abcdef")
 	for _, c1 := range hexLetters {
@@ -124,28 +151,6 @@ func New(dir string, maxSizeBytes int64, maxBlobSize int64, storageMode string,
 		}
 	}
 
-	compressionType := casblob.Zstandard
-	if storageMode == "uncompressed" {
-		compressionType = casblob.Identity
-	}
-
-	resolved, err := filepath.EvalSymlinks(dir)
-	if err != nil {
-		return nil, err
-	}
-
-	c := &Cache{
-		dir:          resolved,
-		storageMode:  compressionType,
-		proxy:        proxy,
-		maxBlobSize:  maxBlobSize,
-		accessLogger: accessLogger,
-	}
-
-	if proxy != nil {
-		c.spawnContainsQueueWorkers()
-	}
-
 	// The eviction callback deletes the file from disk.
 	// This function is only called while the lock is held
 	// by the current goroutine.
diff --git a/cache/disk/options.go b/cache/disk/options.go
new file mode 100644
index 0000000..f8a2ea7
--- /dev/null
+++ b/cache/disk/options.go
@@ -0,0 +1,58 @@
+package disk
+
+import (
+	"fmt"
+	"log"
+
+	"github.com/buchgr/bazel-remote/cache"
+	"github.com/buchgr/bazel-remote/cache/disk/casblob"
+)
+
+type Option func(*Cache) error
+
+func WithStorageMode(mode string) Option {
+	return func(c *Cache) error {
+		if mode == "zstd" {
+			c.storageMode = casblob.Zstandard
+			return nil
+		} else if mode == "uncompressed" {
+			c.storageMode = casblob.Identity
+			return nil
+		} else {
+			return fmt.Errorf("Unsupported storage mode: " + mode)
+		}
+	}
+}
+
+func WithMaxBlobSize(size int64) Option {
+	return func(c *Cache) error {
+		if size <= 0 {
+			return fmt.Errorf("Invalid MaxBlobSize: %d", size)
+		}
+
+		c.maxBlobSize = size
+		return nil
+	}
+}
+
+func WithProxyBackend(proxy cache.Proxy) Option {
+	return func(c *Cache) error {
+		if c.proxy != nil && proxy != nil {
+			return fmt.Errorf("Proxy backends may be set only once")
+		}
+
+		if proxy != nil {
+			c.proxy = proxy
+			c.spawnContainsQueueWorkers()
+		}
+
+		return nil
+	}
+}
+
+func WithAccessLogger(logger *log.Logger) Option {
+	return func(c *Cache) error {
+		c.accessLogger = logger
+		return nil
+	}
+}
diff --git a/main.go b/main.go
index 67d6443..95c0269 100644
--- a/main.go
+++ b/main.go
@@ -81,7 +81,14 @@ func run(ctx *cli.Context) error {
 
 	rlimit.Raise()
 
-	diskCache, err := disk.New(c.Dir, int64(c.MaxSize)*1024*1024*1024, c.MaxBlobSize, c.StorageMode, c.ProxyBackend, c.AccessLogger)
+	opts := []disk.Option{
+		disk.WithStorageMode(c.StorageMode),
+		disk.WithMaxBlobSize(c.MaxBlobSize),
+		disk.WithProxyBackend(c.ProxyBackend),
+		disk.WithAccessLogger(c.AccessLogger),
+	}
+
+	diskCache, err := disk.New(c.Dir, int64(c.MaxSize)*1024*1024*1024, opts...)
 	if err != nil {
 		log.Fatal(err)
 	}


Test patch:
diff --git a/cache/disk/disk_test.go b/cache/disk/disk_test.go
index c0d39c9..484eb35 100644
--- a/cache/disk/disk_test.go
+++ b/cache/disk/disk_test.go
@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
-	"math"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -53,7 +52,7 @@ func TestCacheBasics(t *testing.T) {
 	// Add some overhead for likely CAS blob storage expansion.
 	cacheSize := int64(itemSize*2 + BlockSize)
 
-	testCache, err := New(cacheDir, cacheSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	testCache, err := New(cacheDir, cacheSize, WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -96,7 +95,7 @@ func TestCacheBasics(t *testing.T) {
 func TestCachePutWrongSize(t *testing.T) {
 	cacheDir := tempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache, err := New(cacheDir, BlockSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	testCache, err := New(cacheDir, BlockSize, WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -132,7 +131,7 @@ func TestCacheGetContainsWrongSize(t *testing.T) {
 
 	cacheDir := tempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache, err := New(cacheDir, BlockSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	testCache, err := New(cacheDir, BlockSize, WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -172,7 +171,7 @@ func TestCacheGetContainsWrongSizeWithProxy(t *testing.T) {
 
 	cacheDir := tempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache, err := New(cacheDir, BlockSize, math.MaxInt64, "zstd", new(proxyStub), testutils.NewSilentLogger())
+	testCache, err := New(cacheDir, BlockSize, WithProxyBackend(new(proxyStub)), WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -316,7 +315,7 @@ func TestOverwrite(t *testing.T) {
 	cacheDir := tempDir(t)
 	defer os.RemoveAll(cacheDir)
 
-	testCache, err := New(cacheDir, BlockSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	testCache, err := New(cacheDir, BlockSize, WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -416,7 +415,7 @@ func TestCacheExistingFiles(t *testing.T) {
 	// Add some overhead for likely CAS blob storage expansion.
 	const cacheSize = BlockSize * 5
 
-	testCache, err := New(cacheDir, cacheSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	testCache, err := New(cacheDir, cacheSize, WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -471,7 +470,7 @@ func TestCacheExistingFiles(t *testing.T) {
 func TestCacheBlobTooLarge(t *testing.T) {
 	cacheDir := tempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache, err := New(cacheDir, BlockSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	testCache, err := New(cacheDir, BlockSize, WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -497,7 +496,7 @@ func TestCacheBlobTooLarge(t *testing.T) {
 func TestCacheCorruptedCASBlob(t *testing.T) {
 	cacheDir := tempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache, err := New(cacheDir, BlockSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	testCache, err := New(cacheDir, BlockSize, WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -570,7 +569,7 @@ func TestMigrateFromOldDirectoryStructure(t *testing.T) {
 	// Add some overhead for likely CAS blob storage expansion.
 	const cacheSize = 2560*2 + BlockSize*2
 
-	testCache, err := New(cacheDir, cacheSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	testCache, err := New(cacheDir, cacheSize, WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -668,7 +667,7 @@ func TestLoadExistingEntries(t *testing.T) {
 	// Add some overhead for likely CAS blob storage expansion.
 	cacheSize := int64((blobSize + BlockSize) * numBlobs * 2)
 
-	testCache, err := New(cacheDir, cacheSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	testCache, err := New(cacheDir, cacheSize, WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -714,7 +713,7 @@ func TestDistinctKeyspaces(t *testing.T) {
 	// Add some overhead for likely CAS blob storage expansion.
 	cacheSize := int64((blobSize+BlockSize)*3) * 2
 
-	testCache, err := New(cacheDir, cacheSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	testCache, err := New(cacheDir, cacheSize, WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -840,7 +839,7 @@ func TestHttpProxyBackend(t *testing.T) {
 	// Add some overhead for likely CAS blob storage expansion.
 	cacheSize := int64(1024*10) * 2
 
-	testCache, err := New(cacheDir, cacheSize, math.MaxInt64, "zstd", proxy, testutils.NewSilentLogger())
+	testCache, err := New(cacheDir, cacheSize, WithProxyBackend(proxy), WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -878,7 +877,7 @@ func TestHttpProxyBackend(t *testing.T) {
 	// Create a new (empty) testCache, without a proxy backend.
 	cacheDir = testutils.TempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache, err = New(cacheDir, cacheSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	testCache, err = New(cacheDir, cacheSize, WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -943,7 +942,7 @@ func TestGetValidatedActionResult(t *testing.T) {
 	cacheDir := testutils.TempDir(t)
 	defer os.RemoveAll(cacheDir)
 
-	testCache, err := New(cacheDir, 1024*32, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	testCache, err := New(cacheDir, 1024*32, WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1118,7 +1117,7 @@ func TestGetWithOffset(t *testing.T) {
 
 	const blobSize = 2048 + 256
 
-	testCache, err := New(cacheDir, blobSize*2, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	testCache, err := New(cacheDir, blobSize*2, WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/cache/httpproxy/httpproxy_test.go b/cache/httpproxy/httpproxy_test.go
index b62a981..7fc5a7a 100644
--- a/cache/httpproxy/httpproxy_test.go
+++ b/cache/httpproxy/httpproxy_test.go
@@ -7,7 +7,6 @@ import (
 	"io"
 	"io/ioutil"
 	"log"
-	"math"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -123,7 +122,7 @@ func TestEverything(t *testing.T) {
 	}
 
 	diskCacheSize := int64(len(casData) + disk.BlockSize)
-	diskCache, err := disk.New(cacheDir, diskCacheSize, math.MaxInt64, "zstd", proxyCache, testutils.NewSilentLogger())
+	diskCache, err := disk.New(cacheDir, diskCacheSize, disk.WithProxyBackend(proxyCache), disk.WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -281,7 +280,7 @@ func TestEverything(t *testing.T) {
 	cacheDir2 := testutils.TempDir(t)
 	defer os.RemoveAll(cacheDir2)
 
-	diskCache, err = disk.New(cacheDir2, diskCacheSize, math.MaxInt64, "zstd", proxyCache, testutils.NewSilentLogger())
+	diskCache, err = disk.New(cacheDir2, diskCacheSize, disk.WithProxyBackend(proxyCache), disk.WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/server/grpc_test.go b/server/grpc_test.go
index 5f485ff..aa4cbbb 100644
--- a/server/grpc_test.go
+++ b/server/grpc_test.go
@@ -9,7 +9,6 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
-	"math"
 	"net"
 	"os"
 	"testing"
@@ -77,7 +76,7 @@ func TestMain(m *testing.M) {
 	// Add some overhead for likely CAS blob storage expansion.
 	cacheSize := int64(10 * maxChunkSize * 2)
 
-	diskCache, err = disk.New(dir, cacheSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	diskCache, err = disk.New(dir, cacheSize, disk.WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		fmt.Println("Test setup failed")
 		os.Exit(1)
diff --git a/server/http_test.go b/server/http_test.go
index cf23f58..9739e43 100644
--- a/server/http_test.go
+++ b/server/http_test.go
@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
-	"math"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -35,7 +34,7 @@ func TestDownloadFile(t *testing.T) {
 	// Add some overhead for likely CAS blob storage expansion.
 	cacheSize := blobSize*2 + disk.BlockSize
 
-	c, err := disk.New(cacheDir, cacheSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	c, err := disk.New(cacheDir, cacheSize, disk.WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -103,7 +102,7 @@ func TestUploadFilesConcurrently(t *testing.T) {
 	// Add some overhead for likely CAS blob storage expansion.
 	cacheSize := int64(NumUploads * blobSize * 2)
 
-	c, err := disk.New(cacheDir, cacheSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	c, err := disk.New(cacheDir, cacheSize, disk.WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -167,7 +166,7 @@ func TestUploadSameFileConcurrently(t *testing.T) {
 	// Add some overhead for likely CAS blob storage expansion.
 	cacheSize := int64(len(data) * numWorkers * 2)
 
-	c, err := disk.New(cacheDir, cacheSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	c, err := disk.New(cacheDir, cacheSize, disk.WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -208,7 +207,7 @@ func TestUploadCorruptedFile(t *testing.T) {
 
 	r := httptest.NewRequest("PUT", "/cas/"+hash, bytes.NewReader(corruptedData))
 
-	c, err := disk.New(cacheDir, 2048, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	c, err := disk.New(cacheDir, 2048, disk.WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -248,7 +247,7 @@ func TestUploadEmptyActionResult(t *testing.T) {
 
 	r := httptest.NewRequest("PUT", "/ac/"+hash, bytes.NewReader(data))
 
-	c, err := disk.New(cacheDir, disk.BlockSize, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	c, err := disk.New(cacheDir, disk.BlockSize, disk.WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -309,7 +308,7 @@ func testEmptyBlobAvailable(t *testing.T, method string) {
 	data, hash := testutils.RandomDataAndHash(0)
 	r := httptest.NewRequest(method, "/cas/"+hash, bytes.NewReader(data))
 
-	c, err := disk.New(cacheDir, 2048, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	c, err := disk.New(cacheDir, 2048, disk.WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -335,7 +334,7 @@ func TestStatusPage(t *testing.T) {
 
 	r := httptest.NewRequest("GET", "/status", nil)
 
-	c, err := disk.New(cacheDir, 2048, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	c, err := disk.New(cacheDir, 2048, disk.WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -478,7 +477,7 @@ func TestRemoteReturnsNotFound(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer os.RemoveAll(cacheDir)
-	emptyCache, err := disk.New(cacheDir, 1024, math.MaxInt64, "zstd", nil, testutils.NewSilentLogger())
+	emptyCache, err := disk.New(cacheDir, 1024, disk.WithAccessLogger(testutils.NewSilentLogger()))
 	if err != nil {
 		t.Fatal(err)
 	}