Skip to content

Commit

Permalink
logstore: rm test-only DiskSideloadStorage field
Browse files Browse the repository at this point in the history
This commit removes the dirCreated field of DiskSideloadStorage, because
it is only used in tests, and is reduntant (directory existence check
already does the job).

Epic: none
Release note: none
  • Loading branch information
pav-kv committed Nov 15, 2023
1 parent e174990 commit 4d1f243
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 26 deletions.
17 changes: 6 additions & 11 deletions pkg/kv/kvserver/logstore/sideload_disk.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,10 @@ var _ SideloadStorage = &DiskSideloadStorage{}
//
// TODO(pavelkalinnikov): remove the interface, this type is the only impl.
type DiskSideloadStorage struct {
st *cluster.Settings
limiter *rate.Limiter
dir string
dirCreated bool
eng storage.Engine
st *cluster.Settings
limiter *rate.Limiter
dir string
eng storage.Engine
}

func sideloadedPath(baseDir string, rangeID roachpb.RangeID) string {
Expand Down Expand Up @@ -77,9 +76,7 @@ func NewDiskSideloadStorage(
}

func (ss *DiskSideloadStorage) createDir() error {
err := ss.eng.MkdirAll(ss.dir, os.ModePerm)
ss.dirCreated = ss.dirCreated || err == nil
return err
return ss.eng.MkdirAll(ss.dir, os.ModePerm)
}

// Dir implements SideloadStorage.
Expand Down Expand Up @@ -170,9 +167,7 @@ func (ss *DiskSideloadStorage) purgeFile(ctx context.Context, filename string) (

// Clear implements SideloadStorage.
func (ss *DiskSideloadStorage) Clear(_ context.Context) error {
err := ss.eng.RemoveAll(ss.dir)
ss.dirCreated = ss.dirCreated && err != nil
return err
return ss.eng.RemoveAll(ss.dir)
}

// TruncateTo implements SideloadStorage.
Expand Down
27 changes: 12 additions & 15 deletions pkg/kv/kvserver/logstore/sideload_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,20 +95,17 @@ func testSideloadingSideloadedStorage(t *testing.T, eng storage.Engine) {
ctx := context.Background()
ss := newTestingSideloadStorage(eng)

assertCreated := func(isCreated bool) {
assertExists := func(exists bool) {
t.Helper()
if is := ss.dirCreated; is != isCreated {
t.Fatalf("assertion failed: expected dirCreated=%t, got %t", isCreated, is)
}
_, err := ss.eng.Stat(ss.dir)
if !ss.dirCreated {
require.True(t, oserror.IsNotExist(err), "%v", err)
if !exists {
require.True(t, oserror.IsNotExist(err), err)
} else {
require.NoError(t, err)
}
}

assertCreated(false)
assertExists(false)

const (
lowTerm = 1
Expand All @@ -123,7 +120,7 @@ func testSideloadingSideloadedStorage(t *testing.T, eng storage.Engine) {
t.Fatal(err)
}

assertCreated(true)
assertExists(true)

if c, err := ss.Get(ctx, 1, highTerm); err != nil {
t.Fatal(err)
Expand All @@ -147,7 +144,7 @@ func testSideloadingSideloadedStorage(t *testing.T, eng storage.Engine) {
t.Fatal(err)
}

assertCreated(false)
assertExists(false)

for n, test := range []struct {
fun func() error
Expand Down Expand Up @@ -188,7 +185,7 @@ func testSideloadingSideloadedStorage(t *testing.T, eng storage.Engine) {
if err := ss.Clear(ctx); err != nil {
t.Fatalf("%d: %+v", n, err)
}
assertCreated(false)
assertExists(false)
}

// Write some payloads at various indexes. Note that this tests Put
Expand All @@ -201,7 +198,7 @@ func testSideloadingSideloadedStorage(t *testing.T, eng storage.Engine) {
}
}

assertCreated(true)
assertExists(true)

// Write some more payloads, overlapping, at the past term.
pastPayloads := append([]kvpb.RaftIndex{81}, payloads...)
Expand All @@ -220,7 +217,7 @@ func testSideloadingSideloadedStorage(t *testing.T, eng storage.Engine) {
t.Fatalf("got %q, wanted %q", c, exp)
}
}
assertCreated(true)
assertExists(true)

for n := range payloads {
freed, retained, err := ss.BytesIfTruncatedFromTo(ctx, 0, payloads[n])
Expand Down Expand Up @@ -292,7 +289,7 @@ func testSideloadingSideloadedStorage(t *testing.T, eng storage.Engine) {
i := payloads[n]
require.NoError(t, ss.Put(ctx, i, highTerm, file(i, highTerm)))
}
assertCreated(true)
assertExists(true)
freed, retained, err := ss.BytesIfTruncatedFromTo(ctx, 0, math.MaxUint64)
require.NoError(t, err)
require.Zero(t, retained)
Expand All @@ -307,7 +304,7 @@ func testSideloadingSideloadedStorage(t *testing.T, eng storage.Engine) {

require.NoError(t, ss.Clear(ctx))

assertCreated(false)
assertExists(false)

// Sanity check that we can call BytesIfTruncatedFromTo and TruncateTo
// without the directory existing.
Expand All @@ -320,7 +317,7 @@ func testSideloadingSideloadedStorage(t *testing.T, eng storage.Engine) {
require.Zero(t, freed)
require.Zero(t, retained)

assertCreated(false)
assertExists(false)

// Repopulate with a few entries at indexes=1,2,4 and term 10 to test `maybePurgeSideloaded`
// with.
Expand Down

0 comments on commit 4d1f243

Please sign in to comment.