Reviewable status: 0 of 10 files reviewed, 4 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)

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

	opt.maxBatchCount = opt.maxBatchSize / int64(skl.MaxNodeSize)

	if opt.ValueThreshold > math.MaxUint32-100 { // TODO: correct this.

Disallow greater than opt.MaxTableSize or maybe opt.maxBatchSize.

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

	if opt.ValueThreshold > math.MaxUint32-100 { // TODO: correct this.
		return nil, ErrValueThreshold

The error string can be generated here.

options.go, line 168 at r2 (raw file):

	// TODO: exact value of ValueThreshold ??
	LSMOnlyOptions.ValueThreshold = 4294967195 // Max value length which fits in uint32.

ValueThreshold can be 1MB.

Reviewable status: 0 of 11 files reviewed, 4 unresolved discussions (waiting on @golangcibot, @jarifibrahim, and @manishrjain)

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

Done.

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

Done.

options.go, line 168 at r2 (raw file):

Done.

table/builder.go, line 135 at r1 (raw file):

Done.

except for a couple minor comments. Defer to @manishrjain for the final approval.

Reviewed 3 of 8 files at r1, 2 of 2 files at r2, 6 of 6 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @jarifibrahim, and @manishrjain)

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

I think Manish meant calling errors.Errorf here like right below.

errors.go, line 29 at r3 (raw file):

	// ErrValueThreshold is returned when ValueThreshold is set
	// to a value greater than ValueThresholdLimit.

Can this comment be in a single line? If not disregard.

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

errors.go, line 31 at r3 (raw file):

	// to a value greater than ValueThresholdLimit.
	ErrValueThreshold = errors.Errorf(
		"Invalid ValueThreshold, must be less than or equal to %d", ValueThresholdLimit)

Let's remove this error here. And inline it in the place of usage.

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

const (
	// ValueThresholdLimit is the maximum permissible value of opt.ValueThreshold.
	ValueThresholdLimit = (1 << 20) // 1 MB.

Does it have to be public? Can't this be a private var?

Also, is it being used in more than 1 place in the main code? (ignoring test files)

Got comments. Defer to @jarifibrahim to ensure my comments are addressed, and for final approval.

Reviewed 3 of 8 files at r1, 2 of 2 files at r2, 2 of 6 files at r3, 4 of 4 files at r4.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @jarifibrahim, and @manishrjain)

db.go, line 196 at r4 (raw file):

	// We are limiting opt.ValueThreshold to LSMOnlyOptions.ValueThreshold for now.
	if opt.ValueThreshold > LSMOnlyOptions.ValueThreshold {

This doesn't make sense. LSMOnlyOptions is already an opt, which could have been passed to this Open call.

db.go, line 204 at r4 (raw file):

	// the transaction APIs. Transaction batches entries into batches of size opt.maxBatchSize.
	if int64(opt.ValueThreshold) > opt.maxBatchSize {
		return nil, errors.Errorf("Valuethreshold size too big. Either reduce opt.ValueThreshold " +

ValueThreshold greater than max batch size of %d. Either reduce ...

options.go, line 167 at r4 (raw file):

	LSMOnlyOptions = DefaultOptions

	LSMOnlyOptions.ValueThreshold = (1 << 20) // 1 MB for now, can be changed in future.

Remove the "for now, can be changed in future."

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

db_test.go, line 1475 at r4 (raw file):

	dopts.ValueThreshold = 1 << 21
	_, err = Open(dopts)

Close the opened DB. We don't want unnecessary go routines running the background.

db_test.go, line 1482 at r4 (raw file):

	// maxBatchSize is calculated from MaxTableSize.
	dopts.MaxTableSize = int64(LSMOnlyOptions.ValueThreshold)
	_, err = Open(dopts)

Close the opened DB.

db_test.go, line 1483 at r4 (raw file):

	dopts.MaxTableSize = int64(LSMOnlyOptions.ValueThreshold)
	_, err = Open(dopts)
	require.Error(t, err, "db creation should have been failed")

"db creation should have been failed" => "DB creation failed"

skl/skl_test.go, line 126 at r4 (raw file):

	l.Put(y.KeyWithTs([]byte("key4"), 1), y.ValueStruct{Value: val5, Meta: 60, UserMeta: 0})
	v = l.Get(y.KeyWithTs([]byte("key4"), 1))
	require.True(t, v.Value != nil)

You can use require.NotNil(..) https://godoc.org/github.com/stretchr/testify/require#NotNil

skl/skl_test.go, line 188 at r4 (raw file):

			defer wg.Done()
			v := l.Get(key(i))
			require.True(t, v.Value != nil)

You can use require.NotNil(..) https://godoc.org/github.com/stretchr/testify/require#NotNil

Reviewable status: 7 of 11 files reviewed, 12 unresolved discussions (waiting on @golangcibot, @jarifibrahim, @manishrjain, and @martinmr)

db.go, line 196 at r4 (raw file):

Fixed it now.

db.go, line 204 at r4 (raw file):

Done.

db_test.go, line 1475 at r4 (raw file):

This test is for for testing error . If there is no error, it should fail.

db_test.go, line 1482 at r4 (raw file):

Again same reason.

db_test.go, line 1483 at r4 (raw file):

Here we are checking for non nil error. If error is nil, then require should fail test.

errors.go, line 29 at r3 (raw file):

Moved the error to be inline.

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

I have made maxValueThreshold as private. It is getting used in LSMOnlyOptions and error message when opt.ValueThreshold is greater than maxValueThreshold.

options.go, line 167 at r4 (raw file):

Done.

skl/skl_test.go, line 126 at r4 (raw file):

Done.

skl/skl_test.go, line 188 at r4 (raw file):

Done.

Reviewed 4 of 4 files at r5.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @golangcibot, @manishrjain, and @martinmr)