Skip to content

Fixed the compactor successfully exiting when actually an error occurred while compacting a blocks group#1931

Merged
bwplotka merged 1 commit intothanos-io:masterfrom
pracucci:fix-bucket-compactor-error-handling
Jan 2, 2020
Merged

Fixed the compactor successfully exiting when actually an error occurred while compacting a blocks group#1931
bwplotka merged 1 commit intothanos-io:masterfrom
pracucci:fix-bucket-compactor-error-handling

Conversation

@pracucci
Copy link
Contributor

There are conditions under which the BucketCompactor.Compact() returns no error even if an error occurred.

This happens when the number of groups left to compact is <= the concurrency. In this scenario, all jobs have been pushed to workers, so the code executions moves to wg.Wait() and any subsequent error is not catched and then returned.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Fixed the compactor successfully exiting when actually an error occurred while compacting a blocks group

Verification

Manual tests.

…red while compacting a blocks group

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Seems like an obvious fix in the logic here to me, not sure if any tests are needed here.

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Yup, thanks for spotting and fixing! <3

@bwplotka bwplotka merged commit a37ac09 into thanos-io:master Jan 2, 2020
@pracucci pracucci deleted the fix-bucket-compactor-error-handling branch January 2, 2020 14:37
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.

3 participants