-
Notifications
You must be signed in to change notification settings - Fork 515
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
Refactoring: do not pass the chunks pool to newBucketChunkReader() #3611
Conversation
Signed-off-by: Marco Pracucci <marco@pracucci.com>
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. I think we can simplify further.
// save a copy of b's payload to a buffer pulled from chunksPool. | ||
// The buffer containing the chunk data is returned. | ||
// The returned slice becomes invalid once chunksPool is released. | ||
func (r *bucketChunkReader) save(b []byte, chunksPool *pool.BatchBytes) ([]byte, 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.
This doesn't have to be a method of bucketChunkReader
anymore
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.
And even better - what do you think about inlining it in populateChunk
? It's simple enough and it's not used elsewhere
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.
Makes sense to me.
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 will also move populateChunk()
to bucket_chunk_reader.go
.
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.
While doing it, I realized that inlining save()
logic into populateChunk()
makes the code harder to read. I will keep other suggestions tho.
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.
See: c2a2039
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'm happy with what you have. also removing the indirection of passing a func is good enough
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!
…nk() to bucket_chunk_reader.go Signed-off-by: Marco Pracucci <marco@pracucci.com>
…3611) * Refactoring: do not pass the chunks pool to newBucketChunkReader() Signed-off-by: Marco Pracucci <marco@pracucci.com> * Renamed bucketChunkReader.save() to saveChunk() and moved populateChunk() to bucket_chunk_reader.go Signed-off-by: Marco Pracucci <marco@pracucci.com> Signed-off-by: Marco Pracucci <marco@pracucci.com>
…rafana#3611) * Refactoring: do not pass the chunks pool to newBucketChunkReader() Signed-off-by: Marco Pracucci <marco@pracucci.com> * Renamed bucketChunkReader.save() to saveChunk() and moved populateChunk() to bucket_chunk_reader.go Signed-off-by: Marco Pracucci <marco@pracucci.com> Signed-off-by: Marco Pracucci <marco@pracucci.com>
What this PR does
Currently
newBucketChunkReader()
requires the chunks pool in input. This complicates the implementation of the streaming store-gateway, so in this PR I'm proposing to remove it and just pass the chunks pool tobucketChunkReader.load()
which is where is required. Keep in mindpool.BatchBytes
is concurrency safe.Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]