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

Dual DHT scaffold #570

Merged
merged 13 commits into from
Apr 9, 2020
Merged

Dual DHT scaffold #570

merged 13 commits into from
Apr 9, 2020

Conversation

willscott
Copy link
Contributor

@willscott willscott commented Apr 7, 2020 •
edited
Loading

address #567

  • Are the appended options to lan/wan sufficient / correct?
  • What other interface methods should be exposed on Dual?
  • Do any more of the methods currently running in serial need to run in parallel?
  • Tests

@willscott willscott requested a review from Stebalien April 7, 2020 22:19
Stebalien
Stebalien reviewed Apr 7, 2020
View reviewed changes
dual/dual.go Outdated Show resolved Hide resolved
dual/dual.go Outdated Show resolved Hide resolved
dual/dual.go Show resolved Hide resolved
dual/dual.go Outdated Show resolved Hide resolved
dual/dual.go Show resolved Hide resolved
dual/dual.go Outdated Show resolved Hide resolved
dual/dual.go Outdated Show resolved Hide resolved
dual/dual.go Outdated Show resolved Hide resolved
dual/dual.go Outdated Show resolved Hide resolved
dual/dual.go Outdated Show resolved Hide resolved
Stebalien
Stebalien reviewed Apr 7, 2020
View reviewed changes
dual/dual.go Outdated Show resolved Hide resolved
@Stebalien
Copy link
Member

What other interface methods should be exposed on Dual?

I think we're good, actually. Go-ipfs can access what it needs by looking at the sub-DHTs directly.

@aarshkshah1992
Copy link
Contributor

@willscott Do you want an additional review on this ?

@willscott
Copy link
Contributor Author

@willscott Do you want an additional review on this ?

i wouldn't look too deeply at the parallel setups yet. i'll work on tests tomorrow and that'll get this code quite a bit cleaner. If you have comments on the interface / how it's interacting with the main DHT package, a glance wouldn't hurt.

The main point of feedback from @Stebalien at this point is that the LAN dht shouldn't default to ServerAuto mode - instead, we should first construct the WAN dht; make an accessor for mode to learn if it has been over-ridden from auto to client mode, and then construct the LAN DHT to default to server mode unless the WAN dht has been set to client.

@aarshkshah1992 aarshkshah1992 self-requested a review April 8, 2020 05:52
@aarshkshah1992
Copy link
Contributor

aarshkshah1992 commented Apr 8, 2020 •
edited
Loading

@willscott

The main point of feedback from @Stebalien at this point is that the LAN dht shouldn't default to ServerAuto mode - instead, we should first construct the WAN dht; make an accessor for mode to learn if it has been over-ridden from auto to client mode, and then construct the LAN DHT to default to server mode unless the WAN dht has been set to client.

Do you mean:

1. If AutoNAT sends the WAN DHT from auto to client -> LAN DHT should be instantiated with auto ?
If yes, how would the LAN DHT ever detect it's own reachability on private networks unless we have some sort of timeout as proposed in #564 ? The problem is that AutoNAT servers reject requests from clients on the same network.

2. If AutoNAT deems the WAN DHT to be a server, than ofcourse the LAN DHT should be a server ?
Makes sense.

@willscott
Copy link
Contributor Author

1. If AutoNAT sends the WAN DHT from auto to client -> LAN DHT should be instantiated with auto ?
If yes, how would the LAN DHT ever detect it's own reachability on private networks unless we have some sort of timeout as proposed in #564 ? The problem is that AutoNAT servers reject requests from clients on the same network.

If the user has constructed the dht with an explicit client mode option, the LAN should also end up only in client mode. If the user has not set a mode option (auto), or if the user has forced server mode, then the LAN DHT should run in server mode.

@willscott willscott marked this pull request as ready for review April 9, 2020 02:11
@willscott willscott requested a review from Stebalien April 9, 2020 02:24
Stebalien
Stebalien reviewed Apr 9, 2020
View reviewed changes
dual/dual.go Outdated Show resolved Hide resolved
dual/dual.go Outdated Show resolved Hide resolved
dual/dual.go Outdated Show resolved Hide resolved
dual/dual.go Outdated Show resolved Hide resolved
Stebalien
Stebalien reviewed Apr 9, 2020
View reviewed changes
dual/dual.go Outdated Show resolved Hide resolved
@willscott willscott requested a review from Stebalien April 9, 2020 04:15
Stebalien
Stebalien reviewed Apr 9, 2020
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Couple of small nits but looks good otherwise.

dual/dual.go Outdated Show resolved Hide resolved
dual/dual.go Outdated Show resolved Hide resolved
dual/dual.go Outdated Show resolved Hide resolved
dual/dual.go Outdated Show resolved Hide resolved
Stebalien
Stebalien approved these changes Apr 9, 2020
View reviewed changes
dual/dual.go

found := make(map[peer.ID]struct{}, count)
var pi peer.AddrInfo
for (zeroCount || count > 0) && (wanCh != nil || lanCh != nil) {
Copy link
Member

Choose a reason for hiding this comment

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

Ok, fine, this is definitely cleaner than the way I fixed this...

@Stebalien Stebalien merged commit 8a4963d into master Apr 9, 2020
@Stebalien Stebalien deleted the feat/dual branch April 9, 2020 18:37
@willscott willscott mentioned this pull request Apr 9, 2020
Closed
aschmahmann
aschmahmann reviewed Apr 12, 2020
View reviewed changes
dual/dual.go
if dht.activeWAN() {
return dht.WAN.Provide(ctx, key, announce)
}
return dht.LAN.Provide(ctx, key, announce)
Copy link
Contributor

Choose a reason for hiding this comment

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

@Stebalien @willscott I thought we were concerned about constantly providing/putting to people on our LANs. Did we decide that wasn't a big deal or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this only does the puts/provides to the LAN-filtered DHT when there are no discovered peers in the WAN DHT routing table.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh right, I'm being silly didn't see the return 🤦

@willscott willscott mentioned this pull request Apr 17, 2020
Closed
@ivan4th ivan4th mentioned this pull request Jan 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aschmahmann aschmahmann aschmahmann left review comments

@Stebalien Stebalien Stebalien approved these changes

@aarshkshah1992 aarshkshah1992 Awaiting requested review from aarshkshah1992

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

Successfully merging this pull request may close these issues.
4 participants
@willscott @Stebalien @aarshkshah1992 @aschmahmann