Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
index header: Remove memWriter from fileWriter #6509
index header: Remove memWriter from fileWriter #6509
Changes from all commits
9bc4e71
6486b6c
a10991b
4c3266b
15f898c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 looks a bit like a hack. Where is the
Buffer
method called? Maybe we can just use aPosWriterWithBuffer
there 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.
It is used here https://github.com/thanos-io/thanos/blob/main/pkg/block/indexheader/binary_reader.go#L130.
It will only be called if it is a
MemWriter
. If it is file writer mode it returns previously. I think the design is not the best though and needs more refactoring.If we think this kind of hack is not ok, I can add the
Buffer
method back toFileWriter
and just return nilThere 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.
Is it possible to write the bytes to both the file writer and the memory writer? So we would need some sort of a composite
Writer
that writes to multipleWriter
s.This way the
Buffer
would also go away.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.
It is the current implementation and I don't think it is correct. We don't have to write memory writer if we have the file writer IIUC. It is just holding the extra memory for nothing. (We are reading from the file anyway)
In the current implementation, file writer has a memory writer but the memory writer does nothing, only providing the
Pos()
method because memory writer keeps track the pos, but we can do the same in the file writer itself.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.
Hm then why do we need the buffer? Does it hold the meta file temporarily?
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.
The
buffer
is used by memory writer only because it holds the index header all in memory.It is not used in file writer at all. But the current interface
PosWriter
hasbuffer
method so both file writer and memory writer implements it.This pr basically removes that buffer from file writer. But I don't have a good way to refactor the
PosWriter
interface to removebuffer
method so I addedPosWriterWithBuffer
.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.
https://github.com/thanos-io/thanos/blob/main/pkg/block/indexheader/binary_reader.go#L547 As you can see, the first returned value is the buffer but it is ignored if it is file mode. Otherwise, at line 554 the returned buffer will be used.
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.
@fpetkovski Does it make sense overall? TLDR is that buffer is not used at all for file writer.
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 this is fine for now, it's more important to remove the memory footprint than to address the design.