Implement support for insertion into Kafka tables#6012
Implement support for insertion into Kafka tables#6012abyss7 merged 20 commits intoClickHouse:masterfrom
Conversation
|
Should implement #5834 |
|
Part with output formats seems ok. |
Don't see any reason to do it now - there is a single param. |
KochetovNicolai
left a comment
KochetovNicolai
left a comment
There was a problem hiding this comment.
Part with streams is fine.
| inline void next() | ||
| { | ||
| if (!offset()) | ||
| if (!offset() && available()) |
There was a problem hiding this comment.
I suggest that I wanted to prevent miscalling next() if there is still available space in buffer - it's permissive, so no penalties for logical error in the code. But maybe there should be assert(!available()). Does this break anything?
There was a problem hiding this comment.
Does this break anything?
I have't any example, but is this strictly correct?
AFAICS right now destructor tries to send the data, so after this change it will send only if the buffer is full, sounds icky
I came to this while working on #19451
And quite frankly to me sending some data from destructor looks not safe anyway, and the goal is to replace destructor final flush with assert that will ensure that there is no bytes pending in buffer.
There was a problem hiding this comment.
if (!offset() && available())
return;
This is strange.
The following code means: if there is no data to write - ignore the next call:
if (!offset())
return;
This is how it was and it is 100% correct.
But available() means - if the buffer is not filled up completely.
Adding this condition with "and" is bogus - because if the buffer size is greater than zero, and !offset() - at least one byte is available.
Even if you meant this:
if (!offset() || available())
return;
it also does not make sense, because flushing half-filled buffer is completely legit and is used extensively.
There was a problem hiding this comment.
Okay, let's try to revert this hunk - #19886, and see, maybe CI will find something interesting.
In ClickHouse#6012 the behavior of the WriteBuffer has been changed. Before, WriteBuffer write the data when next() is called. After, WriteBuffer will write the data if, and only if, the buffer is full. See the discussion here [1]. [1]: ClickHouse#6012 (comment)
4 participants
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
For changelog. Remove if this is non-significant change.
Category (leave one):
Short description (up to few sentences):
Implement support for INSERT-query with Kafka tables