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: add function to remove no compact marker #6917

Merged
merged 5 commits into from
Dec 13, 2023

Conversation

ying-jeanne
Copy link
Contributor

@ying-jeanne ying-jeanne commented Dec 13, 2023

What this PR does

Add function to remove no-compact marker, this function could then be called from admin UI in order to mark/remove no-compact marker on the block. a follow up of this PR #6848

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

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

pkg/storage/tsdb/block/block.go Outdated Show resolved Hide resolved
pkg/storage/tsdb/block/block.go Outdated Show resolved Hide resolved
Comment on lines 421 to 422
for _, tcase := range []struct {
name string
Copy link
Member

Choose a reason for hiding this comment

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

unless you need strict ordering, I would find map[string]struct{..} nicer, with key being the test name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using the vector to keep the similar test styles with all other tests in this file. I don't have strong opinion on this, will change it to map.

100, 0, 1000, labels.FromStrings("ext1", "val1"))
require.NoError(t, err)

tcase.preUpload(t, id, bkt)
Copy link
Member

Choose a reason for hiding this comment

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

nit: We upload marker file before uploading block.

Copy link
Contributor Author

@ying-jeanne ying-jeanne Dec 13, 2023

Choose a reason for hiding this comment

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

what do you mean? this preUpload is just to load a no-compact marker file in the object store in order to test the delete.

Copy link
Member

Choose a reason for hiding this comment

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

Next line calls Upload(ctx, log.NewNopLogger(), bkt, path.Join(tmpDir, id.String()), nil) which uploads block. But preUpload can already upload no-compact mark. It's not a big deal, but this sequence of actions doesn't happen in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got what you said, it makes sense, I moved the upload also into the test setup function. it is in good order now, upload blocks, then upload the marker.

pkg/storage/tsdb/block/block_test.go Outdated Show resolved Hide resolved
pkg/storage/tsdb/block/block_test.go Outdated Show resolved Hide resolved
ying-jeanne and others added 3 commits December 13, 2023 12:08
Co-authored-by: Peter Štibraný <pstibrany@gmail.com>
Co-authored-by: Peter Štibraný <pstibrany@gmail.com>
Co-authored-by: Peter Štibraný <pstibrany@gmail.com>
if err := bkt.Delete(ctx, m); err != nil {
return errors.Wrapf(err, "deletion of no-compaction marker for block %s has failed", id.String())
}
level.Info(logger).Log("msg", "block has been unmarked for no compaction", "block", id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain better what has happened? The double negative sounds complicated. Something like "no compaction mark has been removed; block may be compacted again the future"

setupTest func(t testing.TB, id ulid.ULID, bkt objstore.Bucket)
expectedError func(id ulid.ULID) error
}{
"unmark existing block should succeed": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should were add a test case for unmarking a block which doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second test "unmark non-existing block should fail", is this what you want?

Copy link
Contributor

Choose a reason for hiding this comment

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

crap, this was written in autopilot. I meant write a test which tries to unmark a block which doesn't have a marker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌 will add it in the next PR

Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm (after addressing Dimitar's comments).

@ying-jeanne ying-jeanne marked this pull request as ready for review December 13, 2023 15:39
@ying-jeanne ying-jeanne requested a review from a team as a code owner December 13, 2023 15:39
@ying-jeanne ying-jeanne merged commit 7c16572 into main Dec 13, 2023
28 checks passed
@ying-jeanne ying-jeanne deleted the unMarkForNoCompact branch December 13, 2023 15:59
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