Skip to content

Commit ebf7320

Browse files
committed
db, manifest: clean up error cases when picking compactions
We change some of the compaction functions to not return errors and instead log errors or panic in invariant builds. We also add a missing return check for `setupInputs`.
1 parent b2207e1 commit ebf7320

File tree

3 files changed

+66
-81
lines changed

3 files changed

+66
-81
lines changed

compaction_picker.go

Lines changed: 27 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1482,6 +1482,8 @@ func (p *compactionPickerByScore) pickedCompactionFromCandidateFile(
14821482
}
14831483

14841484
if !pc.setupInputs(p.opts, env.diskAvailBytes, pc.startLevel) {
1485+
// TODO(radu): do we expect this to happen? (it does seem to happen if I add
1486+
// a log here).
14851487
return nil
14861488
}
14871489

@@ -1610,6 +1612,7 @@ func pickAutoLPositive(
16101612
}
16111613

16121614
if !pc.setupInputs(opts, env.diskAvailBytes, pc.startLevel) {
1615+
opts.Logger.Errorf("%v", base.AssertionFailedf("setupInputs failed"))
16131616
return nil
16141617
}
16151618
return pc.maybeAddLevel(opts, env.diskAvailBytes)
@@ -1740,56 +1743,49 @@ func (wa WriteAmpHeuristic) String() string {
17401743

17411744
// Helper method to pick compactions originating from L0. Uses information about
17421745
// sublevels to generate a compaction.
1743-
func pickL0(env compactionEnv, opts *Options, vers *version, baseLevel int) (pc *pickedCompaction) {
1746+
func pickL0(env compactionEnv, opts *Options, vers *version, baseLevel int) *pickedCompaction {
17441747
// It is important to pass information about Lbase files to L0Sublevels
17451748
// so it can pick a compaction that does not conflict with an Lbase => Lbase+1
17461749
// compaction. Without this, we observed reduced concurrency of L0=>Lbase
17471750
// compactions, and increasing read amplification in L0.
17481751
//
17491752
// TODO(bilal) Remove the minCompactionDepth parameter once fixing it at 1
17501753
// has been shown to not cause a performance regression.
1751-
lcf, err := vers.L0Sublevels.PickBaseCompaction(1, vers.Levels[baseLevel].Slice())
1752-
if err != nil {
1753-
opts.Logger.Errorf("error when picking base compaction: %s", err)
1754-
return
1755-
}
1754+
lcf := vers.L0Sublevels.PickBaseCompaction(opts.Logger, 1, vers.Levels[baseLevel].Slice())
17561755
if lcf != nil {
1757-
pc = newPickedCompactionFromL0(lcf, opts, vers, baseLevel, true)
1758-
pc.setupInputs(opts, env.diskAvailBytes, pc.startLevel)
1759-
if pc.startLevel.files.Empty() {
1760-
opts.Logger.Fatalf("empty compaction chosen")
1756+
pc := newPickedCompactionFromL0(lcf, opts, vers, baseLevel, true)
1757+
if pc.setupInputs(opts, env.diskAvailBytes, pc.startLevel) {
1758+
if pc.startLevel.files.Empty() {
1759+
opts.Logger.Errorf("%v", base.AssertionFailedf("empty compaction chosen"))
1760+
}
1761+
return pc.maybeAddLevel(opts, env.diskAvailBytes)
17611762
}
1762-
return pc.maybeAddLevel(opts, env.diskAvailBytes)
1763+
// TODO(radu): investigate why this happens.
1764+
// opts.Logger.Errorf("%v", base.AssertionFailedf("setupInputs failed"))
17631765
}
17641766

17651767
// Couldn't choose a base compaction. Try choosing an intra-L0
17661768
// compaction. Note that we pass in L0CompactionThreshold here as opposed to
17671769
// 1, since choosing a single sublevel intra-L0 compaction is
17681770
// counterproductive.
1769-
lcf, err = vers.L0Sublevels.PickIntraL0Compaction(env.earliestUnflushedSeqNum, minIntraL0Count)
1770-
if err != nil {
1771-
opts.Logger.Errorf("error when picking intra-L0 compaction: %s", err)
1772-
return
1773-
}
1771+
lcf = vers.L0Sublevels.PickIntraL0Compaction(env.earliestUnflushedSeqNum, minIntraL0Count)
17741772
if lcf != nil {
1775-
pc = newPickedCompactionFromL0(lcf, opts, vers, 0, false)
1776-
if !pc.setupInputs(opts, env.diskAvailBytes, pc.startLevel) {
1777-
return nil
1778-
}
1779-
if pc.startLevel.files.Empty() {
1780-
opts.Logger.Fatalf("empty compaction chosen")
1781-
}
1782-
{
1783-
iter := pc.startLevel.files.Iter()
1784-
if iter.First() == nil || iter.Next() == nil {
1785-
// A single-file intra-L0 compaction is unproductive.
1786-
return nil
1773+
pc := newPickedCompactionFromL0(lcf, opts, vers, 0, false)
1774+
if pc.setupInputs(opts, env.diskAvailBytes, pc.startLevel) {
1775+
if pc.startLevel.files.Empty() {
1776+
opts.Logger.Fatalf("empty compaction chosen")
1777+
}
1778+
// A single-file intra-L0 compaction is unproductive.
1779+
if iter := pc.startLevel.files.Iter(); iter.First() != nil && iter.Next() != nil {
1780+
pc.smallest, pc.largest = manifest.KeyRange(pc.cmp, pc.startLevel.files.Iter())
1781+
return pc
17871782
}
1783+
} else {
1784+
// TODO(radu): investigate why this happens.
1785+
// opts.Logger.Errorf("%v", base.AssertionFailedf("setupInputs failed"))
17881786
}
1789-
1790-
pc.smallest, pc.largest = manifest.KeyRange(pc.cmp, pc.startLevel.files.Iter())
17911787
}
1792-
return pc
1788+
return nil
17931789
}
17941790

17951791
func newPickedManualCompaction(

internal/manifest/l0_sublevels.go

Lines changed: 35 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1414,8 +1414,8 @@ func (is intervalSorterByDecreasingScore) Swap(i, j int) {
14141414
// files that can be selected for compaction. Returns nil if no compaction is
14151415
// possible.
14161416
func (s *L0Sublevels) PickBaseCompaction(
1417-
minCompactionDepth int, baseFiles LevelSlice,
1418-
) (*L0CompactionFiles, error) {
1417+
logger base.Logger, minCompactionDepth int, baseFiles LevelSlice,
1418+
) *L0CompactionFiles {
14191419
// For LBase compactions, we consider intervals in a greedy manner in the
14201420
// following order:
14211421
// - Intervals that are unlikely to be blocked due
@@ -1463,21 +1463,19 @@ func (s *L0Sublevels) PickBaseCompaction(
14631463
// file since they are likely nearby. Note that it is possible that
14641464
// those intervals have seed files at lower sub-levels so could be
14651465
// viable for compaction.
1466-
if f == nil {
1467-
return nil, errors.New("no seed file found in sublevel intervals")
1468-
}
14691466
consideredIntervals.markBits(f.minIntervalIndex, f.maxIntervalIndex+1)
14701467
if f.IsCompacting() {
14711468
if f.IsIntraL0Compacting {
14721469
// If we're picking a base compaction and we came across a seed
14731470
// file candidate that's being intra-L0 compacted, skip the
1474-
// interval instead of erroring out.
1471+
// interval instead of emitting an error.
14751472
continue
14761473
}
1477-
// We chose a compaction seed file that should not be compacting.
1478-
// Usually means the score is not accurately accounting for files
1479-
// already compacting, or internal state is inconsistent.
1480-
return nil, errors.Errorf("file %s chosen as seed file for compaction should not be compacting", f.FileNum)
1474+
// We chose a compaction seed file that should not be compacting; this
1475+
// indicates that the the internal state is inconsistent. Note that
1476+
// base.AssertionFailedf panics in invariant builds.
1477+
logger.Errorf("%v", base.AssertionFailedf("seed file %s should not be compacting", f.FileNum))
1478+
continue
14811479
}
14821480

14831481
c := s.baseCompactionUsingSeed(f, interval.index, minCompactionDepth)
@@ -1503,10 +1501,10 @@ func (s *L0Sublevels) PickBaseCompaction(
15031501
if baseCompacting {
15041502
continue
15051503
}
1506-
return c, nil
1504+
return c
15071505
}
15081506
}
1509-
return nil, nil
1507+
return nil
15101508
}
15111509

15121510
// Helper function for building an L0 -> Lbase compaction using a seed interval
@@ -1640,7 +1638,7 @@ func (s *L0Sublevels) extendFiles(
16401638
// selection.
16411639
func (s *L0Sublevels) PickIntraL0Compaction(
16421640
earliestUnflushedSeqNum base.SeqNum, minCompactionDepth int,
1643-
) (*L0CompactionFiles, error) {
1641+
) *L0CompactionFiles {
16441642
scoredIntervals := make([]intervalAndScore, len(s.orderedIntervals))
16451643
for i := range s.orderedIntervals {
16461644
interval := &s.orderedIntervals[i]
@@ -1661,49 +1659,44 @@ func (s *L0Sublevels) PickIntraL0Compaction(
16611659
continue
16621660
}
16631661

1664-
var f *TableMetadata
16651662
// Pick the seed file for the interval as the file in the highest
16661663
// sub-level.
1667-
stackDepthReduction := scoredInterval.score
1668-
for i := len(interval.files) - 1; i >= 0; i-- {
1669-
f = interval.files[i]
1670-
if f.IsCompacting() {
1671-
break
1672-
}
1673-
consideredIntervals.markBits(f.minIntervalIndex, f.maxIntervalIndex+1)
1674-
// Can this be the seed file? Files with newer sequence numbers than
1675-
// earliestUnflushedSeqNum cannot be in the compaction.
1676-
if f.LargestSeqNum >= earliestUnflushedSeqNum {
1664+
seedFile := func() *TableMetadata {
1665+
stackDepthReduction := scoredInterval.score
1666+
for i := len(interval.files) - 1; i >= 0; i-- {
1667+
f := interval.files[i]
1668+
if f.IsCompacting() {
1669+
// This file could be in a concurrent intra-L0 or base compaction; we
1670+
// can't use this interval.
1671+
return nil
1672+
}
1673+
consideredIntervals.markBits(f.minIntervalIndex, f.maxIntervalIndex+1)
1674+
// Can this be the seed file? Files with newer sequence numbers than
1675+
// earliestUnflushedSeqNum cannot be in the compaction.
1676+
if f.LargestSeqNum < earliestUnflushedSeqNum {
1677+
return f
1678+
}
16771679
stackDepthReduction--
1678-
if stackDepthReduction == 0 {
1679-
break
1680+
if stackDepthReduction < minCompactionDepth {
1681+
// Can't use this interval.
1682+
return nil
16801683
}
1681-
} else {
1682-
break
16831684
}
1684-
}
1685-
if stackDepthReduction < minCompactionDepth {
1686-
// Can't use this interval.
1687-
continue
1688-
}
1689-
1690-
if f == nil {
1691-
return nil, errors.New("no seed file found in sublevel intervals")
1692-
}
1693-
if f.IsCompacting() {
1694-
// This file could be in a concurrent intra-L0 or base compaction.
1685+
return nil
1686+
}()
1687+
if seedFile == nil {
16951688
// Try another interval.
16961689
continue
16971690
}
16981691

16991692
// We have a seed file. Build a compaction off of that seed.
17001693
c := s.intraL0CompactionUsingSeed(
1701-
f, interval.index, earliestUnflushedSeqNum, minCompactionDepth)
1694+
seedFile, interval.index, earliestUnflushedSeqNum, minCompactionDepth)
17021695
if c != nil {
1703-
return c, nil
1696+
return c
17041697
}
17051698
}
1706-
return nil, nil
1699+
return nil
17071700
}
17081701

17091702
func (s *L0Sublevels) intraL0CompactionUsingSeed(

internal/manifest/l0_sublevels_test.go

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -354,8 +354,8 @@ func TestL0Sublevels(t *testing.T) {
354354
var lcf *L0CompactionFiles
355355
if pickBaseCompaction {
356356
baseFiles := NewLevelSliceKeySorted(base.DefaultComparer.Compare, fileMetas[baseLevel])
357-
lcf, err = sublevels.PickBaseCompaction(minCompactionDepth, baseFiles)
358-
if err == nil && lcf != nil {
357+
lcf = sublevels.PickBaseCompaction(base.DefaultLogger, minCompactionDepth, baseFiles)
358+
if lcf != nil {
359359
// Try to extend the base compaction into a more rectangular
360360
// shape, using the smallest/largest keys of the files before
361361
// and after overlapping base files. This mimics the logic
@@ -381,10 +381,7 @@ func TestL0Sublevels(t *testing.T) {
381381
lcf)
382382
}
383383
} else {
384-
lcf, err = sublevels.PickIntraL0Compaction(earliestUnflushedSeqNum, minCompactionDepth)
385-
}
386-
if err != nil {
387-
return fmt.Sprintf("error: %s", err.Error())
384+
lcf = sublevels.PickIntraL0Compaction(earliestUnflushedSeqNum, minCompactionDepth)
388385
}
389386
if lcf == nil {
390387
return "no compaction picked"
@@ -610,8 +607,7 @@ func BenchmarkL0SublevelsInitAndPick(b *testing.B) {
610607
if sl == nil {
611608
b.Fatal("expected non-nil L0Sublevels to be generated")
612609
}
613-
c, err := sl.PickBaseCompaction(2, LevelSlice{})
614-
require.NoError(b, err)
610+
c := sl.PickBaseCompaction(base.DefaultLogger, 2, LevelSlice{})
615611
if c == nil {
616612
b.Fatal("expected non-nil compaction to be generated")
617613
}

0 commit comments

Comments
 (0)