-
Notifications
You must be signed in to change notification settings - Fork 529
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
Implement polling improvements #2652
Conversation
7e423c0
to
63f1c85
Compare
f0417b7
to
a47fec5
Compare
As long as the polling takes less time than the polling cycle I think we're ok. Double the polling time might not matter if it's going from 20s -> 40s on 200k blocks or something like that. |
2672fbe
to
4c7072c
Compare
I've made a small change to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass. Have some Qs. Let me know if you want to have a sync convo.
4c7072c
to
ba11f7e
Compare
Thanks for the review @joe-elliott. I'm happy to chat through this whenever. |
ee8950c
to
bf5f1c6
Compare
4512d1d
to
40faa93
Compare
tempodb/blocklist/poller.go
Outdated
} else if cm != nil { | ||
chCompactedMeta <- cm | ||
} else if err != nil { | ||
mtx.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the cost of the lock in this context is basically nothing. lock/defer unlock is a safer pattern that is more difficult to mess up as functionality changes.
you could also rely on the lock in this case to protect the err var. up to you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of smaller comments, but I'm g2g. Great improvement!
continue | ||
} | ||
|
||
// TODO: Review the ability to avoid polling for compacted blocks that we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the code you've added does this? if so we can clean up this todo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite. What I had in mind might not be possible, but the note here is about avoiding pulling the data entirely if the block transitions from meta -> compactedMeta. We have all the detail except the time it was compacted. More generally, I wonder if we already had this data in memory, then we know that it is only one polling cycle out of date, and perhaps we could be fuzzy enough on this that we don't have to know the precise time. More thinking needed probably.
What this PR does:
Currently, each polling cycle, the block list is read from the backend, followed by a read of each meta.json file. If the meta.json returns not-found, then an additional call is made to the backend to fetch the meta.compacted.json. During compaction, the meta.json is replaced with meta.compacted.json, but the details about the block remain unchanged. This means that tempo performs lots of fetching for information that is already known from the previous poll. This can be a significant source of backend calls, and in turn, latency of the polling cycle. In environments with a high number of blocks, this problem is amplified.
Here we extend the backend interfaces to create a ListBlocks() method that can be used to target block information direclty from each backend.
This results in fewer calls to the backend, and a may decrease in total time spent for each poll cycle.
Additional changes to the structure of the backend are made to improve the duration while keeping the total calls in check.
Which issue(s) this PR fixes:
Fixes #2521
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]