From 4786ab3cbbd434ab89688350cdeabbc9604d95ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julio=20L=C3=B3pez?= <1953782+julio-lopez@users.noreply.github.com> Date: Tue, 18 Jun 2024 12:24:56 -0700 Subject: [PATCH] refactor(general): remove no-longer used functions and tests (#3924) Followup cleanup for #3919 All the changes are in the `epoch` package Summary: - Removal of now unused code and tests. - Refactoring to simplify test and remove a linter annotation. - Fix a typo --- internal/epoch/epoch_utils.go | 38 --- internal/epoch/epoch_utils_test.go | 392 +++-------------------------- 2 files changed, 34 insertions(+), 396 deletions(-) diff --git a/internal/epoch/epoch_utils.go b/internal/epoch/epoch_utils.go index 42a569586..07594025d 100644 --- a/internal/epoch/epoch_utils.go +++ b/internal/epoch/epoch_utils.go @@ -115,8 +115,6 @@ func (r closedIntRange) isEmpty() bool { return r.length() == 0 } -var errNonContiguousRange = errors.New("non-contiguous range") - // constants from the standard math package. const ( //nolint:mnd @@ -126,42 +124,6 @@ func (r closedIntRange) isEmpty() bool { minInt = -1 << (intSize - 1) ) -// Returns a range for the keys in m. It returns an empty range when m is empty. -func getKeyRange[E any](m map[int]E) closedIntRange { - if len(m) == 0 { - return closedIntRange{lo: 0, hi: -1} - } - - lo, hi := maxInt, minInt - for k := range m { - if k < lo { - lo = k - } - - if k > hi { - hi = k - } - } - - return closedIntRange{lo: lo, hi: hi} -} - -// Returns a contiguous range for the keys in m. -// When the range is not continuous an error is returned. -func getContiguousKeyRange[E any](m map[int]E) (closedIntRange, error) { - r := getKeyRange(m) - - // r.hi and r.lo are from unique map keys, so for the range to be continuous - // then the range length must be exactly the same as the size of the map. - // For example, if lo==2, hi==4, and len(m) == 3, the range must be - // contiguous => {2, 3, 4} - if r.length() != uint(len(m)) { - return closedIntRange{-1, -2}, errors.Wrapf(errNonContiguousRange, "[lo: %d, hi: %d], length: %d", r.lo, r.hi, len(m)) - } - - return r, nil -} - func getFirstContiguousKeyRange[E any](m map[int]E) closedIntRange { if len(m) == 0 { return closedIntRange{lo: 0, hi: -1} diff --git a/internal/epoch/epoch_utils_test.go b/internal/epoch/epoch_utils_test.go index fd7890a6e..83bcdf240 100644 --- a/internal/epoch/epoch_utils_test.go +++ b/internal/epoch/epoch_utils_test.go @@ -78,243 +78,11 @@ func TestGroupByEpochNumber(t *testing.T) { } } -func TestGetContiguosKeyRange(t *testing.T) { - invalidEmptyRange := closedIntRange{-1, -2} - - cases := []struct { - input map[int]bool - want closedIntRange - shouldErr bool - length uint - isEmpty bool - }{ - { - isEmpty: true, - want: closedIntRange{0, -1}, - }, - { - input: map[int]bool{0: true}, - want: closedIntRange{lo: 0, hi: 0}, - length: 1, - }, - { - input: map[int]bool{-5: true}, - want: closedIntRange{lo: -5, hi: -5}, - length: 1, - }, - { - input: map[int]bool{-5: true, -4: true}, - want: closedIntRange{lo: -5, hi: -4}, - length: 2, - }, - { - input: map[int]bool{0: true}, - want: closedIntRange{lo: 0, hi: 0}, - length: 1, - }, - { - input: map[int]bool{5: true}, - want: closedIntRange{lo: 5, hi: 5}, - length: 1, - }, - { - input: map[int]bool{0: true, 1: true}, - want: closedIntRange{lo: 0, hi: 1}, - length: 2, - }, - { - input: map[int]bool{8: true, 9: true}, - want: closedIntRange{lo: 8, hi: 9}, - length: 2, - }, - { - input: map[int]bool{1: true, 2: true, 3: true, 4: true, 5: true}, - want: closedIntRange{lo: 1, hi: 5}, - length: 5, - }, - { - input: map[int]bool{8: true, 10: true}, - want: invalidEmptyRange, - shouldErr: true, - isEmpty: true, - }, - { - input: map[int]bool{1: true, 2: true, 3: true, 5: true}, - want: invalidEmptyRange, - shouldErr: true, - isEmpty: true, - }, - { - input: map[int]bool{-5: true, -7: true}, - want: invalidEmptyRange, - shouldErr: true, - isEmpty: true, - }, - { - input: map[int]bool{0: true, minInt: true}, - want: invalidEmptyRange, - shouldErr: true, - isEmpty: true, - }, - { - input: map[int]bool{0: true, maxInt: true}, - want: invalidEmptyRange, - shouldErr: true, - isEmpty: true, - }, - { - input: map[int]bool{maxInt: true, minInt: true}, - want: invalidEmptyRange, - shouldErr: true, - isEmpty: true, - }, - { - input: map[int]bool{minInt: true}, - want: closedIntRange{lo: minInt, hi: minInt}, - length: 1, - }, - { - input: map[int]bool{maxInt - 1: true}, - want: closedIntRange{lo: maxInt - 1, hi: maxInt - 1}, - length: 1, - }, - { - input: map[int]bool{maxInt: true}, - want: closedIntRange{lo: maxInt, hi: maxInt}, - length: 1, - }, - } - - for i, tc := range cases { - t.Run(fmt.Sprint("case: ", i), func(t *testing.T) { - got, err := getContiguousKeyRange(tc.input) - if tc.shouldErr { - require.Error(t, err, "input: %v", tc.input) - } - - require.Equal(t, tc.want, got, "input: %#v", tc.input) - require.Equal(t, tc.length, got.length()) - require.Equal(t, tc.isEmpty, got.isEmpty()) - }) - } -} - func TestAssertMinMaxIntConstants(t *testing.T) { require.Equal(t, math.MinInt, minInt) require.Equal(t, math.MaxInt, maxInt) } -func TestGetKeyRange(t *testing.T) { - cases := []struct { - input map[int]bool - want closedIntRange - length uint - isEmpty bool - }{ - { - isEmpty: true, - want: closedIntRange{lo: 0, hi: -1}, - }, - { - input: map[int]bool{0: true}, - want: closedIntRange{lo: 0, hi: 0}, - length: 1, - }, - { - input: map[int]bool{-5: true}, - want: closedIntRange{lo: -5, hi: -5}, - length: 1, - }, - { - input: map[int]bool{-5: true, -4: true}, - want: closedIntRange{lo: -5, hi: -4}, - length: 2, - }, - { - input: map[int]bool{0: true}, - want: closedIntRange{lo: 0, hi: 0}, - length: 1, - }, - { - input: map[int]bool{5: true}, - want: closedIntRange{lo: 5, hi: 5}, - length: 1, - }, - { - input: map[int]bool{0: true, 1: true}, - want: closedIntRange{lo: 0, hi: 1}, - length: 2, - }, - { - input: map[int]bool{8: true, 9: true}, - want: closedIntRange{lo: 8, hi: 9}, - length: 2, - }, - { - input: map[int]bool{1: true, 2: true, 3: true, 4: true, 5: true}, - want: closedIntRange{lo: 1, hi: 5}, - length: 5, - }, - { - input: map[int]bool{8: true, 10: true}, - want: closedIntRange{lo: 8, hi: 10}, - length: 3, - }, - { - input: map[int]bool{1: true, 2: true, 3: true, 5: true}, - want: closedIntRange{lo: 1, hi: 5}, - length: 5, - }, - { - input: map[int]bool{-5: true, -7: true}, - want: closedIntRange{lo: -7, hi: -5}, - length: 3, - }, - { - input: map[int]bool{0: true, minInt: true}, - want: closedIntRange{lo: minInt, hi: 0}, - length: -minInt + 1, - }, - { - input: map[int]bool{0: true, maxInt: true}, - want: closedIntRange{lo: 0, hi: maxInt}, - length: maxInt + 1, - }, - { - input: map[int]bool{maxInt: true, minInt: true}, - want: closedIntRange{lo: minInt, hi: maxInt}, - length: 0, // corner case, not representable :( - isEmpty: true, - }, - { - input: map[int]bool{minInt: true}, - want: closedIntRange{lo: minInt, hi: minInt}, - length: 1, - }, - { - input: map[int]bool{maxInt - 1: true}, - want: closedIntRange{lo: maxInt - 1, hi: maxInt - 1}, - length: 1, - }, - { - input: map[int]bool{maxInt: true}, - want: closedIntRange{lo: maxInt, hi: maxInt}, - length: 1, - }, - } - - for i, tc := range cases { - t.Run(fmt.Sprint("case: ", i), func(t *testing.T) { - got := getKeyRange(tc.input) - - require.Equal(t, tc.want, got, "input: %#v", tc.input) - require.Equal(t, tc.length, got.length()) - require.Equal(t, tc.isEmpty, got.isEmpty()) - }) - } -} - -//nolint:maintidx func TestOldestUncompactedEpoch(t *testing.T) { cases := []struct { input CurrentSnapshot @@ -408,136 +176,64 @@ func TestOldestUncompactedEpoch(t *testing.T) { { input: CurrentSnapshot{ - LongestRangeCheckpointSets: []*RangeMetadata{ - { - MinEpoch: 0, - MaxEpoch: 2, - Blobs: []blob.Metadata{ - {BlobID: rangeCheckpointBlobPrefix(0, 2) + "foo-0-2"}, - }, - }, - }, + LongestRangeCheckpointSets: makeLongestRange(0, 2), }, expectedEpoch: 3, }, { input: CurrentSnapshot{ - LongestRangeCheckpointSets: []*RangeMetadata{ - { - MinEpoch: 0, - MaxEpoch: 2, - Blobs: []blob.Metadata{ - {BlobID: rangeCheckpointBlobPrefix(0, 2) + "foo-0-2"}, - }, - }, - }, - SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{0, 1}), + LongestRangeCheckpointSets: makeLongestRange(0, 2), + SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{0, 1}), }, expectedEpoch: 3, }, { input: CurrentSnapshot{ - LongestRangeCheckpointSets: []*RangeMetadata{ - { - MinEpoch: 0, - MaxEpoch: 2, - Blobs: []blob.Metadata{ - {BlobID: rangeCheckpointBlobPrefix(0, 2) + "foo-0-2"}, - }, - }, - }, - SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{1, 2}), + LongestRangeCheckpointSets: makeLongestRange(0, 2), + SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{1, 2}), }, expectedEpoch: 3, }, { input: CurrentSnapshot{ - LongestRangeCheckpointSets: []*RangeMetadata{ - { - MinEpoch: 0, - MaxEpoch: 2, - Blobs: []blob.Metadata{ - {BlobID: rangeCheckpointBlobPrefix(0, 2) + "foo-0-2"}, - }, - }, - }, - SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{1}), + LongestRangeCheckpointSets: makeLongestRange(0, 2), + SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{1}), }, expectedEpoch: 3, }, { input: CurrentSnapshot{ - LongestRangeCheckpointSets: []*RangeMetadata{ - { - MinEpoch: 0, - MaxEpoch: 2, - Blobs: []blob.Metadata{ - {BlobID: rangeCheckpointBlobPrefix(0, 2) + "foo-0-2"}, - }, - }, - }, - SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{4, 5}), + LongestRangeCheckpointSets: makeLongestRange(0, 2), + SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{4, 5}), }, expectedEpoch: 3, }, { input: CurrentSnapshot{ - LongestRangeCheckpointSets: []*RangeMetadata{ - { - MinEpoch: 0, - MaxEpoch: 2, - Blobs: []blob.Metadata{ - {BlobID: rangeCheckpointBlobPrefix(0, 2) + "foo-0-2"}, - }, - }, - }, - SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{2, 3}), + LongestRangeCheckpointSets: makeLongestRange(0, 2), + SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{2, 3}), }, expectedEpoch: 4, }, { input: CurrentSnapshot{ - LongestRangeCheckpointSets: []*RangeMetadata{ - { - MinEpoch: 0, - MaxEpoch: 2, - Blobs: []blob.Metadata{ - {BlobID: rangeCheckpointBlobPrefix(0, 2) + "foo-0-2"}, - }, - }, - }, - SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{3, 4}), + LongestRangeCheckpointSets: makeLongestRange(0, 2), + SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{3, 4}), }, expectedEpoch: 5, }, { input: CurrentSnapshot{ - LongestRangeCheckpointSets: []*RangeMetadata{ - { - MinEpoch: 1, - MaxEpoch: 2, - Blobs: []blob.Metadata{ - {BlobID: rangeCheckpointBlobPrefix(1, 2) + "foo-1-2"}, - }, - }, - }, - SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{3, 4}), + LongestRangeCheckpointSets: makeLongestRange(1, 2), + SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{3, 4}), }, expectedEpoch: -1, wantErr: errInvalidCompactedRange, }, { input: CurrentSnapshot{ - LongestRangeCheckpointSets: []*RangeMetadata{ - { - MinEpoch: 0, - MaxEpoch: 2, - Blobs: []blob.Metadata{ - {BlobID: rangeCheckpointBlobPrefix(0, 2) + "foo-0-2"}, - }, - }, - }, + LongestRangeCheckpointSets: makeLongestRange(0, 2), // non-contiguous single epoch compaction set SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{3, 5}), }, @@ -545,15 +241,7 @@ func TestOldestUncompactedEpoch(t *testing.T) { }, { input: CurrentSnapshot{ - LongestRangeCheckpointSets: []*RangeMetadata{ - { - MinEpoch: 0, - MaxEpoch: 7, - Blobs: []blob.Metadata{ - {BlobID: rangeCheckpointBlobPrefix(0, 7) + "foo-0-7"}, - }, - }, - }, + LongestRangeCheckpointSets: makeLongestRange(0, 7), // non-contiguous single epoch compaction set, but most of the set overlaps with the compacted range SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{0, 1, 2, 4, 6, 9}), }, @@ -561,30 +249,14 @@ func TestOldestUncompactedEpoch(t *testing.T) { }, { input: CurrentSnapshot{ - LongestRangeCheckpointSets: []*RangeMetadata{ - { - MinEpoch: 0, - MaxEpoch: 7, - Blobs: []blob.Metadata{ - {BlobID: rangeCheckpointBlobPrefix(0, 7) + "foo-0-7"}, - }, - }, - }, - SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{9, 10}), + LongestRangeCheckpointSets: makeLongestRange(0, 7), + SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{9, 10}), }, expectedEpoch: 8, }, { input: CurrentSnapshot{ - LongestRangeCheckpointSets: []*RangeMetadata{ - { - MinEpoch: 0, - MaxEpoch: 7, - Blobs: []blob.Metadata{ - {BlobID: rangeCheckpointBlobPrefix(0, 7) + "foo-0-7"}, - }, - }, - }, + LongestRangeCheckpointSets: makeLongestRange(0, 7), // non-contiguous single epoch compaction set SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{8, 10}), }, @@ -592,15 +264,7 @@ func TestOldestUncompactedEpoch(t *testing.T) { }, { input: CurrentSnapshot{ - LongestRangeCheckpointSets: []*RangeMetadata{ - { - MinEpoch: 0, - MaxEpoch: 7, - Blobs: []blob.Metadata{ - {BlobID: rangeCheckpointBlobPrefix(0, 7) + "foo-0-7"}, - }, - }, - }, + LongestRangeCheckpointSets: makeLongestRange(0, 7), // non-contiguous single epoch compaction set SingleEpochCompactionSets: makeSingleCompactionEpochSets([]int{8, 9, 12}), }, @@ -632,7 +296,19 @@ func makeSingleCompactionEpochSets(epochs []int) map[int][]blob.Metadata { return es } -func TestGetFirstContiguosKeyRange(t *testing.T) { +func makeLongestRange(minEpoch, maxEpoch int) []*RangeMetadata { + return []*RangeMetadata{ + { + MinEpoch: minEpoch, + MaxEpoch: maxEpoch, + Blobs: []blob.Metadata{ + {BlobID: blob.ID(fmt.Sprintf("%sfoo-%v-%v", rangeCheckpointBlobPrefix(minEpoch, maxEpoch), minEpoch, maxEpoch))}, + }, + }, + } +} + +func TestGetFirstContiguousKeyRange(t *testing.T) { cases := []struct { input map[int]bool want closedIntRange