LBRYCommunity/herald.go
1
0
Fork 0
Code Issues 16 Pull requests Projects Releases 14 Packages Wiki Activity

UDPServer / ip address resolution #24

Merged
jeffreypicard merged 12 commits from feature/18/jeffreypicard/udp-endpoint into master 2021-12-06 17:57:47 +01:00
Conversation 2 Commits 12 Files changed 13 +6563 -265
jeffreypicard commented 2021-11-10 01:44:45 +01:00 (Migrated from github.com)
Copy link

UDP Ping/Pong protocol implemented, unit tests against itself and prod server.

Also switched to serving udp on the same port as grpc, and taking that
into account when pinging other hubs with udp.

UDP Ping/Pong protocol implemented, unit tests against itself and prod server. Also switched to serving udp on the same port as grpc, and taking that into account when pinging other hubs with udp.
jackrobison (Migrated from github.com) reviewed 2021-11-10 01:44:45 +01:00
lyoshenka (Migrated from github.com) reviewed 2021-11-10 01:44:45 +01:00
lyoshenka (Migrated from github.com) reviewed 2021-11-11 23:19:03 +01:00
server/server.go
@ -38,3 +38,3 @@
PeerServers map[string]*FederatedServer
PeerServers map[string]*Peer
PeerServersMut sync.RWMutex
NumPeerServers *int64
lyoshenka (Migrated from github.com) commented 2021-11-11 23:17:35 +01:00
Copy link

there's a net.IP type that you should use for IPs

there's a `net.IP` type that you should use for IPs
lyoshenka commented 2021-11-11 23:19:57 +01:00 (Migrated from github.com)
Copy link

Also switched to serving udp on the same port as grpc

This is good. We regret having separate ports for tcp and udp in the client.

> Also switched to serving udp on the same port as grpc This is good. We regret having separate ports for tcp and udp in the client.
lyoshenka (Migrated from github.com) reviewed 2021-12-01 16:27:41 +01:00
server/federation.go
@ -22,2 +22,4 @@
Port string
LastSeen time.Time
}
lyoshenka (Migrated from github.com) commented 2021-12-01 16:18:06 +01:00
Copy link

weird. how does that happen?

weird. how does that happen?
server/federation.go
lyoshenka (Migrated from github.com) commented 2021-12-01 16:19:22 +01:00
Copy link

weird that you're passing this function a pb.ServerMessage. seems like it should take an address and port instead so it can be used elsewhere

weird that you're passing this function a pb.ServerMessage. seems like it should take an address and port instead so it can be used elsewhere
server/federation.go
@ -222,8 +265,11 @@ func (s *Server) writePeers() {
}
lyoshenka (Migrated from github.com) commented 2021-12-01 16:22:02 +01:00
Copy link

in general, comments should describe the interface and not the implementation. say what it means to notify a peer, not what functions are called inside this one. thats more informative and you can change the implementation without changing the comment.

in general, comments should describe the interface and not the implementation. say what it means to notify a peer, not what functions are called inside this one. thats more informative and you can change the implementation without changing the comment.
lyoshenka (Migrated from github.com) commented 2021-12-01 16:22:39 +01:00
Copy link

if you haven't read Philosophy of Software Design, i strongly recommend it (esp the chapters on comments). i can buy you a copy if you'd like

if you haven't read Philosophy of Software Design, i strongly recommend it (esp the chapters on comments). i can buy you a copy if you'd like
server/federation.go
@ -286,3 +328,1 @@
if s.Args.Port == msg.Port &&
(localHosts[msg.Address] || msg.Address == s.Args.Host) {
log.Printf("%s:%s addPeer: Self peer, skipping...\n", s.Args.Host, s.Args.Port)
// addPeer takes a new peer, optionally checks to see if they're online, and
lyoshenka (Migrated from github.com) commented 2021-12-01 16:25:00 +01:00
Copy link

this is a good comment, though i'd delete "takes a new peer as a pb.ServerMessage" since you can tell that from the signature (though maybe rename msg to newPeer since that's what it is). and in fact, maybe you need a Peer type so pb.ServerMessage is not doing double duty

