sstable: factor out common iterator init code

[RaduBerinde]

We've had multiple issues with divergences between initializing the data block read handles. This PR removes the code duplication.

sstable: add constructors for iterators

Add newSingleLevelIterator and newTwoLevelIterator constructors.

sstable: factor out common iterator init code

This commits factors out most of the singleLevelIterator
initialization code, so that twoLevelIterator can use it too.

[cockroach-teamcity]

This change is 

[sumeerbhola]

Reviewed 3 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jbowens and @RaduBerinde)

---

sstable/reader_iter_single_lvl.go line 204 at r1 (raw file):

		ctx, r, v, transforms, lower, upper, filterer, useFilter, stats, categoryAndQoS, statsCollector, rp, bufferPool,
	); err != nil {
		_ = i.Close()

There's a small change here in that on master we were not calling Close() if init() returned an error.
Do we have enough testing of error paths (or a careful read of the Close code) to have some certainty that calling Close() is safe, i.e., there isn't some partially initialized state that will blow up in Close (e.g. unexpected nil pointer or something).

---

edit: there are changes in the second commit that slightly alter the details of the question, but it still stands.

---

sstable/reader_iter_two_lvl.go line 166 at r1 (raw file):

	topLevelIndexH, err := r.readIndex(ctx, i.indexFilterRH, stats, &i.iterStats)
	if err != nil {
		_ = i.Close()

ditto

---

sstable/reader_iter_two_lvl.go line 188 at r1 (raw file):

	if err != nil {
		// blockIter.Close releases topLevelIndexH and always returns a nil error
		_ = i.Close()

ditto

[RaduBerinde]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jbowens and @sumeerbhola)

---

sstable/reader_iter_single_lvl.go line 204 at r1 (raw file):

Previously, sumeerbhola wrote…

There's a small change here in that on master we were not calling Close() if init() returned an error.
Do we have enough testing of error paths (or a careful read of the Close code) to have some certainty that calling Close() is safe, i.e., there isn't some partially initialized state that will blow up in Close (e.g. unexpected nil pointer or something).

---

edit: there are changes in the second commit that slightly alter the details of the question, but it still stands.

Good point, I added a test.

[sumeerbhola]

Reviewed 1 of 3 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jbowens and @RaduBerinde)

---

sstable/reader_iter_single_lvl.go line 204 at r1 (raw file):

Previously, RaduBerinde wrote…

Good point, I added a test.

Thanks

---

sstable/reader_iter_test.go line 54 at r4 (raw file):

	var stats base.InternalIteratorStats
	for k := 0; k < 20; k++ {

wondering why this iteration instead of simply creating each kind of iterator once?

[RaduBerinde]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jbowens and @sumeerbhola)

---

sstable/reader_iter_test.go line 54 at r4 (raw file):

Previously, sumeerbhola wrote…

wondering why this iteration instead of simply creating each kind of iterator once?

Just so we reuse the same iterator from the pool (in case ae leave it in a bad state in this path)