rocksdb #29

Merged
jeffreypicard merged 93 commits from feature/27/jeffreypicard/rocksdb into master 2022-04-29 17:04:01 +02:00
jeffreypicard commented 2021-12-07 04:31:56 +01:00 (Migrated from github.com)

closes #27, #32

closes #27, #32
lyoshenka (Migrated from github.com) requested changes 2022-01-17 15:39:15 +01:00
lyoshenka (Migrated from github.com) left a comment

i skimmed and it looks fine to me. anything in particular that you wanted me to focus on?

one nit: plz make it clear that the stuff in resources dir is actually for testing. maybe call it testdata instead?

i skimmed and it looks fine to me. anything in particular that you wanted me to focus on? one nit: plz make it clear that the stuff in `resources` dir is actually for testing. maybe call it `testdata` instead?
lyoshenka (Migrated from github.com) commented 2022-01-17 15:31:24 +01:00

what are you using git-lfs for?

what are you using git-lfs for?
@ -2,2 +2,3 @@
go 1.16
go 1.17
lyoshenka (Migrated from github.com) commented 2022-01-17 15:36:21 +01:00

if you're using lbry.go, please use the v3 branch and update your code accordingly. its the one that's most up to date and depends on the latest lbcd

if you're using lbry.go, please use the v3 branch and update your code accordingly. its the one that's most up to date and depends on the latest lbcd
lyoshenka (Migrated from github.com) reviewed 2022-01-17 15:39:46 +01:00
@ -2,2 +2,3 @@
go 1.16
go 1.17
lyoshenka (Migrated from github.com) commented 2022-01-17 15:39:45 +01:00
https://github.com/lbryio/lbry.go/tree/v3
jeffreypicard (Migrated from github.com) reviewed 2022-01-17 16:18:50 +01:00
jeffreypicard (Migrated from github.com) commented 2022-01-17 16:18:50 +01:00

Oops, I need to remove this. Originally I was planning on use git-lfs when I was going to put binary files in there for testing. After switching to human readable csvs we don't need it.

Oops, I need to remove this. Originally I was planning on use git-lfs when I was going to put binary files in there for testing. After switching to human readable csvs we don't need it.
jeffreypicard commented 2022-01-17 16:21:08 +01:00 (Migrated from github.com)

i skimmed and it looks fine to me. anything in particular that you wanted me to focus on?

one nit: plz make it clear that the stuff in resources dir is actually for testing. maybe call it testdata instead?

I'll switch it to testdata. resources is a leftover habit from when I was writing a lot of enterprise Java/Scala, JVM language repos seem to generally put non source code test data in a directory called resources

> i skimmed and it looks fine to me. anything in particular that you wanted me to focus on? > > one nit: plz make it clear that the stuff in `resources` dir is actually for testing. maybe call it `testdata` instead? I'll switch it to testdata. `resources` is a leftover habit from when I was writing a lot of enterprise Java/Scala, JVM language repos seem to generally put non source code test data in a directory called `resources`
jeffreypicard commented 2022-01-17 16:23:13 +01:00 (Migrated from github.com)

i skimmed and it looks fine to me. anything in particular that you wanted me to focus on?
one nit: plz make it clear that the stuff in resources dir is actually for testing. maybe call it testdata instead?

I'll switch it to testdata. resources is a leftover habit from when I was writing a lot of enterprise Java/Scala, JVM language repos seem to generally put non source code test data in a directory called resources

i skimmed and it looks fine to me. anything in particular that you wanted me to focus on?

I guess mostly the stuff around trying to make generic functions for the Key,Value pairs based on the type constants. I tried to remove as much duplicated code as I could, but wanted to make sure I was doing that in an efficient / idiomatic way.

> > i skimmed and it looks fine to me. anything in particular that you wanted me to focus on? > > one nit: plz make it clear that the stuff in `resources` dir is actually for testing. maybe call it `testdata` instead? > > I'll switch it to testdata. `resources` is a leftover habit from when I was writing a lot of enterprise Java/Scala, JVM language repos seem to generally put non source code test data in a directory called `resources` > i skimmed and it looks fine to me. anything in particular that you wanted me to focus on? I guess mostly the stuff around trying to make generic functions for the Key,Value pairs based on the type constants. I tried to remove as much duplicated code as I could, but wanted to make sure I was doing that in an efficient / idiomatic way.
steverusso commented 2022-02-26 17:53:22 +01:00 (Migrated from github.com)