this is a good comment, though i'd delete "takes a new peer as a pb.ServerMessage" since you can tell that from the signature (though maybe rename `msg` to `newPeer` since that's what it is). and in fact, maybe you need a Peer type so pb.ServerMessage is not doing double duty
server/udp.go
lyoshenka (Migrated from github.com) commented 2021-12-01 16:26:32 +01:00
Copy link

should this be a const?

should this be a const?
jeffreypicard (Migrated from github.com) reviewed 2021-12-01 16:34:09 +01:00
server/federation.go
@ -22,2 +22,4 @@
Port string
LastSeen time.Time
}
jeffreypicard (Migrated from github.com) commented 2021-12-01 16:34:08 +01:00
Copy link

This happens when the net.IP type is empty and gets turned into a string.

This happens when the net.IP type is empty and gets turned into a string.
jeffreypicard (Migrated from github.com) reviewed 2021-12-01 16:36:52 +01:00
server/udp.go
jeffreypicard (Migrated from github.com) commented 2021-12-01 16:36:52 +01:00
Copy link

You're right, I thought I had some reason to make it a var originally, but I can't remember now, I'll change that.

You're right, I thought I had some reason to make it a var originally, but I can't remember now, I'll change that.
jeffreypicard (Migrated from github.com) reviewed 2021-12-01 16:41:37 +01:00
server/federation.go
jeffreypicard (Migrated from github.com) commented 2021-12-01 16:41:37 +01:00
Copy link

Agreed.

Agreed.
jeffreypicard (Migrated from github.com) reviewed 2021-12-01 16:54:51 +01:00
server/federation.go
@ -286,3 +328,1 @@
if s.Args.Port == msg.Port &&
(localHosts[msg.Address] || msg.Address == s.Args.Host) {
log.Printf("%s:%s addPeer: Self peer, skipping...\n", s.Args.Host, s.Args.Port)
// addPeer takes a new peer, optionally checks to see if they're online, and
jeffreypicard (Migrated from github.com) commented 2021-12-01 16:54:51 +01:00
Copy link

I agree. I actually do have a type exactly for this, but it's still called FederatedServer because that's what I called it months ago when I started this. I think I'll actually do a little refactor and rename that to Peer throughout the code base. It definitely makes more sense.

I agree. I actually do have a type exactly for this, but it's still called FederatedServer because that's what I called it months ago when I started this. I think I'll actually do a little refactor and rename that to Peer throughout the code base. It definitely makes more sense.
jeffreypicard (Migrated from github.com) reviewed 2021-12-02 01:37:17 +01:00
server/federation.go
@ -222,8 +265,11 @@ func (s *Server) writePeers() {
}
jeffreypicard (Migrated from github.com) commented 2021-12-02 01:37:17 +01:00
Copy link

Makes sense! I'll take a look at that book.

Makes sense! I'll take a look at that book.
lyoshenka (Migrated from github.com) reviewed 2021-12-03 17:26:55 +01:00
server/federation.go
lyoshenka (Migrated from github.com) commented 2021-12-03 17:20:07 +01:00
Copy link

what is Ts? preferably make the name more descriptive or add a comment

i know Go loves short names, but i'm not a fan. here's a heuristic: the less distance (lines of code) between var declaration and var use, the shorter the name can be. so i is perfectly fine as the index of a loop, but its bad as a property of a struct

what is `Ts`? preferably make the name more descriptive or add a comment i know Go loves short names, but i'm not a fan. here's a heuristic: the less distance (lines of code) between var declaration and var use, the shorter the name can be. so `i` is perfectly fine as the index of a loop, but its bad as a property of a struct
server/federation.go
lyoshenka (Migrated from github.com) commented 2021-12-03 17:23:48 +01:00
Copy link

very minor, but i brought this up in the last review so i wanted to point out another example: this comment reveals implementation details. id instead say something like "getAndSetExternalIp detects the server's external IP and stores it"

very minor, but i brought this up in the last review so i wanted to point out another example: this comment reveals implementation details. id instead say something like "getAndSetExternalIp detects the server's external IP and stores it"
server/server.go
@ -45,3 +44,4 @@
ExternalIP net.IP
pb.UnimplementedHubServer
}
lyoshenka (Migrated from github.com) commented 2021-12-03 17:21:50 +01:00
Copy link

