-
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
chore: Refactor storage interface for rf1 #13415
Conversation
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 - using io.Reader & removing PutWal is a big improvement imo. Couple of minor comments but otherwise LGTM
return store, nil | ||
} | ||
|
||
func (m *Multi) GetStoreFor(ts model.Time) (client.ObjectClient, 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.
Thanks for implementing this - I wasn't sure how it was meant to work with the existing store implementation.
if rs, ok := r.(io.ReadSeeker); ok { | ||
return rs, nil | ||
} | ||
data, err := io.ReadAll(r) |
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.
Should we think about a sync.Pool here? Presumably it allocates a new buffer every time
Should be fine for now, we can return to it if it's an issue.
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 caller should really pass a ReadSeeker, this is a best effort.
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
if 0 <= j && j < len(m.stores) { | ||
return m.stores[j].objectClient, nil | ||
} |
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.
nit: if the first schema is after the ts
, sort.Search() returns 0
, after this we decrease it and it will be -1
that will fail this code.
but it's possible only in case the user misconfigured his schema_config.
What this PR does / why we need it:
This refactor the RF1 storage to use io.Reader as much as possible to avoid creating a temporary buffer.
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