[DBnode] Add Load() API to shard #1831
Conversation
richardartoul
changed the title
src/dbnode/storage/shard.go
Outdated
| } | ||
|
|
||
| func (s *dbShard) Load( | ||
| series *result.Map, |
There was a problem hiding this comment.
Hm, wouldn't it make sense to do this in chunks instead of all at once (so we can move to streaming bootstrap?)
There was a problem hiding this comment.
Yeah I may change the interface here slightly once I start hooking it up to the repair API, but I think this is fine for now. The map can be reset / re-used.
There was a problem hiding this comment.
Sure thing, can revisit later.
| blockStatesSnapshot, bootstrapped := blockStates.Snapshot() | ||
| // Only use block state snapshot information to make eviction decisions if the block state | ||
| // has been properly bootstrapped already. | ||
| if bootstrapped { |
There was a problem hiding this comment.
Wondering why adding this check now is important, it should've always been an issue no?
There was a problem hiding this comment.
it basically "happened" to work before because an empty blockStateSnapshot will have WarmRetrievable false and ColdVersion 0 so you would never evict anything. I noticed this while I was working on the P.R and thought it was really dangerous since basically you come in and think you're programming against data that has been properly bootstrapped but actually in some cases you're (temporarily) running against a zero value so I wanted to make it more explicit so it doesn't get messed up in the future
|
@richardartoul the checks should only be necessary if we start ticking before a bootstrap I assume? (which we should be doing TBH, but that seems disconnected from this PR and change?) |
|
@robskillington We already tick during bootstrap so the guards are actually really important. Right now everything happens to work because the logic happens to be correct for the zero values of the state, but I think enforcing a distinction between bootstrapped and unbootstrapped state is a good idea. Agree that it is a little weird for it to be in this P.R but I realized how easy it would have been for me to mess my P.R up because of the bootstrapping not being explicit so figured I'd add all the guard rails while I'm in here |
| bootstrap bool, | ||
| ) (dbShardBootstrapResult, error) { | ||
| if series == nil { | ||
| return dbShardBootstrapResult{}, nil |
There was a problem hiding this comment.
Hm I'd be in preference of making this an error case, you ok with that?
There was a problem hiding this comment.
The reason it behaves this way is because its very common to call series.Bootstrap(nil) but I could move it so Bootstrap(nil) is fine and Load(nil) is an error
There was a problem hiding this comment.
Could we do that? Make bootstrap(nil) valid but Load(nil) not valid?
There was a problem hiding this comment.
@robskillington yeah thats what I implemented
src/dbnode/storage/shard.go
Outdated
| } | ||
| shardBootstrapResult.update(bsResult) | ||
| } else { | ||
| entry.Series.Load(dbBlocks.Blocks, blockStatesSnapshot) |
There was a problem hiding this comment.
Suggestion: instead of having two separate APIs at the series package, maybe just a single API Load(...) with LoadOptions struct { Bootstrap bool; } and a return LoadResult{BootstrapResult: bsResult} from the method.
Thoughts?
There was a problem hiding this comment.
I really don't like using booleans like that in public APis, much prefer to have two separate APIs and keep boolean switches like that in internal methods personally (I understand it looks like API bloat but having an optional bool is basically the same API bloat but just in a round about way)
Codecov Report
@@ Coverage Diff @@
## master #1831 +/- ##
=========================================
- Coverage 62.9% 51.9% -11.1%
=========================================
Files 607 326 -281
Lines 50965 24024 -26941
=========================================
- Hits 32081 12473 -19608
+ Misses 16690 10681 -6009
+ Partials 2194 870 -1324
Continue to review full report at Codecov.
|
src/dbnode/storage/series/types.go
Outdated
|
|
||
| // Snapshot returns a BootstrappedBlockStateSnapshot and a boolean indicating whether the | ||
| // snapshot is bootstrapped or not. | ||
| func (s *ShardBlockStateSnapshot) Snapshot() (BootstrappedBlockStateSnapshot, bool) { |
There was a problem hiding this comment.
Suggestion: Can we rename this method something like Value() or State() or similar? I assumed looking at call sites that this was acquiring a lock or snapshotting some state, rather than just returning the immutable field value.
richardartoul
force-pushed
the
ra/shard-load
branch
from
3058dc0
to
6e2c944
Compare
Add an API for loading data into the shard at any time (not just during bootstrap) to support background repairs and add additional safe guards around using the shard flushState data before it has been bootstrapped.