-
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: Ingester RF-1 #13365
feat: Ingester RF-1 #13365
Conversation
This commit is a work-in-progress of Ingester RF-1 (replication factor = 1). It is disabled by default.
2afad46
to
64306a0
Compare
@@ -159,7 +163,9 @@ func (c *Config) RegisterFlags(f *flag.FlagSet) { | |||
c.CompactorHTTPClient.RegisterFlags(f) | |||
c.CompactorGRPCClient.RegisterFlags(f) | |||
c.IngesterClient.RegisterFlags(f) | |||
//c.IngesterRF1Client.RegisterFlags(f) |
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.
//c.IngesterRF1Client.RegisterFlags(f) | |
//c.IngesterRF1Client.RegisterFlags(f) |
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
A lot will change but let's unlock so we can play with 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 - A couple of small things to tidy up but not worth blocking for since we'll be revisiting this soon
pkg/ingester-rf1/ingester.go
Outdated
case <-i.loopQuit: | ||
return | ||
} | ||
} | ||
} | ||
|
||
func (i *Ingester) doFlushTick() { | ||
i.flushCtx.lock.Lock() | ||
defer i.flushCtx.lock.Unlock() |
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 probably doesn't hurt but we actually don't want to unlock this - the context is only locked once when its no longer accepting writes.
I wonder if its possible that this goroutine can gets pre-empted and another tick can fire before we swap in a new context? In that case we'd block the timer thread completely waiting on the lock... Best to leave the unlock here for now in that case!
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 you are right!
What this PR does / why we need it:
This pull request starts a work-in-progress of Ingester RF-1 (replication factor = 1). It is disabled by default.
Co-authored by @benclive and @grobinson-grafana.
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