LBRYCommunity/reflector.go

Fork 0

Insert in tx #51

Closed

lyoshenka wants to merge 52 commits from insert_in_tx into master

lyoshenka commented

2021-05-20 23:17:21 +02:00

(Migrated from github.com)

my version of @shyba's #50. see which you like better (or maybe you'll have an even better idea?)

wrap blob insertion in tx. fixes lbryio/lbry-sdk#3296

The problem is that inserting an sd blob with ~5k
blobs takes longer than 30 seconds. So the client
times out and retries the request. At that point,
reflector is not done inserting so it replies with
a smaller number of blobs than it should. The client
uploads that many blobs and marks the stream as
reflected. The remaining blobs never get uploaded.

Victor says doing the insert inside a transaction should be
faster than doing 10k (2 per blob) inserts
independently.

He's also doing a client-side fix for this.

my version of @shyba's #50. see which you like better (or maybe you'll have an even better idea?) wrap blob insertion in tx. fixes lbryio/lbry-sdk#3296 The problem is that inserting an sd blob with ~5k blobs takes longer than 30 seconds. So the client times out and retries the request. At that point, reflector is not done inserting so it replies with a smaller number of blobs than it should. The client uploads that many blobs and marks the stream as reflected. The remaining blobs never get uploaded. Victor says doing the insert inside a transaction should be faster than doing 10k (2 per blob) inserts independently. He's also doing a client-side fix for this.

nikooo777 (Migrated from github.com) reviewed 2021-05-20 23:17:21 +02:00

shyba commented

2021-05-21 00:06:56 +02:00

(Migrated from github.com)

client side fix at https://github.com/lbryio/lbry-sdk/pull/3308

if we can make that faster it should also help not having incomplete data on shutdown.
I had a similar issue on the SDK some years ago and using a transaction was absurdly faster, but I don't know how much that replicates to MySQL

client side fix at https://github.com/lbryio/lbry-sdk/pull/3308 if we can make that faster it should also help not having incomplete data on shutdown. I had a similar issue on the SDK some years ago and using a transaction was absurdly faster, but I don't know how much that replicates to MySQL

anbsky (Migrated from github.com) reviewed 2021-05-21 11:25:02 +02:00

db/ifaces.go

anbsky (Migrated from github.com) commented

2021-05-21 11:25:02 +02:00

why not declare it at the outer level?

anbsky (Migrated from github.com) reviewed 2021-05-21 11:27:00 +02:00

db/ifaces.go

anbsky (Migrated from github.com) commented

2021-05-21 11:27:00 +02:00

Would not name this ifaces... saving four bytes over interfaces isn't worth the cognitive load.

Would not name this `ifaces`... saving four bytes over `interfaces` isn't worth the cognitive load.

anbsky (Migrated from github.com) reviewed 2021-05-21 11:34:07 +02:00

db/db.go

anbsky (Migrated from github.com) commented

2021-05-21 11:33:40 +02:00

What's the benefit of just logging and re-panicking here and below? I would just return a wrapped error, i.e.

import "github.com/lbryio/lbry.go/v2/extras/errors"
...
return errors.Prefix("panic while rolling back a transaction", err)

What's the benefit of just logging and re-panicking here and below? I would just return a wrapped error, i.e. ``` import "github.com/lbryio/lbry.go/v2/extras/errors" ... return errors.Prefix("panic while rolling back a transaction", err) ```

anbsky commented

2021-05-21 11:35:30 +02:00

(Migrated from github.com)

I'm in favor of this implementation. Aside from a few nitpicks, I don't have any qualms with it.

lyoshenka (Migrated from github.com) reviewed 2021-05-21 20:27:22 +02:00

db/ifaces.go

lyoshenka (Migrated from github.com) commented

2021-05-21 20:27:22 +02:00

because its not meant for use outside that function. but i could move it if you want

lyoshenka (Migrated from github.com) reviewed 2021-05-21 20:28:38 +02:00

db/db.go

lyoshenka (Migrated from github.com) commented

2021-05-21 20:28:38 +02:00

👍

:+1:

anbsky (Migrated from github.com) reviewed 2021-05-22 17:00:00 +02:00

db/ifaces.go

anbsky (Migrated from github.com) commented

2021-05-22 17:00:00 +02:00

Not sure I can explain it but it just feels dirty to have an inline type definition, plus this one had an uppercase name so felt like a typo.

lyoshenka (Migrated from github.com) reviewed 2021-05-24 16:46:56 +02:00

db/ifaces.go

lyoshenka (Migrated from github.com) commented

2021-05-24 16:46:55 +02:00

made them lowercase

it might feel dirty because Python is OO and interfaces are always declared alongside objects and explicitly implemented by those objects. in Go, otoh, types can implement interfaces that they know nothing about. if i define a struct and it has a Read(p []byte) (n int, err error) method, then it implements io.Reader. it does not need to know that io.Reader exists. In fact I could define a grin.StoryReader interface in a completely unrelated project and my struct will implement that too if the methods match.

does that make it feel better?

ref: https://golang.org/ref/spec#Interface_types

made them lowercase it might feel dirty because Python is OO and interfaces are always declared alongside objects and explicitly implemented by those objects. in Go, otoh, types can implement interfaces that they know nothing about. if i define a struct and it has a `Read(p []byte) (n int, err error)` method, then it implements `io.Reader`. it does not need to know that `io.Reader` exists. In fact I could define a `grin.StoryReader` interface in a completely unrelated project and my struct will implement that too if the methods match. does that make it feel better? ref: https://golang.org/ref/spec#Interface_types

anbsky (Migrated from github.com) reviewed 2021-05-24 18:23:33 +02:00

db/ifaces.go

anbsky (Migrated from github.com) commented

2021-05-24 18:23:32 +02:00

Interfaces are too bleeding edge in Python for me to be familiar with them, from what I gather they've only been added last year with protocols PEP.

My general feeling instead was that meta-objects aka types are better declared namespace-wide (= package-wide in case of golang) unless we're dealing with tests.

Is it actually not possible to declare this as func Begin(db beginner) (Transactor, error)? We seem to be returning Transactor regardless.

Interfaces are too bleeding edge in Python for me to be familiar with them, from what I gather they've only been added last year with protocols PEP. My general feeling instead was that meta-objects aka types are better declared namespace-wide (= package-wide in case of golang) unless we're dealing with tests. Is it actually not possible to declare this as `func Begin(db beginner) (Transactor, error)`? We seem to be returning `Transactor` regardless.