[dbnode] Skip entries for unowned shards in commit log bootstrapper

[justinjc]

What this PR does / why we need it:

Fixes #2129

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

[robskillington]

Hm, believe the naive case of this was fixed with PR #2052.

Although I guess this could be plausible if a bootstrap is already happening and then a further topology change happens, therefore the current bootstrap is bootstrapping data for shards that was valid at beginning of bootstrap but is invalidated by a consequent topology change.

If so, this is a good catch! And unfortunate I introduced it for this upcoming release (in current release candidate) =[

If that's what we're protecting against here, this can also happen in the peer bootstrapper which also uses accumulator.CheckoutSeries...(...) interface.

Maybe we should introduce a third return to the CheckoutSeries...(...) methods that causes the caller to deal with the series no longer being owned.

i.e.

	CheckoutSeriesWithoutLock(
		shardID uint32,
		id ident.ID,
		tags ident.TagIterator,
	) (result CheckoutSeriesResult, owned bool, err error)

	CheckoutSeriesWithLock(
		shardID uint32,
		id ident.ID,
		tags ident.TagIterator,
	) (result CheckoutSeriesResult, owned bool, err error)

This should force all existing call sites to be updated (like the peers bootstrapper call site which I don't believe is updated with this change) and any new ones to need to consider the case when the series is no longer owned.

I'm thinking maybe something like this:

result, owned, err := accumulator.CheckoutSeriesWithoutLock(...)
if err != nil {
 // .. normal err case
}

if !owned {
  // deal with the series not being owned
}

// otherwise use result

Thoughts?

[justinjc]

@robskillington yeah that makes sense. Unfortunately we did hit this issue during multiple topology changes that happened in relatively quick succession.

Your proposed interface changes sound good - let me do some refactoring.

[codecov]

Codecov Report

Merging #2145 into master will increase coverage by 7.0%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #2145      +/-   ##
=========================================
+ Coverage    65.2%   72.3%    +7.0%     
=========================================
  Files         991    1016      +25     
  Lines      102460   87969   -14491     
=========================================
- Hits        66885   63654    -3231     
+ Misses      30735   20047   -10688     
+ Partials     4840    4268     -572     

Flag	Coverage Δ
#aggregator	82.0% <ø> (+0.7%)	⬆️
#cluster	85.3% <ø> (+7.6%)	⬆️
#collector	82.8% <ø> (+31.9%)	⬆️
#dbnode	79.2% <82.2%> (+7.1%)	⬆️
#m3em	74.4% <ø> (+29.5%)	⬆️
#m3ninx	74.2% <ø> (+7.0%)	⬆️
#m3nsch	51.1% <ø> (-48.9%)	⬇️
#metrics	17.6% <ø> (-82.4%)	⬇️
#msg	74.8% <ø> (+0.4%)	⬆️
#query	68.3% <ø> (-31.7%)	⬇️
#x	83.1% <ø> (-16.9%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cd86c7...b222f59. Read the comment docs.

[robskillington]

LGTM, I changed the commit log source bootstrapper code a tiny bit to be more explicit rather than assume any series with a namespace not set needs to be skipped (would rather that panics as should be since unexpected for that to happen and would be a new code bug introduced)