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.

Sign up for GitHub

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

Jump to bottom

routing: Implement GetValuesAsync #3720

Closed
wants to merge 2 commits into from
Closed

routing: Implement GetValuesAsync #3720

wants to merge 2 commits into from

Conversation

mildred
Copy link
Contributor

@mildred mildred commented Feb 22, 2017

routing: Implement GetValuesAsync
b297a2d 
License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-git@mildred.fr>
whyrusleeping
whyrusleeping reviewed Feb 24, 2017
View reviewed changes
routing/mock/centralized_client.go
defer close(res)
data, err := c.GetValue(ctx, key)
if err != nil {
log.Debugf("GetValuesAsync error: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

this really makes me think that the RecvdVal should contain an error field.

whyrusleeping
whyrusleeping approved these changes Feb 24, 2017
View reviewed changes
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.

This LGTM. However we might consider how putting errors in the recvdVal struct would affect the flow of things (I really don't like ignoring errors like this encourages)

@whyrusleeping
Copy link
Member

@Kubuxu do you think this looks good? Or should we push to make GetValues return a channel of things-that-may-contain-errors?

@whyrusleeping whyrusleeping added this to the Ipfs 0.4.7 milestone Mar 2, 2017
@Kubuxu Kubuxu self-requested a review March 2, 2017 02:00
@mildred
Copy link
Contributor Author

mildred commented Mar 3, 2017

I took this approach with errors because another function did. But at first, my first idea was to return a second channel with possible errors. It's very easy to do and I don't mind updating my PR to match that or similar.

@whyrusleeping
Copy link
Member

@mildred could you return just a single channel, but have the type of the channel contain either a response or an error (like an option type)? Two channels cat get pretty tricky to deal with at times

whyrusleeping
whyrusleeping requested changes Mar 4, 2017
View reviewed changes
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.

(see other comment)

@mildred
Copy link
Contributor Author

mildred commented Mar 7, 2017

I started the change to make a response with an error built in, but then couldn't find this discussion any more. What's necessary to do that is to change go-libp2p-routing to add an error field to RecvdVal. I now see that's exactly what you suggested, so I'll change all three PR to account for that.

@mildred mildred force-pushed the get-values-async branch from 18e8c41 to c561672 Compare March 7, 2017 20:04
@mildred
Copy link
Contributor Author

mildred commented Mar 7, 2017 •
edited
Loading

Updated the PR, see also dependent changes in libp2p/go-libp2p-kad-dht#49 and libp2p/go-libp2p-routing#7

routing: return errors of GetValuesAsync
f661847 
License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub@mildred.fr>
@mildred mildred force-pushed the get-values-async branch from c561672 to f661847 Compare March 7, 2017 20:08
@whyrusleeping
Copy link
Member

I need to get to reviewing and merging the dependent PRs. Bumping this task to 0.4.8

@whyrusleeping whyrusleeping modified the milestones: Ipfs 0.4.8, Ipfs 0.4.7 Mar 10, 2017
@whyrusleeping whyrusleeping modified the milestones: Ipfs 0.4.9, Ipfs 0.4.8 Mar 24, 2017
@whyrusleeping whyrusleeping modified the milestones: Ipfs 0.4.10, Ipfs 0.4.9 May 8, 2017
@magik6k magik6k modified the milestones: Ipfs 0.4.10, Ipfs 0.4.11 Jul 28, 2017
Kubuxu
Kubuxu reviewed Oct 16, 2017
View reviewed changes
Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

@whyrusleeping what do we want to do about this PR and connected PRs?

(cleaning out my Review queue).

@whyrusleeping
Copy link
Member

@Kubuxu I definitely want this as part of the API. I think its really important for users of the DHT

@whyrusleeping whyrusleeping modified the milestones: Ipfs 0.4.12, go-ipfs 0.4.13 Nov 14, 2017
@whyrusleeping whyrusleeping removed this from the go-ipfs 0.4.14 milestone Feb 10, 2018
@bigs bigs closed this Sep 11, 2018
@Stebalien
Copy link
Member

Replaced by #5232.

1 similar comment
@Stebalien
Copy link
Member

Replaced by #5232.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kubuxu Kubuxu Kubuxu left review comments

@whyrusleeping whyrusleeping whyrusleeping requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mildred @whyrusleeping @Stebalien @Kubuxu @bigs @magik6k