-
Notifications
You must be signed in to change notification settings - Fork 85
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
v1 tests #25
v1 tests #25
Conversation
@davidmarkclements anyway to still support a general max sockets? in most cases you don't care much if you're acting as a server or client (having the individual is cool too). re, the lookup: false still emitting peers. that's a typo/misunderstanding. any dht operation will emit peers. on your another side note: no a connection should not be established in this case, until the lookup peer does another lookup op against the dht after the announce |
so the reason I split them was that it became apparent that treating them the same could become a footgun. I need to regather my thoughts, it might make sense to have a face to face convo about this one
ok.. I think that means that the implicit ergonomics of this can lead to accidental double operations (again may be better to explain at higher res)
at a high level this feels unintuitive - although it completely makes sense in a server/client paradigm (this is a similar tension to the separating server/client connections discussion) |
@davidmarkclements if you disable lookup and only do announce you are basically opting in to behaving as a server. happy to discuss the maxServer/Client stuff face to face. After reading the impl I'd worry a bit that support both maxPeers and this might be complicated, and maxPeers is more important than maxServer here. The way I initially considered implementing it was kinda like |
this PR is dependent on guts PR: hyperswarm/network#9 |
re peer removal #26 - which is based on this branch |
@davidmarkclements guts 1.0.0 is out here with your PR, you wanna bump it here? |
@davidmarkclements i'm still not sure i follow the maxPeers logic. when reading the code it seems to me like the client connections will max out at 16 (default) although maxPeers is set to 24? Maybe let's talk it through on our call |
discussion result was to set max clients default to infinity - done in hyperswarm/network@ed858f1 |
tests refactored to same approach as used in guts testing
100% coverage
at some point it will make sense to split test utils out (some code with guts is duped), but would like to do that later on
maxPeers
has been replaced withmaxClientSockets
andmaxServerSockets
(maxServerSockets
defaults to infinity for parity withserver.maxConnections
,maxClientSockets
default is 16)various small bug fixes
side note: @hyperswarm/discovery readme seems to suggest that passing
lookup: false
option to discovery.announce method will prevent the topic stream from emitting peers. This is not the case, whether lookup is true/false it will always emit peers - (I don't think dht-rpc has the functionality to control this)another side note: clarification needed here, if an announcing-only peer joins a topic after a lookup-only peer has joined that topic, should a connection be established between the peers?Currently a connection isn't established in this scenario.