[jira] [Comment Edited] (FLINK-31610) Refactoring of LocalBufferPool

["Weijie Guo (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/FLINK-31610?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17704646#comment-17704646 ] 

Weijie Guo edited comment on FLINK-31610 at 3/24/23 2:46 PM:
-------------------------------------------------------------

Thanks [~akalash] for driving this! 

From my point of view, this is really a very meaningful work. {{BufferPool}} is a very low-level and basic component in Flink, but it is now so complicated that we can hardly be confident that there are no bugs in it. To be honest, issues like {{FLINK-31293}} and {{FLINK-29298}} are very difficult to troubleshoot, in part because our current implementation is too complex and there are no strong guarantees at the code level for some assumptions.

Back to this ticket, the simplifications in the description are in the right direction. The existence of {{numberOfRequestedOverdraftMemorySegments}} field makes our consistency maintenance very fragile. But there seems to be a small problem with removing it: Consider such a scenario, the {{CurrentPoolSize = 5}}, {{numOfRequestedMemorySegments = 7}}, {{maxOverdraftBuffersPerGate = 2}}. If {{numberOfRequestedOverdraftMemorySegments = 0}}, then 2 buffers can be requested now. If the counter for requested overdraft-buffers is removed, is it still allowed to request buffers in this case, and if so, how many buffers can be requested at most?

But it is possible that this is the culprit of the current difficulty in maintaining consistency. IIUC, the solution you describe actually changes the definition of overdraft buffer from static to dynamic. If we consider the part exceeding {{CurrentPoolSize}} as overdraft, then things will be much simpler. If this part of the buffer exceeds the upper limit(i.e. {{maxOverdraftBuffersPerGate}}), then no more buffers can be requested. However, there may be other problems in doing so, at least we have broken the previous behavior to some extent. I need to think in more detail about the correctness of this solution.

Any other comments are welcome. I'd like to also cc [~kevin.cyj] to see if he can give more input.



was (Author: weijie guo):
Thanks [~akalash] for driving this! 

From my point of view, this is really a very meaningful work. {{BufferPool}} is a very low-level and basic component in Flink, but it is now so complicated that we can hardly be confident that there are no bugs in it. To be honest, issues like {{FLINK-31293}} and {{FLINK-29298}} are very difficult to troubleshoot, in part because our current implementation is too complex and there are no strong guarantees at the code level for some assumptions.

Back to this ticket, the simplifications in the description are in the right direction. The existence of {{numberOfRequestedOverdraftMemorySegments}} field makes our consistency maintenance very fragile. But there seems to be a small problem with removing it: Consider such a scenario, the {{CurrentPoolSize = 5}}, {{numOfRequestedMemorySegments = 7}}, {{maxOverdraftBuffersPerGate = 2}}. If {{numberOfRequestedOverdraftMemorySegments = 0}}, then 2 buffers can be requested now. If the count of overdraft buffers that have been requested is removed, is it still allowed to request buffers in this case, and if so, how many buffers can be requested at most?

But it is possible that this is the culprit of the current difficulty in maintaining consistency. IIUC, the solution you describe actually changes the definition of overdraft buffer from static to dynamic. If we consider the part exceeding {{CurrentPoolSize}} as overdraft, then things will be much simpler. If this part of the buffer exceeds the upper limit(i.e. {{maxOverdraftBuffersPerGate}}), then no more buffers can be requested. However, there may be other problems in doing so, at least we have broken the previous behavior to some extent. I need to think in more detail about the correctness of this solution.

Any other comments are welcome. I'd like to also cc [~kevin.cyj] to see if he can give more input.


> Refactoring of LocalBufferPool
> ------------------------------
>
>                 Key: FLINK-31610
>                 URL: https://issues.apache.org/jira/browse/FLINK-31610
>             Project: Flink
>          Issue Type: Improvement
>          Components: Runtime / Network
>    Affects Versions: 1.17.0
>            Reporter: Anton Kalashnikov
>            Priority: Major
>
> FLINK-31293 bug highlighted the issue with the internal mutual consistency of different fields in LocalBufferPool. ex.:
> -  `numberOfRequestedOverdraftMemorySegments`
> -  `numberOfRequestedMemorySegments`
> -  `availableMemorySegment`
> -  `currentPoolSize`
> Most of the problem was fixed already(I hope) but it is a good idea to reorganize the code in such a way that all invariants between all fields inside will be clearly determined and difficult to break.
> As one example I can propose getting rid of numberOfRequestedOverdraftMemorySegments and using existing numberOfRequestedMemorySegments instead. That means:
> - the pool will be available when `!availableMemorySegments.isEmpty() && unavailableSubpartitionsCount == 0`
> - we don't request a new `ordinary` buffer when `numberOfRequestedMemorySegments >=  currentPoolSize` but we request the overdraft buffer instead
> - `setNumBuffers` should work automatically without any changes
> I think we can come up with a couple of such improvements to simplify the code.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)