Skip to content
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 peers source to not emit error logs when it should not #948

Merged
merged 2 commits into from
Sep 27, 2018

Conversation

richardartoul
Copy link
Contributor

@richardartoul richardartoul commented Sep 26, 2018

A recent change to the peers bootstrapper source caused it to begin emitting error logs when it should not. The reason it was emitting errors was because normally when we perform an incremental flush, we then remove all the series and blocks from the bootstrap result so that they don't get loaded into memory for no reason (increasing memory pressure).

Now that we've added the concept of a "snapshot" flush, the peers bootstrapper source needs to be aware that it should not remove series/data from memory when its doing an incremental flush that is of type snapshot, because the node is still expecting all that data to be returned for the active block. The existing code detected that the series that it was trying to remove still had data blocks in them, so it emitted an error log instead of removing them.

Correct behavior of this functionality is already tested in a combination of unit and integration tests, and all the tests passed because the defensive code made the bootstrapper do the right thing anyways, but emit an error log.

This P.R refactors the code so that the error logs will not be emitted when they should not be, upgrades the existing error messages to InvariantViolated, and leaves the existing tests intact.

I also went ahead and removed some dead code that was left behind from a previous migration and is no longer required.

@codecov
Copy link

codecov bot commented Sep 26, 2018

Codecov Report

Merging #948 into master will decrease coverage by 0.01%.
The diff coverage is 36.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #948      +/-   ##
==========================================
- Coverage   77.95%   77.93%   -0.02%     
==========================================
  Files         410      410              
  Lines       34401    34399       -2     
==========================================
- Hits        26816    26810       -6     
- Misses       5741     5744       +3     
- Partials     1844     1845       +1
Flag Coverage Δ
#dbnode 81.5% <36.36%> (-0.01%) ⬇️
#m3ninx 75.25% <ø> (-0.08%) ⬇️
#query 64.32% <ø> (ø) ⬆️
#x 80.55% <ø> (ø) ⬆️

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 87a0ec3...ef9e684. Read the comment docs.

Copy link
Collaborator

@prateek prateek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once build is good

@richardartoul richardartoul merged commit b8ea701 into master Sep 27, 2018
@prateek prateek deleted the ra/peers-errors branch September 29, 2018 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants