Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SDK selection refactor #197

Merged
merged 18 commits into from
Apr 22, 2020
Merged

SDK selection refactor #197

merged 18 commits into from
Apr 22, 2020

Conversation

lyoshenka
Copy link
Contributor

@lyoshenka lyoshenka commented Apr 13, 2020

here's how I want to make wallet selection work:

  • request comes into the proxy handler
  • if user is not authenticated (aka does not have a verified email), they get a random sdk
  • if they are authenticated but don't have an sdk assigned in the database, they get assigned the least-loaded sdk (and that's saved in the db)
  • if they do have one assigned, they get the one they have
  • all this logic happens in one place, either in a middleware or in a function that can be called to get the Caller. it should work the same way for proxy calls as for publishing (right now there are two slightly different paths)

i'm planning to drop the wallet_id column in the db, and most references to wallet ids in the code. sdk assignment will be done off user ids, and anytime wallet ids are needed, they'll just be generated from a template (right now that's lbrytv-id.ID.wallet)

other changes i made:

  • dropped the Caller interface in favor of just using the real type. i get why it was necessary (for mocking the rpc client during testing), but we don't need that anymore because I ...
  • rewrote tests to use a real jsonrpc server so we don't need to mock that anymore. and by real, i mean a server running in a separate thread that's created/destroyed for each test. look at internal/test/test.go if you're interested in how that works. it works equally well for mocking http servers

This is a continuation of the work in #195

@lyoshenka lyoshenka force-pushed the sdk_selection_refactor branch from 151eb52 to d262891 Compare April 13, 2020 01:13
app/proxy/client_test.go Outdated Show resolved Hide resolved
@lbry-bot lbry-bot assigned lyoshenka and unassigned lyoshenka Apr 13, 2020
@lyoshenka lyoshenka marked this pull request as ready for review April 14, 2020 21:27
@lyoshenka lyoshenka requested a review from anbsky April 14, 2020 21:27
@lbry-bot lbry-bot assigned anbsky and unassigned lyoshenka Apr 14, 2020
Copy link
Collaborator

@anbsky anbsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this refactoring effort, many places required cleaning up. In general feels like it's more maintainable now.

app/sdkrouter/sdkrouter.go Outdated Show resolved Hide resolved
r.Use(methodTimer)

r.HandleFunc("/", Index)
r.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's just me, but I like to keep any views logic, no matter how trivial, in the handlers source file to minimize surprises in code discovery.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean surprises in code discovery?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The kind of surprise when you read through handlers.go and realize that some of the handlers logic is actually in routes.go

api/routes.go Show resolved Hide resolved
app/auth/auth.go Outdated Show resolved Hide resolved
app/auth/auth.go Outdated Show resolved Hide resolved
internal/status/status.go Show resolved Hide resolved
internal/status/status.go Show resolved Hide resolved
internal/ip/ip.go Outdated Show resolved Hide resolved
internal/ip/ip.go Show resolved Hide resolved
cmd/serve.go Show resolved Hide resolved
@lbry-bot lbry-bot assigned lyoshenka and unassigned anbsky and lyoshenka Apr 17, 2020
@anbsky
Copy link
Collaborator

anbsky commented Apr 17, 2020

Coverage decreased (-1.1%) to 63.347% though

@lbry-bot lbry-bot assigned lyoshenka and unassigned lyoshenka Apr 20, 2020
@lyoshenka
Copy link
Contributor Author

a lot of unused functions in config.go. can i delete them?

@anbsky
Copy link
Collaborator

anbsky commented Apr 20, 2020

a lot of unused functions in config.go. can i delete them?

I think they're mostly from player code so yes.

@lbry-bot lbry-bot assigned lyoshenka and unassigned lyoshenka Apr 20, 2020
@@ -37,21 +36,21 @@ func init() {
Config = GetConfig()
}

func GetConfig() *ConfigWrapper {
func GetConfig() *configWrapper {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning variables of private types from public functions is not a recommended practice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you link me to where this is not recommended? i see this in plenty of places and it makes sense to me

Copy link
Collaborator

@anbsky anbsky Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I learned this from golint and their reasoning seems compelling, namely:

can't put a *sample field in their own struct

Even in my not-so-long practice I distinctly remember being annoyed by this more than a few times.

@lyoshenka lyoshenka force-pushed the sdk_selection_refactor branch from 4d95fef to b5e8e86 Compare April 21, 2020 14:39
@lyoshenka lyoshenka force-pushed the sdk_selection_refactor branch from b5e8e86 to 601d8e2 Compare April 22, 2020 14:04
@lyoshenka lyoshenka merged commit 6a335e1 into master Apr 22, 2020
@lyoshenka lyoshenka deleted the sdk_selection_refactor branch April 23, 2020 20:39
@lbry-bot lbry-bot assigned lyoshenka and unassigned lyoshenka Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants