Add DB.CheckLevels() that compares sequence number across all levels for

[sumeerbhola]

consistency.

[petermattis]

This change is 

[sumeerbhola]

This is defined as a method on DB since it allows looking at the memtables too, but I am open to other ideas. Also, I am unsure how to integrate this into existing tests. I sprayed some random calls to CheckLevels() in compaction_test.go, range_del_test.go etc. but that does not seem like the reasonable thing to do. Ideally, if the DB is running in a test, this should be called before each compaction/ingest, but I don't know what is the idiomatic way of doing that.

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @ajkr, @itsbilal, and @petermattis)

[petermattis]

Just did a quick skim. I'll do a fuller review tomorrow or Friday.

a) I think the metamorphic test should run this check at the end of the test.
b) I think the db check tool should run this, probably instead of the current check which simply scans over all of the records.
c) Perhaps we can add an option or an env var to run this check in DB.Close. I'm least sure of this suggestion.

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @ajkr, @itsbilal, @petermattis, and @sumeerbhola)

---

level_checker.go, line 19 at r1 (raw file):

// This is a simplified version of mergingIter that only needs to step through points (analogous
// to only doing Next()) and does not do any seek optimizations (it minimally needs to seek the
// range del iterators to position them at or past the current point). These optimizations are

Nit: I had to read this a few times to realize that These refers to the seek optimizations. s/These/The seek/g would make this clearer.

---

level_checker.go, line 195 at r1 (raw file):

			}
			if seqNumMin < t.Start.SeqNum() {
				return fmt.Errorf("Encountered tombstone %v at level %d that has higher seqnum than at level %d",

Nit: Go recommends that error messages are lower-cased so they can be more easily composed. s/Encountered/encountered/g. There are a few other error messages in this PR that this applies to.

[sumeerbhola]

I added an Options.DebugCheck bool and use it in a couple of places. What do you think?

Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @ajkr and @itsbilal)

---

level_checker.go, line 19 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: I had to read this a few times to realize that These refers to the seek optimizations. s/These/The seek/g would make this clearer.

Done.

---

level_checker.go, line 195 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: Go recommends that error messages are lower-cased so they can be more easily composed. s/Encountered/encountered/g. There are a few other error messages in this PR that this applies to.

Done

[petermattis]

An Options.DebugCheck seems reasonable, though I might bikeshed that name. I think you forgot to add options.go to the PR as I'm not seeing that part of the name. Also, see below at a more centralized place where we could do the checking.

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @ajkr, @itsbilal, and @sumeerbhola)

---

db.go, line 809 at r2 (raw file):

	}
	if d.opts.DebugCheck {
		if err := d.CheckLevels(); err != nil {

I believe this is redundant with the call in DB.compact. Rather than checking before flushing and compaction, perhaps the check should be done after logAndApply. Similar to our other invariant checks, the assumption would be that the current state is good, and logAndApply would be verifying the new state.

[petermattis]

Would be good to verify that this checking would have caught the bug fixed by #383.

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @ajkr, @itsbilal, and @sumeerbhola)

[petermattis]

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @ajkr, @itsbilal, and @sumeerbhola)

---

level_checker.go, line 17 at r2 (raw file):

)

// This is a simplified version of mergingIter that only needs to step through points (analogous

I could use a high-level comment in this file explaining what the checking is doing and how it is doing it. As I was reading through this code, I was surprised that simpleMergingIter was processing range tombstones, while there is also a separate check for range tombstone consistency. I have a nagging suspicion that there is a simpler approach, but that might just be naiveté.

[petermattis]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @ajkr, @itsbilal, and @sumeerbhola)

---

level_checker.go, line 420 at r2 (raw file):

	// Determine the seqnum to read at after grabbing the read state (current and
	// memtables) above.
	seqNum := atomic.LoadUint64(&d.mu.versions.visibleSeqNum)

Do we need to grab the visibleSeqNum? This is necessary for normal reads to prevent seeing have committed batches in the memtable. But I don't think that is a concern for CheckLevels.

[sumeerbhola]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @ajkr, @itsbilal, @petermattis, and @sumeerbhola)

---