nice

nice
server/udp.go
@ -17,1 +20,4 @@
const magic = 1446058291
const protocolVersion = 1
const defaultFlags = 0b00000000
const availableFlag = 0b00000001
lyoshenka (Migrated from github.com) commented 2021-12-03 17:26:13 +01:00
Copy link

can you add a comment describing what this is for? why is the (wrong) genesis blocktime stored in a var called magic?

can you add a comment describing what this is for? why is the (wrong) genesis blocktime stored in a var called `magic`?
jeffreypicard (Migrated from github.com) reviewed 2021-12-03 17:40:02 +01:00
server/federation.go
jeffreypicard (Migrated from github.com) commented 2021-12-03 17:40:02 +01:00
Copy link

Makes sense.

Makes sense.
jeffreypicard (Migrated from github.com) reviewed 2021-12-03 17:41:44 +01:00
server/udp.go
@ -17,1 +20,4 @@
const magic = 1446058291
const protocolVersion = 1
const defaultFlags = 0b00000000
const availableFlag = 0b00000001
jeffreypicard (Migrated from github.com) commented 2021-12-03 17:41:44 +01:00
Copy link

Sure, so the comment is just stolen from the Python implementation and it's an arbitrary number needed for the UDPPing protocol, but I'll add a comment saying that.

Sure, so the comment is just stolen from the Python implementation and it's an arbitrary number needed for the UDPPing protocol, but I'll add a comment saying that.
jeffreypicard (Migrated from github.com) reviewed 2021-12-03 17:46:51 +01:00
server/federation.go
jeffreypicard (Migrated from github.com) commented 2021-12-03 17:46:51 +01:00
Copy link

Agreed, this is really a last seen timestamp, that I decided to call Ts for "timestamp" I'll rename it to LastSeen, I think that's more correct and descriptive.

Agreed, this is really a last seen timestamp, that I decided to call Ts for "timestamp" I'll rename it to LastSeen, I think that's more correct and descriptive.
kauffj commented 2021-12-06 17:17:02 +01:00 (Migrated from github.com)
Copy link

Jeff has permission to merge

Jeff has permission to merge
Sign in to join this conversation.
Reviewers
No reviewers
jackrobison
lyoshenka
Labels
Clear labels   
consider soon

Discuss this issue at the next planning meeting, then remove this label

  
documentation

Improvements or additions to documentation

  
good first issue

Good for newcomers

  
hacktoberfest

Welcome to Hacktoberfest

  
help wanted

Extra attention is needed

  
priority: blocker

Issue is blocking release, do ASAP

  
priority: high

Work needs to be moved into sprint ASAP

  
priority: low

Work should be done but can stay in the backlog for now

  
priority: medium

Work needs to be done within 2-3 sprints

  
type: bug

Existing functionality is wrong or broken

  
type: discussion

A conversation. No specific changes requested

  
type: improvement

Existing (or partially existing) functionality needs to be changed

  
type: new feature

New functionality that does not exist yet

  
type: refactor

Minimal user-visible changes, but significant internal work

  
type: task

Either work that's not related to the code, or a small chore that does not fit into other categories

No labels
consider soon
documentation
good first issue
hacktoberfest
help wanted
priority: blocker
priority: high
priority: low
priority: medium
type: bug
type: discussion
type: improvement
type: new feature
type: refactor
type: task
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/herald.go#24
No description provided.
Delete branch "feature/18/jeffreypicard/udp-endpoint"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?