[go: nahoru, domu]
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.

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

Jump to bottom

Prune in-memory blocks from missing tenants #314

Merged
merged 12 commits into from
Nov 10, 2020
Merged

Prune in-memory blocks from missing tenants #314

merged 12 commits into from
Nov 10, 2020

Conversation

dgzlopes
Copy link
Member
@dgzlopes dgzlopes commented Nov 3, 2020 •
edited
Loading

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]

annanay25
annanay25 previously requested changes Nov 3, 2020
View reviewed changes
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 •
edited
Loading

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 😄

@dgzlopes
Prune in-memory blocks
cda26a5 
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
@dgzlopes
Add changelog
e5c989d 
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
@dgzlopes
Feedback from review
4525eaf 
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
@dgzlopes
Add tests
c138335 
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.

joe-elliott
joe-elliott requested changes Nov 6, 2020
View reviewed changes
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
joe-elliott
joe-elliott reviewed Nov 6, 2020
View reviewed changes
tempodb/tempodb.go Outdated Show resolved Hide resolved
@dgzlopes
Improve tests
cc1e0f5 
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
@dgzlopes
Remove missing tenant blocks
ddb15bc 
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
@dgzlopes
Remove old code
c63c33f 
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
@dgzlopes
Make CI happy
1a9c557 
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
@dgzlopes
Move code to a method
230a1fd 
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
joe-elliott
joe-elliott requested changes Nov 9, 2020
View reviewed changes
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
@dgzlopes
From map[string]bool to map[string]struct
864f7ee 
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
@dgzlopes
Add test for cleanMissingTenants
ca15c6e 
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
@dgzlopes
Add enconding
bd82075 
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
@dgzlopes
Copy link
Member Author
dgzlopes commented Nov 10, 2020 •
edited
Loading

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

Fixed the nit and added the test for cleanMissingTenants

joe-elliott
joe-elliott approved these changes Nov 10, 2020
View reviewed changes
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
Reviewers

@joe-elliott joe-elliott joe-elliott approved these changes

@annanay25 annanay25 Awaiting requested review from annanay25

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Blocklist never prunes missing tenants
3 participants
@dgzlopes @joe-elliott @annanay25