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

Server endpoints goroutine refactor #69

Merged
jeffreypicard merged 18 commits from server-endpoints-goroutine-refactor into master 2022-10-25 07:48:13 +02:00
Conversation 2 Commits 18 Files changed 21 +546 -307
jeffreypicard commented 2022-10-14 11:33:59 +02:00 (Migrated from github.com)
Copy link
No description provided.
moodyjon (Migrated from github.com) requested changes 2022-10-14 16:42:23 +02:00
db/db.go
@ -59,11 +59,7 @@ type ReadOnlyDBColumnFamily struct {
BlockedChannels map[string][]byte
FilteredStreams map[string][]byte
FilteredChannels map[string][]byte
moodyjon (Migrated from github.com) commented 2022-10-14 16:14:51 +02:00
Copy link

OpenIterators, ItMut not needed anymore.

`OpenIterator`s, `ItMut` not needed anymore.
db/db.go
@ -354,1 +338,3 @@
}
// Check if we've been told to shutdown in between getting created and getting here
if opts.Grp != nil && interruptRequested(opts.Grp.Ch()) {
opts.Grp.Done()
moodyjon (Migrated from github.com) commented 2022-10-14 16:02:16 +02:00
Copy link

if opts.Grp != nil && interruptRequested(opts.Grp.Ch()) {

This simplification eliminating ShutdownCalled is possible (I think) because you register the iterator with the stop group earlier. Now the registration happens in opts.WithDB()

`if opts.Grp != nil && interruptRequested(opts.Grp.Ch()) {` This simplification eliminating `ShutdownCalled` is possible (I think) because you register the iterator with the stop group earlier. Now the registration happens in `opts.WithDB()`
db/iteroptions.go
moodyjon (Migrated from github.com) commented 2022-10-14 15:56:06 +02:00
Copy link

Looks like this can be eliminated too. Only opts.Grp is really being used. As mentioned earlier, opts.DB.ShutdownCalled can be eliminated.

Looks like this can be eliminated too. Only `opts.Grp` is really being used. As mentioned earlier, `opts.DB.ShutdownCalled` can be eliminated.
server/args.go
moodyjon (Migrated from github.com) commented 2022-10-14 16:36:25 +02:00
Copy link

Not needed.

Not needed.
server/args.go
@ -146,3 +244,4 @@
// flags for disabling features
debug := parser.Flag("", "debug", &argparse.Options{Required: false, Help: "enable debug logging", Default: false})
disableEs := parser.Flag("", "disable-es", &argparse.Options{Required: false, Help: "Disable elastic search, for running/testing independently", Default: false})
disableLoadPeers := parser.Flag("", "disable-load-peers", &argparse.Options{Required: false, Help: "Disable load peers from disk at startup", Default: DefaultDisableLoadPeers})
moodyjon (Migrated from github.com) commented 2022-10-14 16:35:31 +02:00
Copy link

What is the reason to move/rename this? The package server_test seems like a fine location for it. Is it because it was in federation_test.go and shared by other test files?

What is the reason to move/rename this? The package `server_test` seems like a fine location for it. Is it because it was in `federation_test.go` and shared by other test files?
server/jsonrpc_service.go
moodyjon (Migrated from github.com) commented 2022-10-14 16:37:51 +02:00
Copy link

Please do the same registrations in session.go. It's in addSession() toward the bottom.

Please do the same registrations in session.go. It's in `addSession()` toward the bottom.
moodyjon (Migrated from github.com) commented 2022-10-14 16:41:39 +02:00
Copy link

I don't think you need separate service for each RPC. The reason we had to create them for blockchain.xxx.yyy RPCs was because there are TWO dots in the name.

Here the service would be named simply "server".

I don't think you need separate service for each RPC. The reason we had to create them for `blockchain.xxx.yyy` RPCs was because there are TWO dots in the name. Here the service would be named simply "server".
server/server.go
@ -272,4 +270,4 @@
if err != nil {
logrus.Warning(err)
}
}
moodyjon (Migrated from github.com) commented 2022-10-14 16:20:05 +02:00
Copy link

Ahaa. So this is how it gets set. You might consider passing grp down into LoadDatabase(). So it gets initialized ASAP. Also, don't forget the ReadonlyDBColumnFamilies instance constructed in db_test.go.

Ahaa. So this is how it gets set. You might consider passing `grp` down into `LoadDatabase()`. So it gets initialized ASAP. Also, don't forget the `ReadonlyDBColumnFamilies` instance constructed in db_test.go.
moodyjon (Migrated from github.com) reviewed 2022-10-14 16:44:00 +02:00
server/args.go
@ -146,3 +244,4 @@
// flags for disabling features
debug := parser.Flag("", "debug", &argparse.Options{Required: false, Help: "enable debug logging", Default: false})
disableEs := parser.Flag("", "disable-es", &argparse.Options{Required: false, Help: "Disable elastic search, for running/testing independently", Default: false})
disableLoadPeers := parser.Flag("", "disable-load-peers", &argparse.Options{Required: false, Help: "Disable load peers from disk at startup", Default: DefaultDisableLoadPeers})
moodyjon (Migrated from github.com) commented 2022-10-14 16:43:59 +02:00
Copy link

I think you could create util_test.go and put it there if the file name (federation_test.go) is the concern.

I think you could create util_test.go and put it there if the file name (federation_test.go) is the concern.
moodyjon (Migrated from github.com) reviewed 2022-10-14 16:47:46 +02:00
db/iteroptions.go
moodyjon (Migrated from github.com) commented 2022-10-14 16:47:46 +02:00
Copy link

This is great in that it enables other simplifications. However, we've got to be careful now about the use of IterOptions objects. Anytime one is constructed, it MUST later be used in IterCF(). Otherwise the refcount of active iterators leaks +1.

This is great in that it enables other simplifications. However, we've got to be careful now about the use of IterOptions objects. Anytime one is constructed, it MUST later be used in `IterCF()`. Otherwise the refcount of active iterators leaks +1.
jeffreypicard (Migrated from github.com) reviewed 2022-10-15 15:53:35 +02:00
server/args.go
@ -146,3 +244,4 @@
// flags for disabling features
debug := parser.Flag("", "debug", &argparse.Options{Required: false, Help: "enable debug logging", Default: false})
disableEs := parser.Flag("", "disable-es", &argparse.Options{Required: false, Help: "Disable elastic search, for running/testing independently", Default: false})
disableLoadPeers := parser.Flag("", "disable-load-peers", &argparse.Options{Required: false, Help: "Disable load peers from disk at startup", Default: DefaultDisableLoadPeers})
jeffreypicard (Migrated from github.com) commented 2022-10-15 15:53:34 +02:00
Copy link

Yes, this seems a little more general than to just be in federation_test.go I agree there's probably a need for a util_test package, I'll look at what we can add into something like that.

Yes, this seems a little more general than to just be in `federation_test.go` I agree there's probably a need for a util_test package, I'll look at what we can add into something like that.
jeffreypicard (Migrated from github.com) reviewed 2022-10-15 17:06:32 +02:00
db/iteroptions.go
jeffreypicard (Migrated from github.com) commented 2022-10-15 17:06:32 +02:00
Copy link

Yeah, I'm trying to weigh the pros and cons between requiring them in the "constructor" versus the flexibility of not requiring you have them in cases when they aren't needed.

Yeah, I'm trying to weigh the pros and cons between requiring them in the "constructor" versus the flexibility of not requiring you have them in cases when they aren't needed.
moodyjon (Migrated from github.com) approved these changes 2022-10-19 16:35:54 +02:00
db/db.go
moodyjon (Migrated from github.com) commented 2022-10-19 16:00:14 +02:00
Copy link

Is this for the goroutine created in RunDetectChanges()? If so, I suggest placing it there.

Is this for the goroutine created in `RunDetectChanges()`? If so, I suggest placing it there.
db/db_test.go
moodyjon (Migrated from github.com) commented 2022-10-19 16:16:01 +02:00
Copy link

db.Cleanup is already set inside GetProdDB(), so not needed. I suggest standardizing on that approach. Have GetProdDB, OpenAndFillTmpDBColumnFamilies, etc all set db.Cleanup and eliminate the toDefer return.

Every time GetProdDB and others are called, do defer db.Shutdown() as you are doing here.

`db.Cleanup` is already set inside `GetProdDB()`, so not needed. I suggest standardizing on that approach. Have `GetProdDB`, `OpenAndFillTmpDBColumnFamilies`, etc all set `db.Cleanup` and eliminate the `toDefer` return. Every time `GetProdDB` and others are called, do `defer db.Shutdown()` as you are doing here.
db/db_test.go
@ -303,3 +302,3 @@
url := "lbry://@Styxhexenhammer666#2/legacy-media-baron-les-moonves-(cbs#9"
filePath := "../testdata/FULL_resolve.csv"
db, _, toDefer, err := OpenAndFillTmpDBColumnFamlies(filePath)
db, _, err := OpenAndFillTmpDBColumnFamlies(filePath)
moodyjon (Migrated from github.com) commented 2022-10-19 16:18:32 +02:00
Copy link

Have OpenAndFillTmpDBColumnFamilies() assign db.Cleanup = toDefer inside to simplify this.

...Ditto all the other places calling OpenAndFillTmpDBColumnFamilies().

Have `OpenAndFillTmpDBColumnFamilies()` assign `db.Cleanup = toDefer` inside to simplify this. ...Ditto all the other places calling `OpenAndFillTmpDBColumnFamilies()`.
server/jsonrpc_blockchain_test.go
moodyjon (Migrated from github.com) commented 2022-10-19 16:24:39 +02:00
Copy link

Should this be NewDebug() like other tests? Ditto all tests in this file.

Should this be `NewDebug()` like other tests? Ditto all tests in this file.
moodyjon (Migrated from github.com) commented 2022-10-19 16:26:42 +02:00
Copy link

Is this needed?

Is this needed?
server/jsonrpc_blockchain_test.go
@ -281,8 +288,9 @@ func TestHeadersSubscribe(t *testing.T) {
func TestGetBalance(t *testing.T) {
secondaryPath := "asdf"
moodyjon (Migrated from github.com) commented 2022-10-19 16:25:36 +02:00
Copy link

defer db.Shutdown() to match style elsewhere.

`defer db.Shutdown()` to match style elsewhere.
server/server.go
@ -269,3 +268,2 @@
// var dbShutdown = func() {}
if !args.DisableResolve {
myDB, err = LoadDatabase(args)
myDB, err = LoadDatabase(args, grp)
moodyjon (Migrated from github.com) commented 2022-10-19 15:57:30 +02:00
Copy link

Not needed.

Not needed.
server/session.go
moodyjon (Migrated from github.com) commented 2022-10-19 16:34:07 +02:00
Copy link

sm.grp.Done() here, and sm.grp.Add(1) in (sm *SessionManager) start() to account for goroutine running manage().

`sm.grp.Done()` here, and `sm.grp.Add(1)` in `(sm *SessionManager) start()` to account for goroutine running `manage()`.
server/session.go
moodyjon (Migrated from github.com) commented 2022-10-19 16:35:00 +02:00
Copy link

RegisterTCPService -> RegisterName

`RegisterTCPService` -> `RegisterName`
moodyjon (Migrated from github.com) reviewed 2022-10-19 16:38:01 +02:00
server/jsonrpc_service.go
moodyjon (Migrated from github.com) commented 2022-10-19 16:38:01 +02:00
Copy link

Reminder: Remove commented code.

Reminder: Remove commented code.
jeffreypicard (Migrated from github.com) reviewed 2022-10-20 11:13:15 +02:00
db/db_test.go
jeffreypicard (Migrated from github.com) commented 2022-10-20 11:13:14 +02:00
Copy link

Agreed. Originally when I was developing this I was using the using the lower level db in some of the tests... I still might be actually I need to check. that's why I had a separate cleanup function I think I called it directly in the tests? Either way it's messy now and needs to be cleaned up.

Agreed. Originally when I was developing this I was using the using the lower level db in some of the tests... I still might be actually I need to check. that's why I had a separate cleanup function I think I called it directly in the tests? Either way it's messy now and needs to be cleaned up.
jeffreypicard (Migrated from github.com) reviewed 2022-10-24 13:45:12 +02:00
server/args.go
@ -146,3 +244,4 @@
// flags for disabling features
debug := parser.Flag("", "debug", &argparse.Options{Required: false, Help: "enable debug logging", Default: false})
disableEs := parser.Flag("", "disable-es", &argparse.Options{Required: false, Help: "Disable elastic search, for running/testing independently", Default: false})
disableLoadPeers := parser.Flag("", "disable-load-peers", &argparse.Options{Required: false, Help: "Disable load peers from disk at startup", Default: DefaultDisableLoadPeers})
jeffreypicard (Migrated from github.com) commented 2022-10-24 13:45:12 +02:00
Copy link

So I'm actually not going to do this right now. I just realized that the jsonrpc_blockchain_test.go is in the server package, and relies heavily on using non-exported functions and structs. Technically I think the idiom is supposed to be that we put the tests in a "server_test" package and then use the server_test_pkg.go as a pivot to wrap and export all the parts in the server package that need it. This will be a pretty significant change in jsonrpc_blockchain_test.go though, so let's go that in a separate pr.

So I'm actually not going to do this right now. I just realized that the jsonrpc_blockchain_test.go is in the server package, and relies heavily on using non-exported functions and structs. Technically I think the idiom is supposed to be that we put the tests in a "server_test" package and then use the server_test_pkg.go as a pivot to wrap and export all the parts in the server package that need it. This will be a pretty significant change in jsonrpc_blockchain_test.go though, so let's go that in a separate pr.
moodyjon commented 2022-10-25 03:52:44 +02:00 (Migrated from github.com)
Copy link

Latest looks good, and all my comments are addressed, but I would encourage you to move away from creating separate server_test and server packages. Avoids the need to wrestle with creating exported versions of private methods. I think lbcd puts tests in the same package as the code they test.

Latest looks good, and all my comments are addressed, but I would encourage you to move away from creating separate `server_test` and `server` packages. Avoids the need to wrestle with creating exported versions of private methods. I think lbcd puts tests in the same package as the code they test.
jeffreypicard commented 2022-10-25 07:48:00 +02:00 (Migrated from github.com)
Copy link

Latest looks good, and all my comments are addressed, but I would encourage you to move away from creating separate server_test and server packages. Avoids the need to wrestle with creating exported versions of private methods. I think lbcd puts tests in the same package as the code they test.

So I think maybe this is a bit to do with style, but also around how we imagine this being used, a lot of places mention that it's best practices to put the tests in their own package, i.e. https://medium.com/@matryer/5-simple-tips-and-tricks-for-writing-unit-tests-in-golang-619653f90742 but that's more I guess if we imagined other people importing our library and using it from the outside, anything internal it makes sense to be in the same package I suppose.

> Latest looks good, and all my comments are addressed, but I would encourage you to move away from creating separate `server_test` and `server` packages. Avoids the need to wrestle with creating exported versions of private methods. I think lbcd puts tests in the same package as the code they test. So I think maybe this is a bit to do with style, but also around how we imagine this being used, a lot of places mention that it's best practices to put the tests in their own package, i.e. https://medium.com/@matryer/5-simple-tips-and-tricks-for-writing-unit-tests-in-golang-619653f90742 but that's more I guess if we imagined other people importing our library and using it from the outside, anything internal it makes sense to be in the same package I suppose.
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#69
No description provided.
Delete branch "server-endpoints-goroutine-refactor"

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?