rocksdb #29
No reviewers
Labels
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
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: LBRYCommunity/herald.go#29
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/27/jeffreypicard/rocksdb"
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?
closes #27, #32
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 ittestdata
instead?what are you using git-lfs for?
@ -2,2 +2,3 @@
go 1.16
go 1.17
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
@ -2,2 +2,3 @@
go 1.16
go 1.17
https://github.com/lbryio/lbry.go/tree/v3
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.
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 calledresources
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.
Fwiw, the zero value for
int
is0
and for pointers and slices isnil
. Therefore:= nil
is unnecessary in places such as here and here.var i int
ori := 0
.there's no need to prefix numerical variables with
n
. infunc(nFields int)
, it should just befields 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
no need to start these with N
and many of these are unused
is this really no big deal? seems like an error to me but idk rocksdb
typo: Famlies -> Families
this can be unexported
still need this?
typo
👀
@ -0,0 +58,4 @@
BlockedChannels map[string][]byte
FilteredStreams map[string][]byte
FilteredChannels map[string][]byte
ShutdownChan chan struct{}
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
all these hashes should be chainhash.Hash type
@ -0,0 +287,4 @@
}
func (ps *PathSegment) Normalized() string {
return internal.NormalizeName(ps.name)
strongly suggest using lbcd's normalize function so its consistent
@ -0,0 +366,4 @@
ro := grocksdb.NewDefaultReadOptions()
/*
FIXME:
needs fixing?
@ -0,0 +435,4 @@
}
// GetProdDB returns a db that is used for production.
func GetProdDB(name string, secondaryPath string) (*ReadOnlyDBColumnFamily, func(), error) {
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'
needs doing?
@ -0,0 +711,4 @@
return err
}
//TODO: figure out a reasonable default and make it a constant
👀
every call of this uses the same bisect function. does it make sense to pass it in?
why snake case here?
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
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 {
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
all this adding of byte counts is weird. is there a better way?
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) {
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"
)
👍 to this
dont use that error package anymore. if you do want to use one, use https://github.com/cockroachdb/errors
@ -0,0 +162,4 @@
func (v *DBStateValue) PackValue() []byte {
// b'>32sLL32sLLBBlllL'
n := 32 + 4 + 4 + 32 + 4 + 4 + 1 + 1 + 4 + 4 + 4 + 4
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.
@ -0,0 +162,4 @@
func (v *DBStateValue) PackValue() []byte {
// b'>32sLL32sLLBBlllL'
n := 32 + 4 + 4 + 32 + 4 + 4 + 1 + 1 + 4 + 4 + 4 + 4
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.
@ -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) {
Agreed. This is where we could possibly use generics from go 1.18 btw, I'm literally faking that here.
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.
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.
@ -0,0 +68,4 @@
)
// GetPrefixes returns an array of all the byte prefix constants.
func GetPrefixes() [][]byte {
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.
@ -0,0 +435,4 @@
}
// GetProdDB returns a db that is used for production.
func GetProdDB(name string, secondaryPath string) (*ReadOnlyDBColumnFamily, func(), error) {
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.
@ -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'
No, this is an extra check that was in the python that isn't actually needed. I removed the comment.
agreed, changed.
@ -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 {
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(
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
i requested docs from https://pkg.go.dev/github.com/lbryio/hub . might show up later
this is already a subpackage of
db
. it should be calledstack
, notdb_stack
assuming this package is named
stack
, this should just beSliceBacked
. no need to put stack in the name againshould this be named iterator.go or iteroptions.go? it only has options stuff in it
or maybe just options.go
you're using v2 and v3? what are you using from v2?
also have you run
go mod tidy
recently?seems like
search
is a better name thansort
@ -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 {
docs plz
@ -7,2 +6,4 @@
"time"
_ "net/http/pprof"
prolly here just for profiling, right?
@ -0,0 +1,5 @@
#!/bin/bash
./protobuf/build.sh
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 changebut if they change often, then its fine to leave it here
@ -12,46 +12,66 @@ import (
const (
ServeCmd = iota
SearchCmd = iota
DBCmd = iota
what are cmd2 and cmd3? what do they mean?
is it? maybe a separate function? this ones long as it is
@ -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 {
That's just an internal distinction because it takes the optional boolean to use the extended expiration time. I added a comment.
@ -12,46 +12,66 @@ import (
const (
ServeCmd = iota
SearchCmd = iota
DBCmd = iota
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.
@ -7,2 +6,4 @@
"time"
_ "net/http/pprof"
yeah
One straggler, just removed it.
I'd normally agree, function I'm thinly wrapping comes straight from Go's "sort" package, so I thought this might make more sense.