-
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
Re-license and export pkg/ingester
WAL code to be used in Promtail's WAL
#8315
Conversation
pkg/ingester
WAL code to be used in Promtail's WALpkg/ingester
WAL code to be used in Promtail's WAL
I'd like to, as part of this PR, rename
What do you think @cstyan ? |
pkg/ingester
WAL code to be used in Promtail's WALpkg/ingester
WAL code to be used in Promtail's WAL
./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
Agreed on yes |
pkg/ingester/wal/encoding.go
Outdated
var ( | ||
// shared pool for WALRecords and []logproto.Entries | ||
recordPool = NewRecordPool() | ||
) | ||
|
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 we don't need this package global here? it was just used in the actual ingester package, so we can create one with NewRecordPool
there if we really need a global
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.
Yhea, just found the dependency why I created that in that file:
- It was used in encoding_test, so created one as global there for the tests themselves to use
- Used in
Record#Reset
. Inverted the dependency, and moved that to the pool itself, since it was the only caller of reset
Also, created one as global in pkg/ingester/wal.go
since it's used a bunch there
./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 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.
lets just clean up the imports ordering for consistency's sake
after that it looks like you'll need to merge in main or rebase but after that we're good to go IMO
@cstyan all import orders should be fixed now 😀 |
./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
@cstyan this PR should be good to go right? |
What this PR does / why we need it:
In PRs mentioned in #8197 , we are adding WAL support for Promtail. Since under-the-cover structure of the WAL will be much like Loki's, we need to re-use some bits from Loki's code:
WALRecord
and all of it's encode/decode methods exported (https://github.com/grafana/loki/blob/main/pkg/ingester/encoding.go#L35)resettingPool
, also exporter (loki/pkg/ingester/wal.go
Line 202 in 855d2b7
This two bits have the default Loki license applies (AGPL), which is not compatible with Promtail's , ALv2. This PR moves them to a new package, and re-licenses it.
Also, some other bits are needed which due to its package
pkg/util
, fall under ALv2 so we are ok with them:NewWALReader
(loki/pkg/util/wal/reader.go
Line 11 in 855d2b7
pkg/util/encoding
(https://github.com/grafana/loki/blob/855d2b74bc075341044130303d879a3a23fa2f64/pkg/util/encoding/encoding.go)They are also shown in this commit in of the related PRs.
Also, since the new package is called wal, the main
WALRecord
struct has been renamed towal.Record
to avoid repetition in the name itself.Which issue(s) this PR fixes:
Required for #8197
Special notes for your reviewer:
Checklist
CONTRIBUTING.md
guide (required)CHANGELOG.md
updateddocs/sources/upgrading/_index.md