This repository has been archived by the owner on Aug 2, 2021. It is now read-only.
network/newstream: new stream! protocol base implementation #1500
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
acud
force-pushed
the
new-syncer
branch
3 times, most recently
from
f43129d to
6815d14
Compare
nonsense
reviewed
Jun 21, 2019
nonsense
reviewed
Jun 21, 2019
nonsense
reviewed
Jun 21, 2019
nonsense
reviewed
Jun 21, 2019
nonsense
reviewed
Jun 21, 2019
nonsense
reviewed
Jun 21, 2019
nonsense
reviewed
Jun 21, 2019
nonsense
reviewed
Jun 21, 2019
add handlers.go file, syncer stream maintainance logic copy subscription diff functionality from old syncer network/syncer: add stream req handler network/syncer: add StreamInfoReq handler network/syncer: create initial StreamInfo message exchange remove peer
… (in a bigger star topology) simulation/bucket: remove ok param from function and panic instead network/syncer: renamed file
acud
changed the title
acud
changed the title
acud
changed the title
nonsense
reviewed
Jul 5, 2019
| // You should have received a copy of the GNU Lesser General Public License | ||
| // along with the Swarm library. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| package newstream |
There was a problem hiding this comment.
The package is called newstream. Probably we should not be prefixing most of the structs in this package with Stream.
There was a problem hiding this comment.
i'm not sure i understand. existing stream directory will be removed. the package is not the same package. do you want the types to be prefixed with NewStream?
There was a problem hiding this comment.
when your package is called stream or newstream, the types inside already have the context of the word / concept Stream, so no need to generally prefix them. For example newstream.StreamProvider -> newstream.Provider.
There was a problem hiding this comment.
fair enough 👍 I'll take care of it in the following iteration
nonsense
reviewed
Jul 5, 2019
| } | ||
| return p | ||
| } | ||
| func (p *Peer) Left() { |
There was a problem hiding this comment.
This is used at one place. Why not directly inline close(p.quit) there?
nonsense
reviewed
Jul 5, 2019
network/newstream/stream.go
Outdated
| var _ node.Service = (*SlipStream)(nil) | ||
| var ( | ||
| pollTime = 1 * time.Second | ||
| createStreamsDelay = 50 * time.Millisecond //to avoid a race condition where we send a message to a server that hasnt set up yet |
There was a problem hiding this comment.
Is createStreamsDelay really necessary? Can you give a concrete example of what you mean with we send a message to a server that hasnt set up yet?
nonsense
reviewed
Jul 5, 2019
| @@ -0,0 +1,64 @@ | |||
| // Copyright 2019 The Swarm Authors | |||
There was a problem hiding this comment.
Why do we need a separate common_test file?
nonsense
reviewed
Jul 5, 2019
| s.kad.On(np) | ||
| defer s.kad.Off(np) | ||
|
|
||
| sp := NewPeer(bp, s.intervalsStore, s.providers) |
There was a problem hiding this comment.
I think if sp contained all the handlers, and not peer, we wouldn't have to have such a weird structure where Peer is instantiated with pointers to providers and intervalsStore. A Peer should contain only what is relevant to a peer.
nonsense
previously approved these changes
Jul 5, 2019
nonsense
approved these changes
Jul 8, 2019
vojtechsimetka
added a commit
that referenced
this pull request
Jul 9, 2019
* master: network/newstream: new stream! protocol base implementation (#1500) swarm: fix bzz_info.port when using dynamic port allocation (#1537) cmd/swarm: make bzzaccount flag optional and add bzzkeyhex flag (#1531) cmd/swarm: remove separate function to parse env vars (#1536) network/bitvector: Multibit set/unset + string rep (#1530) swarm: 0.4.3 unstable (#1526) travis: also build on release tags (#1527) swarm: release v0.4.2 (#1496) network: bump bzz stream hive (#1522) docker: update ca-certificates file (#1525) Add swarm guide to /docs (#1513) network/simulation: Add ExecAdapter capability to swarm simulations (#1503)
chadsr
added a commit
to chadsr/swarm
that referenced
this pull request
Sep 23, 2019
* 'master' of github.com:ethersphere/swarm: (54 commits) api, chunk, cmd, shed, storage: add support for pinning content (ethersphere#1509) docs/swarm-guide: cleanup (ethersphere#1620) travis: split jobs into different stages (ethersphere#1615) simulation: retry if we hit a collision on tcp/udp ports (ethersphere#1616) api, chunk: rename Tag.New to Tag.Create (ethersphere#1614) pss: instrumentation and refactor (ethersphere#1580) api, cmd, network: add --disable-auto-connect flag (ethersphere#1576) changelog: fix typo (ethersphere#1605) version: update to v0.4.4 unstable (ethersphere#1603) swarm: release v0.4.3 (ethersphere#1602) network/retrieve: add bzz-retrieve protocol (ethersphere#1589) PoC: Network simulation framework (ethersphere#1555) network: structured output for kademlia table (ethersphere#1586) client: add bzz client, update smoke tests (ethersphere#1582) swarm-smoke: fix check max prox hosts for pull/push sync modes (ethersphere#1578) cmd/swarm: allow using a network interface by name for nat purposes (ethersphere#1557) pss: disable TestForwardBasic (ethersphere#1544) api, network: count chunk deliveries per peer (ethersphere#1534) network/newstream: new stream! protocol base implementation (ethersphere#1500) swarm: fix bzz_info.port when using dynamic port allocation (ethersphere#1537) ...
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR has originally set out to rewrite our existing pull syncer through a process of generalising and reiterating on the
stream!protocol.There has been a lot of work and examination of possible changes prior to the work done here. A lot of which has culminated into a preliminary spec with #1454.
This PR merges necessary changes to the wire protocol that were identified and deemed necessary while reimplementing the
stream!protocol, as well as basicPeerdata structure that will be needed to be used when incrementally introducing the protocol implementation, as well as the stream provider implementations (right now onlysyncstream).The actual implementations have been removed from this PR and will be introduced in subsequent PRs to ease the diff and for the sake of clarity.
The files have been placed into
network/newstreamand the package namednewstream, all until we can get rid of existingstreamimplementation and move in the new implementation into place.related to:
#1451
ethersphere/user-stories#51