Fwiw, the zero value for int is 0 and for pointers and slices is nil. Therefore:

  • the = nil is unnecessary in places such as here and here.
  • ints could be initialized more concisely in places such as here and here with var i int or i := 0.
Fwiw, the [zero value](https://go.dev/ref/spec#The_zero_value) for `int` is `0` and for pointers and slices is `nil`. Therefore: * the ` = nil` is unnecessary in places such as [here](https://github.com/lbryio/hub/pull/29/files#diff-2873f79a86c0d8b3335cd7731b0ecf7dd4301eb19a82ef7a1cba7589b5252261R124) and [here](https://github.com/lbryio/hub/pull/29/files#diff-30c25f2a7d8ae335d2aada4b0a6013ad9d159595bb6a2a14b1d0022451b6753fR1100-R1101). * ints could be initialized more concisely in places such as [here](https://github.com/lbryio/hub/pull/29/files#diff-a3f4b6041601807543c4326d820592ebb0d88692a4c0f3d98d70dd3600018db8R75) and [here](https://github.com/lbryio/hub/pull/29/files#diff-30c25f2a7d8ae335d2aada4b0a6013ad9d159595bb6a2a14b1d0022451b6753fR380) with `var i int` or `i := 0`.
lyoshenka (Migrated from github.com) reviewed 2022-03-24 22:55:20 +01:00
lyoshenka (Migrated from github.com) left a comment

there's no need to prefix numerical variables with n. in func(nFields int), it should just be fields int. its clear that that's a number.

please add godoc comments. you don't have to comment every single function, but most types and most functions over 10 lines should have them. at the very very least, a paragraph at the top of each package describing that package

lots of TODO and FIXME comments still there, plus chunks of commented-out code. please clean them up

there's no need to prefix numerical variables with `n`. in `func(nFields int)`, it should just be `fields int`. its clear that that's a number. please add godoc comments. you don't have to comment every single function, but most types and most functions over 10 lines should have them. at the very very least, a paragraph at the top of each package describing that package lots of TODO and FIXME comments still there, plus chunks of commented-out code. please clean them up
lyoshenka (Migrated from github.com) commented 2022-03-24 22:26:17 +01:00

no need to start these with N

no need to start these with N
lyoshenka (Migrated from github.com) commented 2022-03-24 22:26:38 +01:00

and many of these are unused

and many of these are unused
lyoshenka (Migrated from github.com) commented 2022-03-24 22:34:50 +01:00

is this really no big deal? seems like an error to me but idk rocksdb

is this really no big deal? seems like an error to me but idk rocksdb
lyoshenka (Migrated from github.com) commented 2022-03-24 22:38:01 +01:00

typo: Famlies -> Families

typo: Famlies -> Families
lyoshenka (Migrated from github.com) commented 2022-03-24 22:41:04 +01:00

this can be unexported

this can be unexported
lyoshenka (Migrated from github.com) commented 2022-03-24 22:41:39 +01:00

still need this?

still need this?
lyoshenka (Migrated from github.com) commented 2022-03-24 22:43:06 +01:00

typo

typo
lyoshenka (Migrated from github.com) commented 2022-03-24 22:43:32 +01:00

👀

:eyes:
@ -0,0 +58,4 @@
BlockedChannels map[string][]byte
FilteredStreams map[string][]byte
FilteredChannels map[string][]byte
ShutdownChan chan struct{}
lyoshenka (Migrated from github.com) commented 2022-03-24 22:40:15 +01:00

fyi this kind of stuff is usually done with sync.WaitGroup, but your way seems fine too

fyi this kind of stuff is usually done with sync.WaitGroup, but your way seems fine too
@ -0,0 +66,4 @@
type ResolveResult struct {
Name string
NormalizedName string
ClaimHash []byte
lyoshenka (Migrated from github.com) commented 2022-03-24 22:29:05 +01:00

all these hashes should be chainhash.Hash type

all these hashes should be chainhash.Hash type
@ -0,0 +287,4 @@
}
func (ps *PathSegment) Normalized() string {
return internal.NormalizeName(ps.name)
lyoshenka (Migrated from github.com) commented 2022-03-24 22:31:14 +01:00

strongly suggest using lbcd's normalize function so its consistent

strongly suggest using lbcd's normalize function so its consistent
@ -0,0 +366,4 @@
ro := grocksdb.NewDefaultReadOptions()
/*
FIXME:
lyoshenka (Migrated from github.com) commented 2022-03-24 22:35:54 +01:00

needs fixing?

needs fixing?
@ -0,0 +435,4 @@
}
// GetProdDB returns a db that is used for production.
func GetProdDB(name string, secondaryPath string) (*ReadOnlyDBColumnFamily, func(), error) {
lyoshenka (Migrated from github.com) commented 2022-03-24 22:37:31 +01:00

why return a cleanup function if its a) not used and b) on the db itself anyway?

why return a cleanup function if its a) not used and b) on the db itself anyway?
@ -0,0 +533,4 @@
db.InitHeaders()
db.InitTxCounts()
}
// TODO: assert tx_count not in self.db.tx_counts, f'boom {tx_count} in {len(self.db.tx_counts)} tx counts'
lyoshenka (Migrated from github.com) commented 2022-03-24 22:38:32 +01:00

needs doing?

needs doing?
@ -0,0 +711,4 @@
return err
}
//TODO: figure out a reasonable default and make it a constant
lyoshenka (Migrated from github.com) commented 2022-03-24 22:42:20 +01:00

👀

:eyes:
lyoshenka (Migrated from github.com) commented 2022-03-24 22:46:33 +01:00

every call of this uses the same bisect function. does it make sense to pass it in?

every call of this uses the same bisect function. does it make sense to pass it in?
lyoshenka (Migrated from github.com) commented 2022-03-24 19:47:15 +01:00

why snake case here?

why snake case here?
lyoshenka (Migrated from github.com) commented 2022-03-24 19:50:57 +01:00

if this is a fixed size, should it be an array? or even a chainhash.Hash? https://github.com/lbryio/lbcd/blob/master/chaincfg/chainhash/hash.go#L25

if this is a fixed size, should it be an array? or even a chainhash.Hash? https://github.com/lbryio/lbcd/blob/master/chaincfg/chainhash/hash.go#L25
lyoshenka (Migrated from github.com) commented 2022-03-24 19:51:47 +01:00

i suggest using lbcd types as much as possible where it makes sense. if something is a Hash, make it a Hash. if its an Address, make it an Address.

i suggest using lbcd types as much as possible where it makes sense. if something is a Hash, make it a Hash. if its an Address, make it an Address.
@ -0,0 +68,4 @@
)
// GetPrefixes returns an array of all the byte prefix constants.
func GetPrefixes() [][]byte {
lyoshenka (Migrated from github.com) commented 2022-03-24 19:46:58 +01:00

why is this [][]byte and not []byte? all the prefixes are bytes

why is this [][]byte and not []byte? all the prefixes are bytes
@ -0,0 +162,4 @@
func (v *DBStateValue) PackValue() []byte {
// b'>32sLL32sLLBBlllL'
n := 32 + 4 + 4 + 32 + 4 + 4 + 1 + 1 + 4 + 4 + 4 + 4
lyoshenka (Migrated from github.com) commented 2022-03-24 19:49:12 +01:00

all this adding of byte counts is weird. is there a better way?

all this adding of byte counts is weird. is there a better way?
lyoshenka (Migrated from github.com) commented 2022-03-24 19:49:35 +01:00

same in DBStateValueUnpack()

same in DBStateValueUnpack()
@ -0,0 +3398,4 @@
}
// generic simulates a generic key packing / unpacking function for the prefixes
func generic(voidstar interface{}, firstByte byte, function byte, functionName string) (interface{}, error) {
lyoshenka (Migrated from github.com) commented 2022-03-24 22:24:01 +01:00

you really need to explain this one

you really need to explain this one
@ -9,4 +10,3 @@
pb "github.com/lbryio/hub/protobuf/go"
"github.com/lbryio/lbry.go/v2/extras/errors"
)
lyoshenka (Migrated from github.com) commented 2022-03-24 22:54:17 +01:00

👍 to this

dont use that error package anymore. if you do want to use one, use https://github.com/cockroachdb/errors

:+1: to this dont use that error package anymore. if you do want to use one, use https://github.com/cockroachdb/errors
jeffreypicard (Migrated from github.com) reviewed 2022-03-24 23:18:40 +01:00
@ -0,0 +162,4 @@
func (v *DBStateValue) PackValue() []byte {
// b'>32sLL32sLLBBlllL'
n := 32 + 4 + 4 + 32 + 4 + 4 + 1 + 1 + 4 + 4 + 4 + 4
jeffreypicard (Migrated from github.com) commented 2022-03-24 23:18:40 +01:00

I go back and forth on this. I think this is more clear than just hard coding the sum (maybe add the sum in a comment?), the even clearer way would be to make each of those constants a variable and sum them, but that seemed excessive.

I go back and forth on this. I think this is more clear than just hard coding the sum (maybe add the sum in a comment?), the even clearer way would be to make each of those constants a variable and sum them, but that seemed excessive.
jeffreypicard (Migrated from github.com) reviewed 2022-03-24 23:22:22 +01:00
@ -0,0 +162,4 @@
func (v *DBStateValue) PackValue() []byte {
// b'>32sLL32sLLBBlllL'
n := 32 + 4 + 4 + 32 + 4 + 4 + 1 + 1 + 4 + 4 + 4 + 4
jeffreypicard (Migrated from github.com) commented 2022-03-24 23:22:22 +01:00

There's a sizeof in the "unsafe" library in go haha https://stackoverflow.com/questions/2113751/sizeof-struct-in-go but I'm not sure if that might return an implementation specific result due to padding, where here we are specifically taking the size of each individual element because we're packing them into a byte array.

There's a sizeof in the "unsafe" library in go haha https://stackoverflow.com/questions/2113751/sizeof-struct-in-go but I'm not sure if that might return an implementation specific result due to padding, where here we are specifically taking the size of each individual element because we're packing them into a byte array.
jeffreypicard (Migrated from github.com) reviewed 2022-03-24 23:26:59 +01:00
@ -0,0 +3398,4 @@
}
// generic simulates a generic key packing / unpacking function for the prefixes
func generic(voidstar interface{}, firstByte byte, function byte, functionName string) (interface{}, error) {
jeffreypicard (Migrated from github.com) commented 2022-03-24 23:26:59 +01:00

Agreed. This is where we could possibly use generics from go 1.18 btw, I'm literally faking that here.

Agreed. This is where we could possibly use generics from go 1.18 btw, I'm literally faking that here.
jeffreypicard (Migrated from github.com) reviewed 2022-03-24 23:32:48 +01:00
jeffreypicard (Migrated from github.com) commented 2022-03-24 23:32:48 +01:00

I agree the "N" can go. I copied these all over from the Python. where I believe they are similarly unused, because I thought it might be the defacto documentation.

I agree the "N" can go. I copied these all over from the Python. where I believe they are similarly unused, because I thought it might be the defacto documentation.
jeffreypicard (Migrated from github.com) reviewed 2022-03-24 23:39:47 +01:00
jeffreypicard (Migrated from github.com) commented 2022-03-24 23:39:47 +01:00
// Valid returns false only when an Iterator has iterated past either the
// first or the last key in the database.
func (iter *Iterator) Valid() bool {
	return C.rocksdb_iter_valid(iter.c) != 0
}

If our iterator was to try to go off the end of the db, or the front if we're going backwards, we need to check this.

``` // Valid returns false only when an Iterator has iterated past either the // first or the last key in the database. func (iter *Iterator) Valid() bool { return C.rocksdb_iter_valid(iter.c) != 0 } ``` If our iterator was to try to go off the end of the db, or the front if we're going backwards, we need to check this.
jeffreypicard (Migrated from github.com) reviewed 2022-04-11 18:48:20 +02:00
@ -0,0 +68,4 @@
)
// GetPrefixes returns an array of all the byte prefix constants.
func GetPrefixes() [][]byte {
jeffreypicard (Migrated from github.com) commented 2022-04-11 18:48:20 +02:00

Because these need to be strings to actually specify column family names, but bytes when being used as prefixes, so I iterate over the [][]byte and convert the []byte to string for each when opening the full db.

Because these need to be strings to actually specify column family names, but bytes when being used as prefixes, so I iterate over the [][]byte and convert the []byte to string for each when opening the full db.
jeffreypicard (Migrated from github.com) reviewed 2022-04-11 18:59:26 +02:00
@ -0,0 +435,4 @@
}
// GetProdDB returns a db that is used for production.
func GetProdDB(name string, secondaryPath string) (*ReadOnlyDBColumnFamily, func(), error) {
jeffreypicard (Migrated from github.com) commented 2022-04-11 18:59:26 +02:00

There's a few unit tests for the db that still use the cleanup function in this way, which is how it was originally, because I'm testing pieces of the db without fully building it and using it through the function interfaces and everything.

There's a few unit tests for the db that still use the cleanup function in this way, which is how it was originally, because I'm testing pieces of the db without fully building it and using it through the function interfaces and everything.
jeffreypicard (Migrated from github.com) reviewed 2022-04-11 19:00:45 +02:00
@ -0,0 +533,4 @@
db.InitHeaders()
db.InitTxCounts()
}
// TODO: assert tx_count not in self.db.tx_counts, f'boom {tx_count} in {len(self.db.tx_counts)} tx counts'
jeffreypicard (Migrated from github.com) commented 2022-04-11 19:00:45 +02:00

No, this is an extra check that was in the python that isn't actually needed. I removed the comment.

No, this is an extra check that was in the python that isn't actually needed. I removed the comment.
jeffreypicard (Migrated from github.com) reviewed 2022-04-11 19:01:08 +02:00
jeffreypicard (Migrated from github.com) commented 2022-04-11 19:01:08 +02:00

agreed, changed.

agreed, changed.
lyoshenka (Migrated from github.com) reviewed 2022-04-15 16:15:42 +02:00
@ -0,0 +19,4 @@
// GetExpirationHeightFull returns the expiration height for the given height.
// Takes boolean to indicated whether to use extended or original expiration time.
func GetExpirationHeightFull(lastUpdatedHeight uint32, extended bool) uint32 {
lyoshenka (Migrated from github.com) commented 2022-04-15 15:59:31 +02:00

unclear what the diff between this and previous function is. whats a full expiration height?

unclear what the diff between this and previous function is. whats a full expiration height?
@ -0,0 +18,4 @@
)
// PrepareResolveResult prepares a ResolveResult to return
func PrepareResolveResult(
lyoshenka (Migrated from github.com) commented 2022-04-15 16:03:17 +02:00

should this function be exported? the name sounds like an internal function. in fact can you do a once-over on all functions to check what should and should not be exported? it may help to generate godocs and read them to see if they make sense as APIs for each thing

should this function be exported? the name sounds like an internal function. in fact can you do a once-over on all functions to check what should and should not be exported? it may help to generate godocs and read them to see if they make sense as APIs for each thing
lyoshenka (Migrated from github.com) commented 2022-04-15 16:03:30 +02:00

i requested docs from https://pkg.go.dev/github.com/lbryio/hub . might show up later

i requested docs from https://pkg.go.dev/github.com/lbryio/hub . might show up later
lyoshenka (Migrated from github.com) commented 2022-04-15 16:04:36 +02:00

this is already a subpackage of db. it should be called stack, not db_stack

this is already a subpackage of `db`. it should be called `stack`, not `db_stack`
lyoshenka (Migrated from github.com) commented 2022-04-15 16:05:18 +02:00

assuming this package is named stack, this should just be SliceBacked. no need to put stack in the name again

assuming this package is named `stack`, this should just be `SliceBacked`. no need to put stack in the name again
lyoshenka (Migrated from github.com) commented 2022-04-15 16:06:27 +02:00

should this be named iterator.go or iteroptions.go? it only has options stuff in it

should this be named iterator.go or iteroptions.go? it only has options stuff in it
lyoshenka (Migrated from github.com) commented 2022-04-15 16:06:54 +02:00

or maybe just options.go

or maybe just options.go
lyoshenka (Migrated from github.com) commented 2022-04-15 16:08:15 +02:00

you're using v2 and v3? what are you using from v2?

you're using v2 and v3? what are you using from v2?
lyoshenka (Migrated from github.com) commented 2022-04-15 16:08:40 +02:00

also have you run go mod tidy recently?

also have you run `go mod tidy` recently?
lyoshenka (Migrated from github.com) commented 2022-04-15 16:09:25 +02:00

seems like search is a better name than sort

seems like `search` is a better name than `sort`
@ -0,0 +3,4 @@
// internal types that need their own file to avoid circular imports.
// HeightHash struct for the height subscription endpoint.
type HeightHash struct {
lyoshenka (Migrated from github.com) commented 2022-04-15 16:10:17 +02:00

docs plz

docs plz
@ -7,2 +6,4 @@
"time"
_ "net/http/pprof"
lyoshenka (Migrated from github.com) commented 2022-04-15 16:10:08 +02:00

prolly here just for profiling, right?

prolly here just for profiling, right?
@ -0,0 +1,5 @@
#!/bin/bash
./protobuf/build.sh
lyoshenka (Migrated from github.com) commented 2022-04-15 16:12:22 +02:00

do you see these changing often? id be careful cause diff versions of protoc could introduce cosmetic changes with big diffs that dont do much. i suggest that building protobufs should be a separate optional build step just for when they change

but if they change often, then its fine to leave it here

do you see these changing often? id be careful cause diff versions of `protoc` could introduce cosmetic changes with big diffs that dont do much. i suggest that building protobufs should be a separate optional build step just for when they change but if they change often, then its fine to leave it here
@ -12,46 +12,66 @@ import (
const (
ServeCmd = iota
SearchCmd = iota
DBCmd = iota
lyoshenka (Migrated from github.com) commented 2022-04-15 16:13:16 +02:00

what are cmd2 and cmd3? what do they mean?

what are cmd2 and cmd3? what do they mean?
lyoshenka (Migrated from github.com) commented 2022-04-15 16:14:38 +02:00

is it? maybe a separate function? this ones long as it is

is it? maybe a separate function? this ones long as it is
jeffreypicard (Migrated from github.com) reviewed 2022-04-15 16:28:07 +02:00
@ -0,0 +19,4 @@
// GetExpirationHeightFull returns the expiration height for the given height.
// Takes boolean to indicated whether to use extended or original expiration time.
func GetExpirationHeightFull(lastUpdatedHeight uint32, extended bool) uint32 {
jeffreypicard (Migrated from github.com) commented 2022-04-15 16:28:06 +02:00

That's just an internal distinction because it takes the optional boolean to use the extended expiration time. I added a comment.

That's just an internal distinction because it takes the optional boolean to use the extended expiration time. I added a comment.
jeffreypicard (Migrated from github.com) reviewed 2022-04-15 16:42:54 +02:00
@ -12,46 +12,66 @@ import (
const (
ServeCmd = iota
SearchCmd = iota
DBCmd = iota
jeffreypicard (Migrated from github.com) commented 2022-04-15 16:42:54 +02:00

These were things I added for testing, I removed them from the main function but missed removing them here. I'll get rid of them.

These were things I added for testing, I removed them from the main function but missed removing them here. I'll get rid of them.
jeffreypicard (Migrated from github.com) reviewed 2022-04-20 15:55:49 +02:00
@ -7,2 +6,4 @@
"time"
_ "net/http/pprof"
jeffreypicard (Migrated from github.com) commented 2022-04-20 15:55:49 +02:00

yeah

yeah
jeffreypicard (Migrated from github.com) reviewed 2022-04-29 16:46:52 +02:00
jeffreypicard (Migrated from github.com) commented 2022-04-29 16:46:52 +02:00

One straggler, just removed it.

One straggler, just removed it.
jeffreypicard (Migrated from github.com) reviewed 2022-04-29 16:49:51 +02:00
jeffreypicard (Migrated from github.com) commented 2022-04-29 16:49:51 +02:00

I'd normally agree, function I'm thinly wrapping comes straight from Go's "sort" package, so I thought this might make more sense.

I'd normally agree, function I'm thinly wrapping comes straight from Go's "sort" package, so I thought this might make more sense.
Sign in to join this conversation.
No reviewers
No milestone
No project
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#29
No description provided.