-
Notifications
You must be signed in to change notification settings - Fork 43
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
refactor: Strongly typed key refactor (#17) #84
Conversation
paramaterize ns/suffixLines 30 to 35 in 687c223
This comment was generated by todo based on a
|
Support multiple spansLines 114 to 117 in 687c223
This comment was generated by todo based on a
|
Config logger param package wideLines 42 to 45 in 687c223
This comment was generated by todo based on a
|
687c223
to
fe8158c
Compare
Codecov Report
@@ Coverage Diff @@
## develop #84 +/- ##
===========================================
+ Coverage 57.47% 58.07% +0.60%
===========================================
Files 103 103
Lines 10045 10206 +161
===========================================
+ Hits 5773 5927 +154
- Misses 3641 3645 +4
- Partials 631 634 +3
|
abeb2d4
to
9c029cc
Compare
Locally run benchmarks against develop:
|
Locally run benchmarks against this branch:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite the change, I know I reviewed it back when you first opened this, but still takes a minute to fully wrap my head around the hierarchy/structure.
In general it all looks good, except the relation between key.DocKey
and core.DataStoreKey
, made some comments about it on the code. Basically we've created weird dependancy resulting in dockey.Key.DocKey
access patterns. I think key.DocKey
should be relatively self contained.
ds.Key
was embedded within a key.DocKey
before just so we could easily use all the ds.Key
methods, but since we are using a structured approach, we dont really need the general purpose methods of the ds.Key
.
You should just see the ds.Key
in the key.DocKey
as a string
type with some helper functions.
9c029cc
to
d6a3780
Compare
5de70cb
to
e31998a
Compare
c2351e2
to
821203f
Compare
821203f
to
5dc940d
Compare
WithPriorityFlag is called within reg.getPriority and should not be the responsibility of the caller
Is not needed and complicates the code
WithValueFlag is called in writeObjectMarker
Is wasteful taking the whole struct (and initing one in most cases)
5dc940d
to
9744349
Compare
I'm not 100% sure what this button does, but I ran the benches again and numbers are comparable with current dev
Closes #17 Closes #18
Updated review of the original PR (below), against develop branch
#19
Suggest re-reviewing properly once we know how this affects performance, it got quite stale and it is possible that I have added in some strangeness in places with the time-off and/or rebase onto latest develop
Also updates the composite dag cids to use the dag-pb codec to make the consistent with the field cids
Benchmarks have been run, suggest little-to-no change.