Skip to content

bucket verify: repair out of order labels #964

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.

Sign up for GitHub

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

Merged
bwplotka merged 9 commits into from
Apr 18, 2019
Merged

bucket verify: repair out of order labels #964

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f0dfe27
bucket verify: repair out of order labels
jjneely Mar 22, 2019
f09d3f5
verify repair: correctly order series in the index on rewrite
jjneely Mar 25, 2019
230bc7b
Fix the TSDB block safe-delete function
jjneely Mar 28, 2019
3f0d6cd
verify repair: fix duplicate chunk detection
jjneely Apr 1, 2019
ed78bde
PR feedback: use errors.Errorf() instead of fmt.Errorf()
jjneely Apr 12, 2019
2f179ff
Use errors.New()
jjneely Apr 15, 2019
24173d3
Liberally comment this for loop
jjneely Apr 15, 2019
0d39253
Take advantage of sort.Interface
jjneely Apr 17, 2019
bfd44f1
PR Feedback: Comments are full sentences.
jjneely Apr 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Liberally comment this for loop 
We're comparing items by pointers, using Go's range variables is
misleading here and we need not fall into the same trap.
  • Loading branch information
@jjneely
jjneely committed Apr 15, 2019
commit 24173d373f851c5cd195f6aa0b20483353c88180
5 changes: 5 additions & 0 deletions pkg/block/index.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,11 @@ func sanitizeChunkSequence(chks []chunks.Meta, mint int64, maxt int64, ignoreChk
var last *chunks.Meta

OUTER:
// This compares the current chunk to the chunk from the last iteration
// by pointers. If we use "i, c := range cks" the variable c is a new
Copy link
Member

@bwplotka bwplotka Apr 18, 2019

Choose a reason for hiding this comment

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

Suggested change
// by pointers. If we use "i, c := range cks" the variable c is a new
// by pointers. If we use "i, c := range chks" the variable c is a new

Copy link
Contributor Author

@jjneely jjneely Apr 18, 2019

Choose a reason for hiding this comment

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

Fixed

// variable who's address doesn't change through the entire loop.
// The current element of the chks slice is copied into it. We must take
Copy link
Member

@bwplotka bwplotka Apr 18, 2019

Choose a reason for hiding this comment

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

Suggested change
// The current element of the chks slice is copied into it. We must take
// The current element of the chks slice is copied into it. We must take

Copy link
Contributor Author

@jjneely jjneely Apr 18, 2019

Choose a reason for hiding this comment

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

Fixed.

// the address of the indexed slice instead.
for i := range chks {
Copy link
Member

@povilasv povilasv Apr 11, 2019

Choose a reason for hiding this comment

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

why the change?

Copy link
Contributor Author

@jjneely jjneely Apr 12, 2019

Choose a reason for hiding this comment

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

Very subtle. So we are remembering a pointer to the chunk c from the last iteration to compare it against the chunk in the current iteration. However, when we use for index, value := range slice the value is not a pointer into the slice. In fact its a new variable the current item of the slice is copied into. Which means our pointer based comparisons are broken -- they always compare the current chunk to itself as the address of the variable c doesn't change throughout the loop.

Using just a slice index here allows us to correctly store a pointer to the item of the slice from the last iteration and compare that to the chunk in the current iteration. Otherwise, this code was removing all chunks in the series other than the first one.

Copy link
Member

@brancz brancz Apr 15, 2019

Choose a reason for hiding this comment

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

Let's document this :)

Copy link
Contributor Author

@jjneely jjneely Apr 15, 2019

Choose a reason for hiding this comment

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

Where is the right place to do so? Glad to do it.

Copy link
Contributor Author

@jjneely jjneely Apr 15, 2019

Choose a reason for hiding this comment

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

Added comments in the code. If that's not the best place, let me know.

Copy link
Member

@povilasv povilasv Apr 16, 2019

Choose a reason for hiding this comment

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

amazing, nice catch!

Copy link
Contributor Author

@jjneely jjneely Apr 16, 2019

Choose a reason for hiding this comment

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

More like, why did the repair just lose all the data in by blocks?

Copy link
Member

@brancz brancz Apr 17, 2019

Choose a reason for hiding this comment

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

Very nice catch, the comment makes it clear when we read this in 3 months again :) 👍

for _, ignoreChkFn := range ignoreChkFns {
ignore, err := ignoreChkFn(mint, maxt, last, &chks[i])
Expand Down