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

Prune in-memory blocks from missing tenants #314

Merged
merged 12 commits into from Nov 10, 2020

Conversation

dgzlopes
Copy link
Member

@dgzlopes dgzlopes commented Nov 3, 2020

What this PR does:
If a tenant disappears blocks are pruned from the in-memory blocklist.

Which issue(s) this PR fixes:
Fixes #285

Checklist

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

Copy link
Contributor

@annanay25 annanay25 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @dgzlopes! I've left some comments.

tempodb/tempodb.go Outdated Show resolved Hide resolved
tempodb/tempodb.go Outdated Show resolved Hide resolved
@joe-elliott
Copy link
Member

To add to Annanay's comments, I would like a test that covers this. Please just ask if you'd like some guidance on building a test, I know they're a little intense at the moment.

@dgzlopes
Copy link
Member Author

dgzlopes commented Nov 4, 2020

Moved the code into a len(blockIDs) == 0 block and changed the log level to info.

Okey @joe-elliott!

I'm diving into the tests, and well, they look fun. I'll try to add the coverage for this PR. If I'm blocked I'll ask for some guidance 😄

Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
@dgzlopes
Copy link
Member Author

dgzlopes commented Nov 6, 2020

Added a test. I'm not sure if it's OK... it's my first test in Go 😄

I re-used TestRetention code, but this time before calling pollBlocklist() I remove the testTenantID trace folder. Then, I check if the in-memory blocklists still have the blocks.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Nice work on the tests. Had a couple of suggestions for improvements and it looks like the linter is mad at you.

tempodb/tempodb_test.go Outdated Show resolved Hide resolved
tempodb/tempodb_test.go Show resolved Hide resolved
tempodb/tempodb.go Outdated Show resolved Hide resolved
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Basically just looking for some tests directly targeting the cleanMissingTenants method. Leave the existing test and add a TestCleanMissingTenants() test that directly exercises this method.

After that should be ready to go!

tempodb/tempodb.go Outdated Show resolved Hide resolved
tempodb/tempodb.go Show resolved Hide resolved
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
@dgzlopes
Copy link
Member Author

dgzlopes commented Nov 10, 2020

Thanks a lot for the patience and the exhaustive review @joe-elliott :)

Fixed the nit and added the test for cleanMissingTenants

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

nice work @dgzlopes

Thanks!

@joe-elliott joe-elliott dismissed annanay25’s stale review November 10, 2020 13:50

I believe I need to dismiss this to merge? It says merging is blocked due to changes requested, but it seems like all changes are resolved?

@joe-elliott joe-elliott merged commit 2f0917d into grafana:master Nov 10, 2020
@dgzlopes dgzlopes deleted the prune-in-memory-blocklist branch November 10, 2020 13:54
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.

Blocklist never prunes missing tenants
3 participants