Fix discard stats moved by GC bug #929

jarifibrahim · 2019-07-13T18:21:16Z

The discard stats are stored like normal keys in badger, which means if
the size of the value of discard stats key is greater the value threshold it
will be stored in the value log. When value log GC happens, we insert
the same key in badger with a !badger!move prefix which points to the
new value log file. So when searching for the value of a key, if the value
points to a value log file which doesn't exist, we search for the same
key with !badger!move prefix.

The above logic was missing from the populateDiscardStats function. The
earlier implementation would never search for the key with !badger!move
prefix. It would return error on the first attempt to find the key. This
commit ensures we search for a key with prefix badger move if the value
for the existing key pointed to a value log file that no longer exists.

Fixes - #926

This change is

The discard stats are stored like normal keys in badger, which means if the size of value of discard stats key is greater the value threshold it will be stored in the value log. When value log GC happens, we insert the same key in badger with a !badger!move prefix which points to the new value log file. So when searching for the value of a key, if the value points to a value log file which doesn't exist, we search for the same key with !badger!move prefix. The above logic was missing from the populateDiscardStats function. The earlier implementation would never search for the key with !badger!move prefix. It would return error on first attempt to find the key. This commit ensures we search for key with prefix badger move if the value for the existing key pointed to a value log file that no longer exist.

manishrjain

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, and @Sch00lb0y)

value.go, line 1431 at r1 (raw file):

func (vlog *valueLog) populateDiscardStats() error {
	key := y.KeyWithTs(lfDiscardStatsKey, math.MaxUint64)
	newKey := make([]byte, len(key))

Don't need newKey

value.go, line 1449 at r1 (raw file):

		// Entry stored in LSM tree.
		if vs.Meta&bitValuePointer == 0 {
			val = make([]byte, len(vs.Value))

val = y.SafeCopy(vs.Value)

value.go, line 1454 at r1 (raw file):

		}
		// Read entry from value log.
		result, cb, err := vlog.Read(vp, new(y.Slice))

Just confirm that you can defer run callback without checking error.

value.go, line 1456 at r1 (raw file):

		result, cb, err := vlog.Read(vp, new(y.Slice))
		defer runCallback(cb)
		val = make([]byte, len(result))

val = y.SafeCopy(result)

value.go, line 1472 at r1 (raw file):

		// Prepend existing key with badger move and search for the key.
		n := copy(newKey, badgerMove)
		copy(newKey[n:], key)

key = append(badgerMove, key...)?

value.go, line 1479 at r1 (raw file):

	}
	if err := json.Unmarshal(val, &statsMap); err != nil {
		return errors.Wrapf(err, "failed to unmarshal discard stats")

Just log it. And continue the logic.

connorgorman · 2019-07-17T22:23:53Z

Will this PR be cherry-picked on top of 1.6 and released as v1.6.1? Currently without this change, v1.6.0 has an unrepairable issue.

jarifibrahim · 2019-07-18T05:20:09Z

Will this PR be cherry-picked on top of 1.6 and released as v1.6.1? Currently without this change, v1.6.0 has an unrepairable issue.

@campoy How should we release this fix?

jarifibrahim

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ashish-goswami, @manishrjain, and @Sch00lb0y)

value.go, line 1431 at r1 (raw file):