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

Compactor: Remove BlockDeletionMarksMigrationEnabled #122

Merged
merged 17 commits into from
Aug 19, 2021

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Aug 11, 2021

What this PR does:
Address TODO and remove BlockDeletionMarksMigrationEnabled in compactor code.

Which issue(s) this PR fixes:

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@aknuds1 aknuds1 requested a review from pracucci August 11, 2021 13:44
@aknuds1 aknuds1 added the chore label Aug 11, 2021
Signed-off-by: Arve Knudsen <[email protected]>
pkg/compactor/blocks_cleaner.go Outdated Show resolved Hide resolved
tools/doc-generator/parser.go Outdated Show resolved Hide resolved
@aknuds1 aknuds1 requested a review from pstibrany August 16, 2021 16:51
@aknuds1 aknuds1 requested a review from pracucci August 17, 2021 11:41
@aknuds1
Copy link
Contributor Author

aknuds1 commented Aug 17, 2021

@pracucci @pstibrany any suggestions on how to fix the failing tests?

@pracucci
Copy link
Collaborator

@pracucci @pstibrany any suggestions on how to fix the failing tests?

Failing tests need to be updated too. They're testing the markers migration.

We also need a CHANGELOG entry.

@pstibrany No migration is required to build #145 because we don't have no-compact markers yet, right? I just wanna make sure we're not removing the support to migration and then we'll add it back for #145.

@pstibrany
Copy link
Member

we don't have no-compact markers yet, right?

That's right. I'm not aware of Cortex/Mimir having or using no-compact markers.

@aknuds1 aknuds1 marked this pull request as ready for review August 18, 2021 07:00
@aknuds1 aknuds1 changed the title Chore: Remove BlockDeletionMarksMigrationEnabled Compactor: Remove BlockDeletionMarksMigrationEnabled Aug 18, 2021
@aknuds1 aknuds1 requested a review from a team August 18, 2021 07:08
@aknuds1
Copy link
Contributor Author

aknuds1 commented Aug 18, 2021

@pstibrany @pracucci looks as if I could fix tests, PTAL.

@aknuds1 aknuds1 requested a review from chaudum August 18, 2021 07:18
@aknuds1
Copy link
Contributor Author

aknuds1 commented Aug 18, 2021

@09jvilla @mattmendick what's your opinion on this change, is it good to go?

Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I left a comment about the CHANGELOG. A part from this, we can merge if/once we get the green light from Jen and Matt.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Marco Pracucci <[email protected]>
@09jvilla
Copy link
Contributor

Yall can go ahead and merge. We'll make a note for customers upgrading to GEM 1.6 that they must go through GEM 1.5, which should ensure we have no issues.

@09jvilla
Copy link
Contributor

cc @mattmendick

Copy link
Contributor

@09jvilla 09jvilla left a comment

Choose a reason for hiding this comment

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

Not a code review, but just confirming this is ok with the GEM team.

Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

LGTM.

I have also ensured that we already have a ticket for the next GEM release (https://github.com/grafana/backend-enterprise/issues/2063) so we don't forget to mention that in the changelog.

@09jvilla
Copy link
Contributor

09jvilla commented Aug 18, 2021 via email

@pracucci
Copy link
Collaborator

Then we merge! Worst case scenario, rolling back this change is not a tragedy.

@pracucci pracucci merged commit 88a3009 into main Aug 19, 2021
@pracucci pracucci deleted the chore/minor-cleanup branch August 19, 2021 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants