fix only first two iters checked

[j0hnsmith]

Only the first two iterators are checked, this is a bug that only shows when there are more than 2 partitions.

[db7]

Thanks for your PR! That is a great catch.

However it does not solve the issue completely. Next() should exhaust the current partition before going to the next partition (and only then change the current index). I tried something like: partition 0 with 2 elements, partition 1 with 0, partition 3 with 3 elements. We should see 5 values, but it was still not working. I think this would do the job.

func (m *multiIterator) Next() bool {
	next := m.iters[m.current].Next()
	for ; !next && m.current < len(m.iters); m.current++ {
		next = m.iters[m.current].Next()
	}
	m.current %= len(m.iters)
	return next
}

The modulo just guarantees that we don't panic out-of-bounds if Next() is called again after it has returned false.

[j0hnsmith]

I did a slight refactor and forgot to remove a m.current++ so it was doing it twice (essentially skipping partitions).

The change above should fix it, I'm writing a test to confirm but it's failing because it seems that items aren't returned in insert order (for memory storage at least). Don't merge until this test is ready

[j0hnsmith]

Test added, ready for review.

Items are returned in correct order, was confused by this not doing what was intended (I assume).

goka/storage/multi_iterator_test.go

Lines 11 to 25 in 8b40064

	numStorages := 3
	numValues := 3
	storages := make([]Storage, numStorages)
	expected := map[string]string{}
	for i := 0; i < numStorages; i++ {
	storages[i] = NewMemory()
	for j := 0; j < numValues; j++ {
	key := fmt.Sprintf("storage-%d", i)
	val := fmt.Sprintf("value-%d", j)
	expected[key] = val
	storages[i].Set(key, []byte(val))
	}
	}

Only 3 keys in are in the storages, one per storage. I believe the intention was to create 3 keys per storage. On lines 22 & 23 existing keys are overwritten.

[db7]

I can’t check it now, but I guess the fix isn’t correct. It will skip the first partition even if it has items. Try your mixedValues test case, but add values only to partitions 0 and 2. I think you should first call Next() on the m.current partition. Can you double check?

…

On 6. Apr 2018, at 11:14, j0hnsmith ***@***.***> wrote: Test added, ready for review. Items are returned in correct order, was confused by this not doing what was intended (I assume). https://github.com/lovoo/goka/blob/8b40064e42411817f53812c22d2428d5b12b5ec2/storage/multi_iterator_test.go#L11-L25 Only 3 keys in are in the storages, one per storage. I believe the intention was to create 3 keys per storage. On lines 22 & 23 existing keys are overwritten. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[j0hnsmith]

I think you should first call Next() on the m.current partition.

That's exactly what it does do (m.current only gets incremented at the end of the iteration when next is false). I updated the test to have values only in storages 0 & 2, it still passes.

[db7]

Ah, I see. That looks good to me. Thanks. @frairon @SamiHiltunen can you merge this, please?

…

On 6. Apr 2018, at 17:55, j0hnsmith ***@***.***> wrote: @j0hnsmith commented on this pull request. In storage/multi_iterator.go: > @@ -14,13 +14,13 @@ func NewMultiIterator(iters []Iterator) Iterator { return &multiIterator{current: 0, iters: iters} } -func (m *multiIterator) Next() bool { - next := m.iters[m.current].Next() - if !next && len(m.iters)-1 > m.current { - m.current++ +func (m *multiIterator) Next() (next bool) { + for ; m.current < len(m.iters); m.current++ { next = m.iters[m.current].Next() I think you should first call Next() on the m.current partition. That's exactly what it does do (m.current only gets incremented when next is false). I updated the test to have values only in storages 0 & 2, it still passes. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[db7]

I'll merge this. The test cases are nondeterministic, however, and fail often. That is because the iteration over maps is nondeterministic in go. I'll create a PR to fix that afterwards.

Thanks for your contribution.