Skip to content

Conversation

kuba--
Copy link
Contributor

@kuba-- kuba-- commented May 4, 2018

This is pretty radical change, so please review carefully. It's related to #256

What has been changed to avoid randomised order by Repository iterator:
Current implementation relied on multiple channels which were populated in constructor NewRowRepoIter.
First we iterated over repos and push them to repos channel, at the same time N go routines pulled repos from a channel, convert them to rows and pushed rows to the next channel.
At the end we had a channel with a mix of rows from different repos (multiplexed by go routines).

This PR drastically removes lot of complexity and make everything linear in the Next function.
We just iterate over rows till we get an EOF error, then we move to the next repo.
I left only one done channel for internal usage (mainly for tests' graceful shutdown) and context.
Also what's worth to notice are atomic operations in RepositoryIter.Next function to make the API thread-safe

@kuba-- kuba-- added wip work in progress feature enhancement New feature or request labels May 5, 2018
kuba-- added 2 commits May 5, 2018 23:43
Signed-off-by: kuba-- <kuba@sourced.tech>
Signed-off-by: kuba-- <kuba@sourced.tech>
@kuba-- kuba-- removed the wip work in progress label May 6, 2018
@kuba-- kuba-- requested review from jfontan, ajnavarro and a team May 6, 2018 21:25
Signed-off-by: kuba-- <kuba@sourced.tech>
Signed-off-by: kuba-- <kuba@sourced.tech>
@ajnavarro
Copy link
Contributor

LGTM after @jfontan have a look on it.

@kuba--
Copy link
Contributor Author

kuba-- commented May 7, 2018

@jfontan - I think, I can also remove internal done channel. Actually we don't use it (only internally), so I believe ctx.Done() + io.EOF error are good enough.
Just to give you more insight. So far, in one of our test we have:

	go func() {
		for {
			_, err := repoIter.Next()
			if err != nil {
				break
			}
		}
	}()

	select {
	case <-repoIter.done:
	case <-repoIter.ctx.Done():
	}
	cancel()

If I remove done then this code will have to change but just a little bit (and will be even simpler in my opinion):

	go func() {
		for {
			_, err := repoIter.Next()
			if err != nil {
                                cancel()
				break
			}
		}
	}()

<-repoIter.ctx.Done()

WDYT?

@jfontan
Copy link
Contributor

jfontan commented May 7, 2018

@kuba-- As you say we don't need done channel anymore. The proposed change to the test looks good. 👍

Signed-off-by: kuba-- <kuba@sourced.tech>
@ajnavarro ajnavarro merged commit 743fdcc into src-d:master May 7, 2018
@kuba-- kuba-- deleted the feature-256/deterministic-iterator branch June 8, 2018 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants