Fix and refactor WriteBiffer-s a little#31265
Conversation
robot-clickhouse
added
the
pr-not-for-changelog
| writer->flush(); | ||
| write_buf->sync(); | ||
| write_buf->finalize(); |
There was a problem hiding this comment.
@Avogar am I understand correctly that these hunk is a the fix (and mostly everything other is refactoring) since before it calls only WriteBuffer::next() which may leave some data?
BTW there is also commented StorageFileSink::flush, worth removing? -
ClickHouse/src/Storages/StorageFile.cpp
Lines 654 to 657 in 69cd19a
There was a problem hiding this comment.
@Avogar am I understand correctly that these hunk is a the fix (and mostly everything other is refactoring) since before it calls only WriteBuffer::next() which may leave some data?
Not only this change is the fix. In some WriteBuffers (for example LZMADeflatingWriteBuffer) final flush was performed only in destructor (not even in finalize method) and sometimes destructor could be called after query already been finished (for example in File, HDFS, S3 and URL engines, so I moved all flushes from destructors in finalize method so that we can flush final data explicitly (and it's also called in destructor if wasn't called explicitly)
There was a problem hiding this comment.
BTW there is also commented StorageFileSink::flush, worth removing? -
Yep, thanks
|
Seems like error in perf test is related, will investigate |
src/IO/WriteBufferDecorator.h
Outdated
|
|
||
| class WriteBuffer; | ||
|
|
||
| /// WriteBuffer that works with another WriteBuffer. |
There was a problem hiding this comment.
Comment is needed why this decorator is needed/what is does.
Also maybe we can use more specific name, but I don't know which one.
There was a problem hiding this comment.
Also maybe we can use more specific name, but I don't know which one.
I chose WriteBufferDecorator because we decorate data and write it to underlying buffer. But maybe we can use WriteBufferWithUnderlyingBuffer or WriteBufferWithNestedBuffer, what do you think?
|
Can someone do a |
|
@azat is not allowed to run commands Hey, I reacted but my real name is @Mergifyio |
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
@Mergifyio update |
✅ Branch has been successfully updated |
5 participants
Changelog category (leave one):
Move final flush from WriteBiffer's destructors to public method finalize so that it can be called explicitly. By now in some cases destructor of WriteBiffer could be called after the query has already been completed and data could be found in unfinished state and client will be unable to read this data for some time.