Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use NextSync method in Datastore #3386

Merged
merged 2 commits into from
Dec 1, 2016
Merged

Use NextSync method in Datastore #3386

merged 2 commits into from
Dec 1, 2016

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Nov 15, 2016

@kevina kevina added topic/perf Performance status/in-progress In progress labels Nov 15, 2016
@kevina kevina changed the title WIP: Use NextSync method in Datastore Use NextSync method in Datastore Nov 23, 2016
@kevina kevina added the status/blocked Unable to be worked further until needs are met label Nov 23, 2016
@whyrusleeping whyrusleeping added status/ready Ready to be worked and removed status/in-progress In progress labels Nov 28, 2016
@kevina kevina removed the status/blocked Unable to be worked further until needs are met label Nov 29, 2016
@kevina kevina added the need/review Needs a review label Nov 29, 2016
@@ -186,8 +186,9 @@ func (bs *blockstore) AllKeysChan(ctx context.Context) (<-chan *cid.Cid, error)
select {
case <-ctx.Done():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we're checking the context at the send layer (and this is in a goroutine, and not expected to really block) we can do away with the select and context check entirely here.

Copy link
Contributor Author

@kevina kevina Nov 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so just use something like:

  e, ok := res.NextSync()
  if !ok { return nil, false }
  ...

Note: Edited.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for a for loop there either i think

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
@whyrusleeping whyrusleeping merged commit a542dea into master Dec 1, 2016
@whyrusleeping whyrusleeping deleted the kevina/nextsync branch December 1, 2016 18:35
@Kubuxu Kubuxu removed the status/ready Ready to be worked label Dec 1, 2016
@ghost ghost mentioned this pull request Dec 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review topic/perf Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants