Expose merkledb defaults #3748
Expose merkledb defaults #3748
Conversation
joshua-kim
force-pushed
the
merkle-db-config
branch
2 times, most recently
from
9545412 to
daab96b
Compare
| Prefetcher | ||
| } | ||
|
|
||
| func NewConfig() Config { |
There was a problem hiding this comment.
It feels a bit odd to default to populating mockMetrics - I'm wondering if the newDefaultConfig needs to exist at all or if we should just use this. (and populate the metrics reg here)..
Although that feels a bit odd as well... so 🤷 maybe the issue is with the mockMetrics as a whole...
It feels like whoever made mockMetrics didn't know that "github.com/prometheus/client_golang/prometheus/testutil" existed.
joshua-kim
force-pushed
the
merkle-db-config
branch
3 times, most recently
from
8955b58 to
a4d93f3
Compare
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
joshua-kim
force-pushed
the
merkle-db-config
branch
from
a4d93f3 to
9c49042
Compare
| numKeyValues, | ||
| deletePortion, | ||
| config.HistoryLength/2, | ||
| 0.25, |
There was a problem hiding this comment.
(No action required) Maybe continue to use a named var for this magic number so that it can be self-documenting?
There was a problem hiding this comment.
I just moved it into the function signature because the function signature self-documents what these variables mean
| maxProofLen = 100 | ||
| maxPastRoots = defaultHistoryLength | ||
| ) | ||
| maxProofLen := 100 |
There was a problem hiding this comment.
(No action required) Are you sure you wouldn't rather satisfy the gods of var()? ;)
There was a problem hiding this comment.
I don't know when we're supposed to use var blocks... it seems to make sense for package-level vars, when you want a short-hand for the zero value, or where you want to choose the type of something, but when those criteria aren't met I usually don't use them (and for zero values I usually explicitly assign to the zero value...).
Why this should be merged
When we merkle-ize the P and X Chains, it's likely that we're going to use similar merkledb configurations so this PR exposes defaults to avoid some boilerplate setup code.
How this works
Exposes
NewConfigthat contains aConfigwith default valuesHow this was tested
CI
Need to be documented in RELEASES.md?
No