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

WIP: Json rpc federation, search/getclaimbyid, and shutdown #76

Merged
jeffreypicard merged 11 commits from json-rpc-federation-and-shutdown into master 2022-12-07 17:01:36 +01:00
Conversation 1 Commits 11 Files changed 20 +412 -131
jeffreypicard commented 2022-11-13 13:01:08 +01:00 (Migrated from github.com)
Copy link
No description provided.
moodyjon (Migrated from github.com) reviewed 2022-11-16 18:36:01 +01:00
server/jsonrpc_claimtrie.go
@ -28,0 +67,4 @@
metrics.RequestsCount.With(prometheus.Labels{"method": "blockchain.claimtrie.getclaimbyid"}).Inc()
// FIXME: this has txos and extras and so does GetClaimById?
allTxos := make([]*pb.Output, 0)
moodyjon (Migrated from github.com) commented 2022-11-16 18:36:00 +01:00
Copy link

OK, I see why you pass the server.Server into the session manager, and also store it in ClaimtrieService.

The python hub has a SearchIndex class which contains the search functionality of the HubServerService: 75d64f9dc6/hub/herald/search.py (L29)

The SearchIndex is passed off to the session manager instead of the whole HubServerService:
https://github.com/lbryio/hub/search?q=SearchIndex

Low priority to do this now, but splitting the search functionality (elastic search client, caches for search, etc) from server.Server into a SearchIndex struct/obj would be nice.

OK, I see why you pass the `server.Server` into the session manager, and also store it in `ClaimtrieService`. The python hub has a `SearchIndex` class which contains the search functionality of the `HubServerService`: https://github.com/lbryio/hub/blob/75d64f9dc6d3b2c913b8b10053bd3589e7a2e8eb/hub/herald/search.py#L29 The `SearchIndex` is passed off to the session manager instead of the whole `HubServerService`: https://github.com/lbryio/hub/search?q=SearchIndex Low priority to do this now, but splitting the search functionality (elastic search client, caches for search, etc) from `server.Server` into a `SearchIndex` struct/obj would be nice.
moodyjon (Migrated from github.com) requested changes 2022-11-17 20:02:45 +01:00
moodyjon (Migrated from github.com) left a comment
Copy link

I'm OK with having the *Server held inside sessionManager for now.

I'm OK with having the `*Server` held inside `sessionManager` for now.
db/db.go
moodyjon (Migrated from github.com) commented 2022-11-17 20:00:04 +01:00
Copy link

I don't see why this is disabled...

I don't see why this is disabled...
server/jsonrpc_federation.go
moodyjon (Migrated from github.com) commented 2022-11-17 19:39:11 +01:00
Copy link

Might as well complete this by adding sessionManager to PeersService (or other appropriate pointer). You have sessionManager.peersSubs already in place.

