-
Notifications
You must be signed in to change notification settings - Fork 455
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
[dbnode] Fix bootstrapping data for an unowned shard with commit log #2052
[dbnode] Fix bootstrapping data for an unowned shard with commit log #2052
Conversation
@@ -476,6 +478,13 @@ func (s *commitLogSource) Read( | |||
datapointsSkippedNotBootstrappingNamespace++ | |||
continue | |||
} | |||
// If not bootstrapping shard for this series then also skip. |
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.
nit for future work: maybe good to have a short explanation here describing how this state can occur
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.
Sounds good, will do.
fmt.Errorf("result missing for namespace: %v", nsID.String()) | ||
|
||
var result NamespaceResult | ||
if err == nil { |
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.
nit for future: this may read cleaner if you do the error logging seperately?
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.
approved with nits for future work
Codecov Report
@@ Coverage Diff @@
## master #2052 +/- ##
==========================================
- Coverage 71.2% 67% -4.2%
==========================================
Files 1011 1011
Lines 87030 110267 +23237
==========================================
+ Hits 61980 73981 +12001
- Misses 20867 31714 +10847
- Partials 4183 4572 +389
Continue to review full report at Codecov.
|
What this PR does / why we need it:
Fixes issue #2051 on master with the latest bootstrapping changes, the commit log bootstrapper was attempting to bootstrap data from shards it no longer owned.
Followup change with an integration test to catch this case will follow.
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: