-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
feat: WAL Manager #13428
feat: WAL Manager #13428
Conversation
d75680b
to
7e227c9
Compare
afc79c5
to
d2fc460
Compare
This commit contains a WAL Manager that manages a pool of segments, and allows a configurable amount of data to be buffered in memory. More data allows for bursts of Append operations that exceed the rate at which segments can be flushed, but adds latency.
d2fc460
to
095ed5d
Compare
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.
Nice 👍 Is this PR meant to include changes for the Push API to actually make use of the manager or is that separate?
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.
LGTM
This commit changes the maxAge behavior from being checked using a fixed interval to when the flush loop gets the next segment to flush. This improves utilization, where as before we would have a lot of nearly empty segments.
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.
LGTM! This'll be far easier to manage than what we had before.
pkg/storage/wal/manager.go
Outdated
return it.r, nil | ||
} | ||
|
||
func (m *Manager) Get() (*PendingItem, error) { |
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.
mega nit: Maybe this should be called GetPending or something else?
API Clients only call Append but my mental model is that Get should return a valid WAL, not a closed one.
I don't feel strongly about it though.
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.
Yeah I don't have strong opinion on this either. I took the name Get
and Put
from sync.Pool
.
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.
Let me think on this, because PutPending
sounds weird and incorrect.
09b9103
to
a629015
Compare
What this PR does / why we need it:
This commit contains a WAL Manager that manages a pool of segments, and allows a configurable amount of data to be buffered in memory. More data allows for bursts of Append operations that exceed the rate at which segments can be flushed, but adds latency.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PRdeprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR