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

Most of federation is written, need to finish udp and test #20

Merged
jeffreypicard merged 9 commits from feature/6/jeffreypicard/hub-federation into master 2021-11-05 16:14:06 +01:00
Conversation 0 Commits 9 Files changed 16 +2047 -497
jeffreypicard commented 2021-10-25 17:27:59 +02:00 (Migrated from github.com)
Copy link

Also some code reorg. Lots of new potential for metrics that I should also take advantage of. Needs much much more testing.

Also some code reorg. Lots of new potential for metrics that I should also take advantage of. Needs much much more testing.
jackrobison (Migrated from github.com) reviewed 2021-10-25 17:27:59 +02:00
lyoshenka (Migrated from github.com) reviewed 2021-10-27 16:28:21 +02:00
lyoshenka (Migrated from github.com) left a comment
Copy link

im having trouble understanding some of federation.go. could you add comments to your functions using the godoc style. or at least to the exported ones.

in general i'd like good comments on exported functions and at least a one-liner on unexported ones (except if they're short and its super obvious whats going on). you should think of comments as part of the API. they explain the contract in a way that just a function name and args list cannot.

also i don't see tests for any of this. im not an expert on testing, but my understanding was that it's good to write some tests first or maybe at the same time as you write the code, and that writing tests afterwards is less valuable. whats your take on tests?

im having trouble understanding some of federation.go. could you add comments to your functions using the godoc style. or at least to the exported ones. in general i'd like good comments on exported functions and at least a one-liner on unexported ones (except if they're short and its super obvious whats going on). you should think of comments as part of the API. they explain the contract in a way that just a function name and args list cannot. also i don't see tests for any of this. im not an expert on testing, but my understanding was that it's good to write some tests first or maybe at the same time as you write the code, and that writing tests afterwards is less valuable. whats your take on tests?
server/federation.go
lyoshenka (Migrated from github.com) commented 2021-10-27 16:22:51 +02:00
Copy link

why do servers subscribe to peers? what messages are they listening for?

i don't think we want every hub to subscribe to every other hub