Might as well complete this by adding sessionManager to PeersService (or other appropriate pointer). You have `sessionManager.peersSubs` already in place.
server/notifier.go
@ -69,3 +76,4 @@
address := ":" + fmt.Sprintf("%d", s.Args.NotifierPort)
addr, err := net.ResolveTCPAddr("tcp", address)
if err != nil {
return err
moodyjon (Migrated from github.com) commented 2022-11-17 19:37:07 +01:00
Copy link

Probably need peerNotification case here.

Probably need `peerNotification` case here.
server/session.go
moodyjon (Migrated from github.com) commented 2022-11-17 19:45:04 +01:00
Copy link

Rename peersBool -> peersSub to match comment.

Rename `peersBool` -> `peersSub` to match comment.
server/session.go
@ -95,6 +100,13 @@ func (s *session) doNotify(notification interface{}) {
status = hex.EncodeToString(note.status)
}
moodyjon (Migrated from github.com) commented 2022-11-17 19:43:55 +01:00
Copy link
if !s.peersBool {
    return
}
``` if !s.peersBool { return } ```
signalsigterm.go
@ -19,3 +16,3 @@
shutdownRequestChannel = stopCh
func initsignals() {
interruptSignals = []os.Signal{os.Interrupt, syscall.SIGTERM}
}
moodyjon (Migrated from github.com) commented 2022-11-17 19:32:34 +01:00
Copy link

It looks like shutdownRequestChannel is not hooked up to anything now -- nobody sends anything on it that I see. Hence you were able to add the code lines 52-54 in signal.go without triggering the spam problem again. It (shutdownRequestChannel) should be eliminated if it's not being used.

It looks like `shutdownRequestChannel` is not hooked up to anything now -- nobody sends anything on it that I see. Hence you were able to add the code lines 52-54 in signal.go without triggering the spam problem again. It (`shutdownRequestChannel`) should be eliminated if it's not being used.
jeffreypicard (Migrated from github.com) reviewed 2022-11-17 23:04:39 +01:00
server/notifier.go
@ -69,3 +76,4 @@
address := ":" + fmt.Sprintf("%d", s.Args.NotifierPort)
addr, err := net.ResolveTCPAddr("tcp", address)
if err != nil {
return err
jeffreypicard (Migrated from github.com) commented 2022-11-17 23:04:39 +01:00
Copy link

Maybe. We need to refactor this whole section of code, but I think that ends up getting handled by the catch-all s.sessionManager.doNotify

Maybe. We need to refactor this whole section of code, but I think that ends up getting handled by the catch-all `s.sessionManager.doNotify`
jeffreypicard (Migrated from github.com) reviewed 2022-11-17 23:15:39 +01:00
signalsigterm.go
@ -19,3 +16,3 @@
shutdownRequestChannel = stopCh
func initsignals() {
interruptSignals = []os.Signal{os.Interrupt, syscall.SIGTERM}
}
jeffreypicard (Migrated from github.com) commented 2022-11-17 23:15:39 +01:00
Copy link

I think I left this in because the intention is that this program can be run in either client or server mode, so there would ostensibly be a control endpoint that lets it be shutdown from a local only port that you could run the binary and send it a command to shutdown versus a ctrl+c. Maybe it's right to remove it for now anyways, but I feel like it's feature we'll implement eventually.

I think I left this in because the intention is that this program can be run in either client or server mode, so there would ostensibly be a control endpoint that lets it be shutdown from a local only port that you could run the binary and send it a command to shutdown versus a ctrl+c. Maybe it's right to remove it for now anyways, but I feel like it's feature we'll implement eventually.
jeffreypicard (Migrated from github.com) reviewed 2022-11-17 23:18:05 +01:00
db/db.go
jeffreypicard (Migrated from github.com) commented 2022-11-17 23:18:05 +01:00
Copy link

This should be removed. After some refactoring the stop group StopAndWait gets called in the higher up server Shutdown routine.

This should be removed. After some refactoring the stop group StopAndWait gets called in the higher up server Shutdown routine.
jeffreypicard (Migrated from github.com) reviewed 2022-11-17 23:38:50 +01:00
server/jsonrpc_claimtrie.go
@ -28,0 +67,4 @@
metrics.RequestsCount.With(prometheus.Labels{"method": "blockchain.claimtrie.getclaimbyid"}).Inc()
// FIXME: this has txos and extras and so does GetClaimById?
allTxos := make([]*pb.Output, 0)
jeffreypicard (Migrated from github.com) commented 2022-11-17 23:38:50 +01:00
Copy link

Yup, this definitely needs to be refactored in the future.

Yup, this definitely needs to be refactored in the future.
moodyjon (Migrated from github.com) reviewed 2022-11-19 01:02:17 +01:00
server/notifier.go
@ -69,3 +76,4 @@
address := ":" + fmt.Sprintf("%d", s.Args.NotifierPort)
addr, err := net.ResolveTCPAddr("tcp", address)
if err != nil {
return err
moodyjon (Migrated from github.com) commented 2022-11-19 01:02:16 +01:00
Copy link

Never mind... I have been reading the code more, and NotifierChan is only populated with HeightHash notifications coming from observing the DB.

Never mind... I have been reading the code more, and NotifierChan is only populated with HeightHash notifications coming from observing the DB.
moodyjon (Migrated from github.com) reviewed 2022-11-19 01:10:47 +01:00
server/jsonrpc_federation.go
moodyjon (Migrated from github.com) commented 2022-11-19 01:10:46 +01:00
Copy link

Ahh.... I meant register the subscription in the sessionManager.peerSubs map. It needs registration there as it's communicating by JSONRPC. Then inside Server.notifyPeerSubs, call s.sessionManager.doNotify() to forward a notification to the session-based mechanism.

The stuff in federation.go is more oriented towards grpc service (and managing the list of known peers). If PeersSubscribe comes in via GRPC, there's a separate mapping Server.PeerSubs for those.

Ahh.... I meant register the subscription in the `sessionManager.peerSubs` map. It needs registration there as it's communicating by JSONRPC. Then inside `Server.notifyPeerSubs`, call `s.sessionManager.doNotify()` to forward a notification to the session-based mechanism. The stuff in federation.go is more oriented towards grpc service (and managing the list of known peers). If PeersSubscribe comes in via GRPC, there's a separate mapping `Server.PeerSubs` for those.
moodyjon (Migrated from github.com) requested changes 2022-11-19 01:20:03 +01:00
moodyjon (Migrated from github.com) left a comment
Copy link

https://github.com/lbryio/herald.go/pull/76/files#r1026987357

https://github.com/lbryio/herald.go/pull/76/files#r1026987357
jeffreypicard (Migrated from github.com) reviewed 2022-11-19 08:27:35 +01:00
server/notifier.go
@ -69,3 +76,4 @@
address := ":" + fmt.Sprintf("%d", s.Args.NotifierPort)
addr, err := net.ResolveTCPAddr("tcp", address)
if err != nil {
return err
jeffreypicard (Migrated from github.com) commented 2022-11-19 08:27:35 +01:00
Copy link

I send it the new peers in federation.go addPeer. So I'm pretty sure this does get notified of new peers.

I send it the new peers in federation.go `addPeer`. So I'm pretty sure this does get notified of new peers.
jeffreypicard (Migrated from github.com) reviewed 2022-11-19 08:34:17 +01:00
server/jsonrpc_federation.go
jeffreypicard (Migrated from github.com) commented 2022-11-19 08:34:17 +01:00
Copy link

Ahh, yeah, just hooking it into the gRPC like this isn't going to quite work I suppose hahah

Ahh, yeah, just hooking it into the gRPC like this isn't going to quite work I suppose hahah
moodyjon commented 2022-12-07 15:08:03 +01:00 (Migrated from github.com)
Copy link

f774823b2e

👍 🙏 Thank You!

https://github.com/lbryio/herald.go/pull/76/commits/f774823b2e1f953b6cba33627899923f42096c69 :thumbsup: :pray: Thank You!
moodyjon (Migrated from github.com) reviewed 2022-12-07 15:09:54 +01:00
server/session.go
@ -57,3 +64,3 @@
var params interface{}
switch notification.(type) {
switch note := notification.(type) {
case headerNotification:
moodyjon (Migrated from github.com) commented 2022-12-07 15:09:54 +01:00
Copy link

Nice!

Nice!
moodyjon (Migrated from github.com) reviewed 2022-12-07 15:10:54 +01:00
server/args.go
moodyjon (Migrated from github.com) commented 2022-12-07 15:10:53 +01:00
Copy link

You should add validatePort for these.

You should add `validatePort` for these.
moodyjon (Migrated from github.com) approved these changes 2022-12-07 15:18:32 +01:00
moodyjon (Migrated from github.com) left a comment
Copy link

Suggest adding validatePort for the port num arguments.

Suggest adding `validatePort` for the port num arguments.
Sign in to join this conversation.
Reviewers
No reviewers
moodyjon
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#76
No description provided.
Delete branch "json-rpc-federation-and-shutdown"

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?