level_checker.go, line 17 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I could use a high-level comment in this file explaining what the checking is doing and how it is doing it. As I was reading through this code, I was surprised that simpleMergingIter was processing range tombstones, while there is also a separate check for range tombstone consistency. I have a nagging suspicion that there is a simpler approach, but that might just be naiveté.

Ack on adding a file level comment.

Regarding the separate check for range tombstone consistency there is a detailed comment below starting with "Checks that range tombstones are mutually consistent ...". I could not figure out a simplification because tombstones are not fully described by their endpoints. The merging iter code can check consistency of points (including tombstone starting points) against tombstones, and mutual consistency of points, but the structure of it isn't amenable to mutual tombstone consistency.

By fragmenting the tombstones relative to each other we can reduce them down to simple fully overlapping intervals which makes their consistency checking easier -- but the problem is that doing this fragmentation first requires properly truncated tombstones, which requires considering the atomic compaction group, which makes tombstones a level concept which does not fit in the levelter pairing of per-file iter and range del iter. One option (which I just realized) is to construct these per-level fragmented tombstones on the side and give them to simpleMergingIterLevel.rangeDelIter. That is, levelIter would not construct a rangeDelIter. Then when one encounters these fragmented range tombstones as part of heap iteration the point consistency logic will also catch inconsistency in these range tombstones. If you have a better idea please let me know, otherwise I will make this change. It will eliminate iterateAndCheckTombstones() but the rest of the fragmentation code will stay.

---

level_checker.go, line 420 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Do we need to grab the visibleSeqNum? This is necessary for normal reads to prevent seeing have committed batches in the memtable. But I don't think that is a concern for CheckLevels.

I did it only because of the mutable memtable we are grabbing below and was concerned about seeing some but not all concurrent writes.

[sumeerbhola]

Thanks for the review. I've addressed the comments and added a few integration points. PTAL

Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @ajkr, @itsbilal, and @petermattis)

---

db.go, line 809 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I believe this is redundant with the call in DB.compact. Rather than checking before flushing and compaction, perhaps the check should be done after logAndApply. Similar to our other invariant checks, the assumption would be that the current state is good, and logAndApply would be verifying the new state.

Done.

---

level_checker.go, line 17 at r2 (raw file):

Previously, sumeerbhola wrote…

Ack on adding a file level comment.

Regarding the separate check for range tombstone consistency there is a detailed comment below starting with "Checks that range tombstones are mutually consistent ...". I could not figure out a simplification because tombstones are not fully described by their endpoints. The merging iter code can check consistency of points (including tombstone starting points) against tombstones, and mutual consistency of points, but the structure of it isn't amenable to mutual tombstone consistency.

By fragmenting the tombstones relative to each other we can reduce them down to simple fully overlapping intervals which makes their consistency checking easier -- but the problem is that doing this fragmentation first requires properly truncated tombstones, which requires considering the atomic compaction group, which makes tombstones a level concept which does not fit in the levelter pairing of per-file iter and range del iter. One option (which I just realized) is to construct these per-level fragmented tombstones on the side and give them to simpleMergingIterLevel.rangeDelIter. That is, levelIter would not construct a rangeDelIter. Then when one encounters these fragmented range tombstones as part of heap iteration the point consistency logic will also catch inconsistency in these range tombstones. If you have a better idea please let me know, otherwise I will make this change. It will eliminate iterateAndCheckTombstones() but the rest of the fragmentation code will stay.

This idea does not work since the start keys of these fragmented tombstones will not also appear as points in the normal iters. Anyway, this was only eliminating iterateAndCheckTombstones() which could be simplified in other ways.

[petermattis]

Reviewable status: 0 of 10 files reviewed, 6 unresolved discussions (waiting on @ajkr, @itsbilal, and @sumeerbhola)

---

compaction.go, line 880 at r3 (raw file):

				log.Fatalf("CheckLevels failed with error: %s", err)
			}
		}

The existing invariant checks are performed after the new version has been generated and before it has been installed. This check is performed after the new version has been installed. I'm guessing you're doing this check afterward because d.CheckLevels isn't visible from versionSet. I think that could be avoided by adding a hook in versionSet that is invoked after the new version has been created but before it has been installed. Doing so would also avoid the need to remember to call d.CheckLevels after logAndApply.

