akalash commented on a change in pull request #11877:
URL: https://github.com/apache/flink/pull/11877#discussion_r662778055
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/PipelinedSubpartition.java
##########
@@ -288,9 +288,7 @@ BufferAndBacklog pollBuffer() {
if (buffers.isEmpty()) {
flushRequested = false;
- }
-
- while (!buffers.isEmpty()) {
+ } else {
Review comment:
> appending data and finish buffer is not an atomic operation.
It is exactly what I want to emphasize. As I understand, we need the empty
buffer only because `appending data and finish buffer is not an atomic
operation.` which happens because BufferBuilder has a gap between `commit` and
`finish`. So I propose to rewrite BufferBuilder in such a way that if it
commits the current state and size == capacity it finishes it immediately(so
there is no explicit finish call needed). These changes allow us to avoid all
logic with an empty buffer which I think pretty untransparent.
One more time, my proposal:
- Rewriting the `BufferBuilder` in such a way that it will be impossible to
have the empty finished buffer(it just requires removing the gap between the
last commit and finish).
- Don't send the empty buffer(after changes described above there won't be
any empty buffers)
@wsry WDYT?
@pnowojski What is your opinion, is it safe to finish BufferBuilder
immediately when it becomes full? I mean not to wait for call explicit `finish`
but finish in the `commit` if it is full?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
