-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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: Remove flush loop and queue from Ingester RF-1 #13538
Conversation
@@ -141,7 +145,6 @@ func NewManager(cfg Config, metrics *Metrics) (*Manager, error) { | |||
metrics: metrics.ManagerMetrics, | |||
available: list.New(), | |||
pending: list.New(), | |||
shutdown: make(chan struct{}), |
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.
Removed as it wasn't used.
m.mu.Lock() | ||
defer m.mu.Unlock() | ||
m.closed = true | ||
if m.available.Len() > 0 { |
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.
This is duplicate code. I will refactor it in a follow up PR. But I wanted to show how it works here.
414c32e
to
a338260
Compare
This commit removes the flush loop and flush queue from Ingester RF-1. This code is from the original ingester code, and is no longer needed since we have the WAL Manager.
a338260
to
cfaa574
Compare
op := o.(*flushOp) | ||
|
||
if it == nil { | ||
// TODO: Do something more clever here instead. |
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 was concerned this would increase CPU usage, but having tested it in dev it doesn't have much effect at all.
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 next pending should be blocking instead
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 thought about it too! I'll take a look at how to use sync.Cond
to achieve this.
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
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, LGTM!
What this PR does / why we need it:
This commit removes the flush loop and flush queue from Ingester RF-1. This code is from the original ingester code, and is no longer needed since we have the WAL Manager.
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