---

level_checker.go, line 17 at r2 (raw file):

Previously, sumeerbhola wrote…

This idea does not work since the start keys of these fragmented tombstones will not also appear as points in the normal iters. Anyway, this was only eliminating iterateAndCheckTombstones() which could be simplified in other ways.

My not-very-well-thought-out idea is to model this checking akin to compactionIter: provide a single stream of records that merges together the point and range tombstones. I think we could use mergingIter for this purpose in just the manner that it is used by compactionIter. I would also expose a new accessor on mergingIter that indicated the "level" of the current entry. A checkerIter would take this stream of records as input and for each level it would maintain a low-water valid key. If we see a key from that level that is less than that low-water key, we've encountered an error.

An example of how this would work. Let's imagine we have the following stream of records:

a#8 @ L2, a#7 @ L1

checkerIter would have a field of lowWater []InternalKey. The first record checkIter sees is a#8 @ L2. We'd check lowWater[2] and everything looks fine, and then update lowWater[0:2]. The next record is a#7 @ L1. We'd check lowWater[1] and see that the new record violates the level invariant. Having written this, I think this is effectively what simpleMergingIter.step is doing, though it is only tracking a single previous record. I'm not sure if there is a benefit to tracking each level separately, though it might be worth commenting that this is what the existing code is doing.

Range tombstones would need separate handling. A range tombstone at Ln imposes a constraint on the sequence numbers for keys covered by that tombstone at Ln+1 and lower. For each level, could we track the active range tombstones?

I'm not sure if any of this is actually simpler (or materially different) than what you have. I'm still working through your implementation.

---

level_checker.go, line 30 at r3 (raw file):

// of consistency checking.
//
// Mutual consistency of range tombstones is non-trivial to check. One needs to handle untruncated

Can you elaborate on the need to handle untruncated range tombstones? It isn't obvious to me why that is true. I'm confused by later in the paragraph where you talk about fragmented tombstones.

---

level_checker.go, line 87 at r3 (raw file):

// Positions all the rangedel iterators at or past the current top of the heap, using SeekGE().
func (m *simpleMergingIter) positionRangeDels() {

Why is simpleMergingIter doing anything with range tombstones? Ah, you're looking for cases where the range tombstones are violating the level invariant. The paragraph above simpleMergingIter implies that it doesn't need to consider range tombstones which had me confused when reading this code.

---

level_checker.go, line 150 at r3 (raw file):

	} else {
		l.iter.Close()
		m.err = l.iter.Error()

I think this can be m.err = l.iter.Close(). All of the iter implementations should return the same error from Close as is returned by Error.

---

options.go, line 375 at r3 (raw file):

	// Set this to true only in tests, since it will cause frequent calls to
	// DB.CheckLevels() that will iterate over all the data in the DB.
	DebugCheck bool

Nit: The fields in the Option struct are sorted.

Nit 2: The comment should describe what this option is doing. That is, it will cause DB.CheckLevels to be called after a new version is installed.

[sumeerbhola]

Reviewable status: 0 of 10 files reviewed, 6 unresolved discussions (waiting on @ajkr, @itsbilal, @petermattis, and @sumeerbhola)

---

level_checker.go, line 17 at r2 (raw file):

My not-very-well-thought-out idea is to model this checking akin to compactionIter: provide a single stream of records that merges together the point and range tombstones. I think we could use mergingIter for this purpose in just the manner that it is used by compactionIter. I would also expose a new accessor on mergingIter that indicated the "level" of the current entry. A checkerIter would take this stream of records as input and for each level it would maintain a low-water valid key. If we see a key from that level that is less than that low-water key, we've encountered an error.
An example of how this would work. Let's imagine we have the following stream of records:
a#8 @ L2, a#7 @ L1

checkerIter would have a field of lowWater []InternalKey. The first record checkIter sees is a#8 @ L2. We'd check lowWater[2] and everything looks fine, and then update lowWater[0:2]. The next record is a#7 @ L1. We'd check lowWater[1] and see that the new record violates the level invariant. Having written this, I think this is effectively what simpleMergingIter.step is doing, though it is only tracking a single previous record. I'm not sure if there is a benefit to tracking each level separately, though it might be worth commenting that this is what the existing code is doing.

Given the sorted order of the keys produced by the heap merge I think we only need to check that the level numbers are non decreasing. The simpleMergingIter is also checking the consistency of points relative to range tombstones since it keeps the two kinds separate (like mergingIter and unlike compactionIter).

The only thing that needs special handling is the consistency checking of range tombstones relative to each other -- fragmenting all of them to do the check is conceptually simple but if holding all of them in memory is not viable then something more sophisticated would be necessary. Keeping track of the active range tombstones that you mention below would be the more sophisticated way -- then one could check that the sequence numbers of the active tombstones were not inconsistent with their levels. I think this active set are tombstones whose fragments are at the same start key -- advancing the iterator over this active set is analogous to advancing to the next minimum start or end key, which is fragmenting all of them relative to each other, but in a more incremental way instead of doing it up front.

Range tombstones would need separate handling. A range tombstone at Ln imposes a constraint on the sequence numbers for keys covered by that tombstone at Ln+1 and lower. For each level, could we track the active range tombstones?
I'm not sure if any of this is actually simpler (or materially different) than what you have. I'm still working through your implementation.

[petermattis]

Reviewable status: 0 of 10 files reviewed, 7 unresolved discussions (waiting on @ajkr, @itsbilal, and @sumeerbhola)

---

level_checker.go, line 17 at r2 (raw file):

The only thing that needs special handling is the consistency checking of range tombstones relative to each other -- fragmenting all of them to do the check is conceptually simple but if holding all of them in memory is not viable then something more sophisticated would be necessary.

I think it is ok for a first-pass to hold all of the range tombstones in memory. We can leave a TODO in the code about the more sophisticated approach if that ever becomes a problem.

Now that I have a better grasp on the high-level approach, let me do another pass on this PR. Unfortunately, this week is a bit hectic, so it might be a few days.

---

tool/db.go, line 270 at r3 (raw file):

	if err := db.CheckLevels(); err != nil {
		fmt.Fprintf(stdout, "%s\n", err)
	}

Doing this check on a scan operation seems expensive and a bit surprising. Rather, I think runCheck should be replaced with a call to db.CheckLevels().

[sumeerbhola]

Reviewable status: 0 of 11 files reviewed, 6 unresolved discussions (waiting on @ajkr, @itsbilal, and @petermattis)

---

compaction.go, line 880 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The existing invariant checks are performed after the new version has been generated and before it has been installed. This check is performed after the new version has been installed. I'm guessing you're doing this check afterward because d.CheckLevels isn't visible from versionSet. I think that could be avoided by adding a hook in versionSet that is invoked after the new version has been created but before it has been installed. Doing so would also avoid the need to remember to call d.CheckLevels after logAndApply.

I am assuming we want to check the whole DB including memtables. If I use the prospective Version from within the VersionSet code, in the flush case this includes data from some memtables that are still in the current readState, so we don't want that duplication. I guess I could plumb through the number of memtables being flushed that could be passed to the hook. Can you think of a better way?

---

level_checker.go, line 17 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The only thing that needs special handling is the consistency checking of range tombstones relative to each other -- fragmenting all of them to do the check is conceptually simple but if holding all of them in memory is not viable then something more sophisticated would be necessary.

I think it is ok for a first-pass to hold all of the range tombstones in memory. We can leave a TODO in the code about the more sophisticated approach if that ever becomes a problem.

Now that I have a better grasp on the high-level approach, let me do another pass on this PR. Unfortunately, this week is a bit hectic, so it might be a few days.

I've added to the longer comment near tombstoneWithLevel that this approach requires holding all range tombstones in-memory.

---

level_checker.go, line 30 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Can you elaborate on the need to handle untruncated range tombstones? It isn't obvious to me why that is true. I'm confused by later in the paragraph where you talk about fragmented tombstones.

That merely means that the algorithm can't just use a tombstone directly since it could be untruncated and needs to be constrained to the sstable bounds. I suspect the problem was in the placement of the comment so I've moved it to the end of the paragraph. The only way it impacts the code is that one needs to consider the atomic compaction unit, so one can cleanly truncate the tombstones, and then those truncated tombstones can be fragmented.

---

level_checker.go, line 87 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Why is simpleMergingIter doing anything with range tombstones? Ah, you're looking for cases where the range tombstones are violating the level invariant. The paragraph above simpleMergingIter implies that it doesn't need to consider range tombstones which had me confused when reading this code.

that comment also says "It can also easily accommodate consistency checking of points relative to range tombstones". Should I rephrase it?

---

level_checker.go, line 150 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I think this can be m.err = l.iter.Close(). All of the iter implementations should return the same error from Close as is returned by Error.

Done.

---

options.go, line 375 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: The fields in the Option struct are sorted.

Nit 2: The comment should describe what this option is doing. That is, it will cause DB.CheckLevels to be called after a new version is installed.

Done

---

tool/db.go, line 270 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Doing this check on a scan operation seems expensive and a bit surprising. Rather, I think runCheck should be replaced with a call to db.CheckLevels().

Done.

[petermattis]

Reviewable status: 0 of 11 files reviewed, 6 unresolved discussions (waiting on @ajkr, @itsbilal, @petermattis, and @sumeerbhola)

---

compaction.go, line 880 at r3 (raw file):

Previously, sumeerbhola wrote…

I am assuming we want to check the whole DB including memtables. If I use the prospective Version from within the VersionSet code, in the flush case this includes data from some memtables that are still in the current readState, so we don't want that duplication. I guess I could plumb through the number of memtables being flushed that could be passed to the hook. Can you think of a better way?

Yes, we definitely want to include the memtables. Not sure how I'd structure this. Perhaps add a check func(*version) error parameter to logAndApply. That isn't exactly clean. Note that the current check here isn't actually checking the new version, as we don't update the read-state until further down in this function. Maybe doing the check before the new version is installed is too severe, and we should instead do the check in updateReadStateLocked before the new read state is installed.

[petermattis]

Reviewable status: 0 of 11 files reviewed, 6 unresolved discussions (waiting on @ajkr, @itsbilal, @petermattis, and @sumeerbhola)

---

level_checker.go, line 87 at r3 (raw file):

Previously, sumeerbhola wrote…

that comment also says "It can also easily accommodate consistency checking of points relative to range tombstones". Should I rephrase it?

Ack. This is fine. I think I wrote that comment while I was still understanding the structure of the code.

---

testdata/level_checker, line 36 at r4 (raw file):

L
e.SET.10 g.SET.15
e.SET.10:10 g.SET.15:15

Why isn't the rangedel from [a-h)#12 invalid with respect to g#15 at a lower level?

---

testdata/level_checker, line 52 at r4 (raw file):

L
a.SET.10 c.RANGEDEL.72057594037927935
a.SET.10:10 b.SET.12:12 a.RANGEDEL.8:f

Similar comment to above. Also, the rangedel [a-f)#8 is in both levels, which seems like it should be impossible and thus should cause some check to fail.

---

testdata/level_checker, line 109 at r4 (raw file):

check
----
found InternalKey c#27,1 at level 0 that came after the same key from a lower level 1

The came after phrasing is weird here as it implies an ordering that is an implementation detail. How about:

found c#27,1 at L0 and c#28,1 at L1

Note that we usually spell level <N> as L<0>.

[sumeerbhola]

Reviewable status: 0 of 16 files reviewed, 6 unresolved discussions (waiting on @ajkr, @itsbilal, @petermattis, and @sumeerbhola)

---

compaction.go, line 880 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Yes, we definitely want to include the memtables. Not sure how I'd structure this. Perhaps add a check func(*version) error parameter to logAndApply. That isn't exactly clean. Note that the current check here isn't actually checking the new version, as we don't update the read-state until further down in this function. Maybe doing the check before the new version is installed is too severe, and we should instead do the check in updateReadStateLocked before the new read state is installed.

I overlooked the fact that logAndApply does not update the readState. I've taken your suggestion and moved the checking to updateReadStateLocked. The callers still have to construct a function but it is unlikely to be forgotten since one has to consciously call updateReadStateLocked with a nil parameter.

---

testdata/level_checker, line 36 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Why isn't the rangedel from [a-h)#12 invalid with respect to g#15 at a lower level?

Because the tombstone is constrained to f.SET.16. I've added comments to clarify.

---

testdata/level_checker, line 52 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Similar comment to above. Also, the rangedel [a-f)#8 is in both levels, which seems like it should be impossible and thus should cause some check to fail.

Added a comment -- the higher range tombstone, once properly truncated is [c, f)#8 and the lower one is [a, c)#8.

---

testdata/level_checker, line 109 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The came after phrasing is weird here as it implies an ordering that is an implementation detail. How about:

found c#27,1 at L0 and c#28,1 at L1

Note that we usually spell level <N> as L<0>.

Good point. I changed it to found InternalKey c#27,1 at level L0 that which has a lower sequence number than the one at level L1. I could copy the whole internal key, to print the actual previous internal key, though Clone() involves a memory allocation (which we may not care about here since CheckLevels is anyway slow). Let me know if you prefer that.

[petermattis]

modulo a few remaining nits.

Reviewable status: 0 of 16 files reviewed, 6 unresolved discussions (waiting on @ajkr, @itsbilal, @petermattis, and @sumeerbhola)

---

level_checker.go, line 1 at r5 (raw file):

// Copyright 2012 The LevelDB-Go and Pebble Authors. All rights reserved. Use

Nit: s/2012/2019/g

---

read_state.go, line 90 at r5 (raw file):

	d.readState.Unlock()
	if checker != nil {
		if err := checker(); err != nil {

Would it be worthwhile to call the checker with the new readState before it is installed. This would prevent a concurrent reader from seeing the new read state if it turns out to contain a problem. Of course, if we crash and restart they'll see the new read state anyways.

---

testdata/level_checker, line 36 at r4 (raw file):

Previously, sumeerbhola wrote…

Because the tombstone is constrained to f.SET.16. I've added comments to clarify.

Ah, I got confused about the format of the test data. The comments are helpeful.

---

testdata/level_checker, line 109 at r4 (raw file):

Previously, sumeerbhola wrote…

Good point. I changed it to found InternalKey c#27,1 at level L0 that which has a lower sequence number than the one at level L1. I could copy the whole internal key, to print the actual previous internal key, though Clone() involves a memory allocation (which we may not care about here since CheckLevels is anyway slow). Let me know if you prefer that.

Nit: There is a bit of stuttering in the phrase level L0 as the L is short for level. This reads to me as level level 0.

The message otherwise looks fine. Rather than copying the whole internal key, you could save the sequence number and kind as you're already copying the user-key portion (I think).

---

tool/db.go, line 203 at r5 (raw file):

		fmt.Fprintf(stdout, "%s\n", err)
	}
	fmt.Fprintf(stdout, "checked %d points and %d tombstones\n", stats.NumPoints, stats.NumTombstones)

Nit: we should add a convenience routine to pluralize these messages. See the use of plural in runScan.

[sumeerbhola]

Reviewable status: 0 of 16 files reviewed, 4 unresolved discussions (waiting on @ajkr, @itsbilal, and @petermattis)

---

level_checker.go, line 1 at r5 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: s/2012/2019/g

Done

---

read_state.go, line 90 at r5 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Would it be worthwhile to call the checker with the new readState before it is installed. This would prevent a concurrent reader from seeing the new read state if it turns out to contain a problem. Of course, if we crash and restart they'll see the new read state anyways.

Can we leave this for a potential future improvement? CheckLevels() cannot work with only a readState -- it needs other things from DB, so doing this means making checker a func (*readState) which is exposing more of the implementation needs of CheckLevels() and slightly uglier.

---

testdata/level_checker, line 36 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ah, I got confused about the format of the test data. The comments are helpeful.

Ack

---

testdata/level_checker, line 109 at r4 (raw file):

Nit: There is a bit of stuttering in the phrase level L0 as the L is short for level. This reads to me as level level 0.

I realized this was incorrect -- these are not the LSM levels. I've changed it to say "level index i".

The message otherwise looks fine. Rather than copying the whole internal key, you could save the sequence number and kind as you're already copying the user-key portion (I think).

Done

---

tool/db.go, line 203 at r5 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: we should add a convenience routine to pluralize these messages. See the use of plural in runScan.

Done