why do servers subscribe to peers? what messages are they listening for? i don't think we want every hub to subscribe to every other hub
server/server.go
@ -136,0 +157,4 @@
opts := []elastic.ClientOptionFunc{
elastic.SetSniff(true),
elastic.SetSnifferTimeoutStartup(time.Second * 60),
elastic.SetSnifferTimeout(time.Second * 60),
lyoshenka (Migrated from github.com) commented 2021-10-27 16:26:05 +02:00
Copy link

you're starting a bunch of goroutines. do you have a graceful way of shutting them down when the server is shut down? or maybe we don't need one?

you're starting a bunch of goroutines. do you have a graceful way of shutting them down when the server is shut down? or maybe we don't need one?
server/udp.go
@ -0,0 +13,4 @@
const maxBufferSize = 1024
// genesis blocktime (which is actually wrong)
const magic = 1446058291
lyoshenka (Migrated from github.com) commented 2021-10-27 16:26:57 +02:00
Copy link

this needs a comment. whats magic?

this needs a comment. whats magic?
jeffreypicard (Migrated from github.com) reviewed 2021-10-27 16:35:05 +02:00
server/server.go
@ -136,0 +157,4 @@
opts := []elastic.ClientOptionFunc{
elastic.SetSniff(true),
elastic.SetSnifferTimeoutStartup(time.Second * 60),
elastic.SetSnifferTimeout(time.Second * 60),
jeffreypicard (Migrated from github.com) commented 2021-10-27 16:35:04 +02:00
Copy link

You're right, I think these should all have another case in the select on ctx.Done() probably to clean up.

You're right, I think these should all have another case in the select on ctx.Done() probably to clean up.
jeffreypicard (Migrated from github.com) reviewed 2021-10-27 16:37:53 +02:00
server/federation.go
jeffreypicard (Migrated from github.com) commented 2021-10-27 16:37:53 +02:00
Copy link

So my idea here is to select the fastest other hub that this hub knows about, and subscribe to it for peer updates. The current getFastestPeer is just a place holder and will be implemented with udp so it doesn't dos other servers trying to connect to them all. So each one will be subscribed to one other.

So my idea here is to select the fastest other hub that this hub knows about, and subscribe to it for peer updates. The current getFastestPeer is just a place holder and will be implemented with udp so it doesn't dos other servers trying to connect to them all. So each one will be subscribed to one other.
jeffreypicard (Migrated from github.com) reviewed 2021-10-27 16:39:14 +02:00
server/udp.go
@ -0,0 +13,4 @@
const maxBufferSize = 1024
// genesis blocktime (which is actually wrong)
const magic = 1446058291
jeffreypicard (Migrated from github.com) commented 2021-10-27 16:39:14 +02:00
Copy link

This file definitely needs more comments and tests, but this is basically all lifted from here https://github.com/lbryio/lbry-sdk/blob/master/lbry/wallet/server/udp.py#L12

This file definitely needs more comments and tests, but this is basically all lifted from here https://github.com/lbryio/lbry-sdk/blob/master/lbry/wallet/server/udp.py#L12
lyoshenka (Migrated from github.com) reviewed 2021-11-02 15:04:08 +01:00
.github/workflows/build-short.yml
lyoshenka (Migrated from github.com) commented 2021-11-02 14:56:40 +01:00
Copy link

do you also test for race conditions (-race)? this makes tests way slower but its worth doing manually for parts that may have races

do you also test for race conditions (`-race`)? this makes tests way slower but its worth doing manually for parts that may have races
internal/metrics/metrics.go
@ -25,0 +26,4 @@
Name: "peers_known",
Help: "Number of peers we know about.",
})
PeersSubscribed = promauto.NewGauge(prometheus.GaugeOpts{
lyoshenka (Migrated from github.com) commented 2021-11-02 14:57:09 +01:00
Copy link

is this still useful if we're not subscribing to peers anymore?

is this still useful if we're not subscribing to peers anymore?
main.go
lyoshenka (Migrated from github.com) commented 2021-11-02 14:57:59 +01:00
Copy link

i think this method is typically called server.New() or server.NewServer(). i prefer the former since the package is already called server

i think this method is typically called `server.New()` or `server.NewServer()`. i prefer the former since the package is already called `server`
protobuf/definitions/hub.proto
@ -10,1 +10,4 @@
rpc Ping (EmptyMessage) returns (StringValue) {}
rpc Hello (HelloMessage) returns (HelloMessage) {}
rpc AddPeer (ServerMessage) returns (StringValue) {}
rpc PeerSubscribe (ServerMessage) returns (StringValue) {}
lyoshenka (Migrated from github.com) commented 2021-11-02 14:59:00 +01:00
Copy link

we dropped this right?

we dropped this right?
server/args.go
@ -0,0 +1,201 @@
package server
lyoshenka (Migrated from github.com) commented 2021-11-02 14:59:46 +01:00
Copy link

good idea putting this stuff in a separate file

good idea putting this stuff in a separate file
server/federation.go
@ -0,0 +118,4 @@
// subscribeToPeer subscribes us to a peer to we'll get updates about their
// known peers.
func (s *Server) subscribeToPeer(peer *FederatedServer) error {
lyoshenka (Migrated from github.com) commented 2021-11-02 15:01:17 +01:00
Copy link

more subscribing stuff. maybe im confused about what we decided to do with it? i thought you were putting it into a separate branch to keep for later.

more subscribing stuff. maybe im confused about what we decided to do with it? i thought you were putting it into a separate branch to keep for later.
server/server.go
@ -108,0 +114,4 @@
s.PeerServersMut.RUnlock()
s.PeerServersMut.Lock()
s.PeerServers[key] = peer
s.PeerServersMut.Unlock()
lyoshenka (Migrated from github.com) commented 2021-11-02 15:02:39 +01:00
Copy link

dont think this is necessary since its the default value

dont think this is necessary since its the default value
server/udp.go
@ -0,0 +12,4 @@
)
const maxBufferSize = 1024
// genesis blocktime (which is actually wrong)
lyoshenka (Migrated from github.com) commented 2021-11-02 15:03:41 +01:00
Copy link

lol

lol
jeffreypicard (Migrated from github.com) reviewed 2021-11-02 15:12:07 +01:00
internal/metrics/metrics.go
@ -25,0 +26,4 @@
Name: "peers_known",
Help: "Number of peers we know about.",
})
PeersSubscribed = promauto.NewGauge(prometheus.GaugeOpts{
jeffreypicard (Migrated from github.com) commented 2021-11-02 15:12:07 +01:00
Copy link

We are still subscribing to peers, we're just not holding the connection open. So this basically just mirrors the number of elements in the subscribed peers internal data structure.

We are still subscribing to peers, we're just not holding the connection open. So this basically just mirrors the number of elements in the subscribed peers internal data structure.
jeffreypicard (Migrated from github.com) reviewed 2021-11-02 15:14:28 +01:00
main.go
jeffreypicard (Migrated from github.com) commented 2021-11-02 15:14:28 +01:00
Copy link

Makes sense, I think I have some deep down avoidance measures against making functions called "new" because of the years of abuse from OO languages I've endured.

Makes sense, I think I have some deep down avoidance measures against making functions called "new" because of the years of abuse from OO languages I've endured.
jeffreypicard (Migrated from github.com) reviewed 2021-11-02 15:16:33 +01:00
protobuf/definitions/hub.proto
@ -10,1 +10,4 @@
rpc Ping (EmptyMessage) returns (StringValue) {}
rpc Hello (HelloMessage) returns (HelloMessage) {}
rpc AddPeer (ServerMessage) returns (StringValue) {}
rpc PeerSubscribe (ServerMessage) returns (StringValue) {}
jeffreypicard (Migrated from github.com) commented 2021-11-02 15:16:33 +01:00
Copy link

Nope, we're still subscribing to peers, just not holding the connect open anymore, but we need a way to tell the server who to communicate their updates to. The only other option would be to just use the list of all known peers and broadcast it, but these are TCP connections with probably quite a bit of overhead, so I don't think that would be very scalable.

Nope, we're still subscribing to peers, just not holding the connect open anymore, but we need a way to tell the server who to communicate their updates to. The only other option would be to just use the list of all known peers and broadcast it, but these are TCP connections with probably quite a bit of overhead, so I don't think that would be very scalable.
jeffreypicard (Migrated from github.com) reviewed 2021-11-02 15:17:00 +01:00
server/federation.go
@ -0,0 +118,4 @@
// subscribeToPeer subscribes us to a peer to we'll get updates about their
// known peers.
func (s *Server) subscribeToPeer(peer *FederatedServer) error {
jeffreypicard (Migrated from github.com) commented 2021-11-02 15:17:00 +01:00
Copy link

Nope, just the streaming pieces. We still need to subscribe.

Nope, just the streaming pieces. We still need to subscribe.
jeffreypicard (Migrated from github.com) reviewed 2021-11-02 15:18:28 +01:00
server/server.go
@ -108,0 +114,4 @@
s.PeerServersMut.RUnlock()
s.PeerServersMut.Lock()
s.PeerServers[key] = peer
s.PeerServersMut.Unlock()
jeffreypicard (Migrated from github.com) commented 2021-11-02 15:18:27 +01:00
Copy link

I think the compiler complains about it being accessed possibly without being set... but I could just initialize it at declaration. That would simplify this.

I think the compiler complains about it being accessed possibly without being set... but I could just initialize it at declaration. That would simplify this.
jeffreypicard (Migrated from github.com) reviewed 2021-11-02 15:19:13 +01:00
server/udp.go
@ -0,0 +12,4 @@
)
const maxBufferSize = 1024
// genesis blocktime (which is actually wrong)
jeffreypicard (Migrated from github.com) commented 2021-11-02 15:19:13 +01:00
Copy link

This comment was stolen verbatim from the python code btw.

This comment was stolen verbatim from the python code btw.
jeffreypicard (Migrated from github.com) reviewed 2021-11-02 15:22:52 +01:00
server/server.go
@ -136,0 +157,4 @@
opts := []elastic.ClientOptionFunc{
elastic.SetSniff(true),
elastic.SetSnifferTimeoutStartup(time.Second * 60),
elastic.SetSnifferTimeout(time.Second * 60),
jeffreypicard (Migrated from github.com) commented 2021-11-02 15:22:52 +01:00
Copy link

I ultimately got rid of most of these go routines, I was essentially just using the channel as a lock so it could do a bunch of other logic in an asynchronous manner from the endpoint, but after more careful consideration that logic needed to be synchronous and there was no longer any justification for the increased complexity and error prone code that comes with channel, so I'm using a sync.Map for a shared datastructure.

I ultimately got rid of most of these go routines, I was essentially just using the channel as a lock so it could do a bunch of other logic in an asynchronous manner from the endpoint, but after more careful consideration that logic needed to be synchronous and there was no longer any justification for the increased complexity and error prone code that comes with channel, so I'm using a sync.Map for a shared datastructure.
lyoshenka (Migrated from github.com) reviewed 2021-11-02 15:31:06 +01:00
server/server.go
@ -136,0 +157,4 @@
opts := []elastic.ClientOptionFunc{
elastic.SetSniff(true),
elastic.SetSnifferTimeoutStartup(time.Second * 60),
elastic.SetSnifferTimeout(time.Second * 60),
lyoshenka (Migrated from github.com) commented 2021-11-02 15:31:06 +01:00
Copy link

have you read the docs for sync.Map? its kind of an unfortunate name because its not meant to be a generic synchronized map. its meant for a narrower use case

have you read the docs for sync.Map? its kind of an unfortunate name because its not meant to be a generic synchronized map. its meant for a narrower use case
jeffreypicard (Migrated from github.com) reviewed 2021-11-02 15:54:46 +01:00
server/server.go
@ -136,0 +157,4 @@
opts := []elastic.ClientOptionFunc{
elastic.SetSniff(true),
elastic.SetSnifferTimeoutStartup(time.Second * 60),
elastic.SetSnifferTimeout(time.Second * 60),
jeffreypicard (Migrated from github.com) commented 2021-11-02 15:54:46 +01:00
Copy link

Of course not, I found an API that looked like it fit the use case and barreled on ahead. I'll take a look at that. The other implementation would just be mutex that you lock/unlock manually around a regular map, which isn't difficult either.

Of course not, I found an API that looked like it fit the use case and barreled on ahead. I'll take a look at that. The other implementation would just be mutex that you lock/unlock manually around a regular map, which isn't difficult either.
jeffreypicard (Migrated from github.com) reviewed 2021-11-02 16:20:21 +01:00
.github/workflows/build-short.yml
jeffreypicard (Migrated from github.com) commented 2021-11-02 16:20:21 +01:00
Copy link

Good call, I'll add that. They do pass with it, but I've already removed most of the go routine / channel stuff which is probably where those bugs would be.

Good call, I'll add that. They do pass with it, but I've already removed most of the go routine / channel stuff which is probably where those bugs would be.
lyoshenka (Migrated from github.com) reviewed 2021-11-02 16:37:43 +01:00
server/server.go
@ -136,0 +157,4 @@
opts := []elastic.ClientOptionFunc{
elastic.SetSniff(true),
elastic.SetSnifferTimeoutStartup(time.Second * 60),
elastic.SetSnifferTimeout(time.Second * 60),
lyoshenka (Migrated from github.com) commented 2021-11-02 16:37:43 +01:00
Copy link

yea that's what they want you to do most of the time - roll your own map + mutex. its annoying, but in line with their overall philosophy.

yea that's what they want you to do most of the time - roll your own map + mutex. its annoying, but in line with their overall philosophy.
lyoshenka (Migrated from github.com) approved these changes 2021-11-05 14:46:05 +01:00
server/federation.go
@ -0,0 +119,4 @@
// subscribeToPeer subscribes us to a peer to we'll get updates about their
// known peers.
func (s *Server) subscribeToPeer(peer *FederatedServer) error {
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
lyoshenka (Migrated from github.com) commented 2021-11-05 14:37:57 +01:00
Copy link

does this just return the first peer every time? i'm confused

does this just return the first peer every time? i'm confused
lyoshenka (Migrated from github.com) reviewed 2021-11-05 14:48:17 +01:00
server/federation.go
@ -0,0 +119,4 @@
// subscribeToPeer subscribes us to a peer to we'll get updates about their
// known peers.
func (s *Server) subscribeToPeer(peer *FederatedServer) error {
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
lyoshenka (Migrated from github.com) commented 2021-11-05 14:48:17 +01:00
Copy link

if yes, is this actually getting the fastest peer?

if yes, is this actually getting the fastest peer?
jeffreypicard (Migrated from github.com) reviewed 2021-11-05 15:07:07 +01:00
server/federation.go
@ -0,0 +119,4 @@
// subscribeToPeer subscribes us to a peer to we'll get updates about their
// known peers.
func (s *Server) subscribeToPeer(peer *FederatedServer) error {
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
jeffreypicard (Migrated from github.com) commented 2021-11-05 15:07:06 +01:00
Copy link

Yeah, it's just a placeholder right now for two reasons, we're currently subscribing to all the peers anyways and this should be implemented with the udp ping protocol that I haven't finished yet.

Yeah, it's just a placeholder right now for two reasons, we're currently subscribing to all the peers anyways and this should be implemented with the udp ping protocol that I haven't finished yet.
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#20
No description provided.
Delete branch "feature/6/jeffreypicard/hub-federation"

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?