You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "reswqa (via GitHub)" <gi...@apache.org> on 2023/04/21 10:26:02 UTC

[GitHub] [flink] reswqa opened a new pull request, #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

reswqa opened a new pull request, #22447:
URL: https://github.com/apache/flink/pull/22447

   ## What is the purpose of the change
   
   *After [FLINK-31763](https://issues.apache.org/jira/browse/FLINK-31763), we don't need the specific field `numberOfRequestedOverdraftMemorySegments` to record the overdraft buffers has been requested anymore since we regard all buffers exceeding the `currentPoolSize` as overdraft.*
   
   
   ## Brief change log
   
     - *Introduce getNumberOfRequestedMemorySegments and rename the old one to a more appropriate name.*
     - *Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool.*
   
   
   ## Verifying this change
   
   
   This change is already covered by existing tests in `LocalBufferPoolTest`.
   
   ## Does this pull request potentially affect one of the following parts:
   
     - Dependencies (does it add or upgrade a dependency): no
     - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
     - The serializers: no
     - The runtime per-record code paths (performance sensitive): per-buffer
     - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
     - The S3 file system connector: no
   
   ## Documentation
   
     - Does this pull request introduce a new feature? no
   


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] reswqa commented on pull request #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

Posted by "reswqa (via GitHub)" <gi...@apache.org>.
reswqa commented on PR #22447:
URL: https://github.com/apache/flink/pull/22447#issuecomment-1518950603

   Hi @1996fanrui and @akalash, would you mind taking a look at this? Thx~


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] reswqa commented on pull request #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

Posted by "reswqa (via GitHub)" <gi...@apache.org>.
reswqa commented on PR #22447:
URL: https://github.com/apache/flink/pull/22447#issuecomment-1521183716

   Thanks @1996fanrui for the quick review, I have updated this in a fix-up commit, please take a look~


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] reswqa commented on pull request #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

Posted by "reswqa (via GitHub)" <gi...@apache.org>.
reswqa commented on PR #22447:
URL: https://github.com/apache/flink/pull/22447#issuecomment-1535639126

   Rebased on master to avoid the CI problem caused by [FLINK-30972](https://issues.apache.org/jira/browse/FLINK-30972).


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] flinkbot commented on pull request #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #22447:
URL: https://github.com/apache/flink/pull/22447#issuecomment-1517625986

   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "62fc6fa527aaf68ee2e2d582a56107ad52fd8714",
       "status" : "UNKNOWN",
       "url" : "TBD",
       "triggerID" : "62fc6fa527aaf68ee2e2d582a56107ad52fd8714",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * 62fc6fa527aaf68ee2e2d582a56107ad52fd8714 UNKNOWN
   
   <details>
   <summary>Bot commands</summary>
     The @flinkbot bot supports the following commands:
   
    - `@flinkbot run azure` re-run the last Azure build
   </details>


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] reswqa commented on a diff in pull request #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

Posted by "reswqa (via GitHub)" <gi...@apache.org>.
reswqa commented on code in PR #22447:
URL: https://github.com/apache/flink/pull/22447#discussion_r1176029546


##########
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/buffer/LocalBufferPoolTest.java:
##########
@@ -878,6 +874,18 @@ private void assertRequestedBufferAndIsAvailable(
     // Helpers
     // ------------------------------------------------------------------------
 
+    private static int getNumberRequestedOverdraftBuffers(LocalBufferPool bufferPool) {
+        return bufferPool.getNumberOfRequestedMemorySegments() > bufferPool.getNumBuffers()
+                ? bufferPool.getNumberOfRequestedMemorySegments() - bufferPool.getNumBuffers()
+                : 0;

Review Comment:
   Good suggestion! 👍 



##########
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/buffer/LocalBufferPoolTest.java:
##########
@@ -878,6 +874,18 @@ private void assertRequestedBufferAndIsAvailable(
     // Helpers
     // ------------------------------------------------------------------------
 
+    private static int getNumberRequestedOverdraftBuffers(LocalBufferPool bufferPool) {
+        return bufferPool.getNumberOfRequestedMemorySegments() > bufferPool.getNumBuffers()
+                ? bufferPool.getNumberOfRequestedMemorySegments() - bufferPool.getNumBuffers()
+                : 0;
+    }
+
+    private static int getNumberRequestedOrdinaryBuffers(LocalBufferPool bufferPool) {
+        return bufferPool.getNumberOfRequestedMemorySegments() > bufferPool.getNumBuffers()
+                ? bufferPool.getNumBuffers()
+                : bufferPool.getNumberOfRequestedMemorySegments();

Review Comment:
   Good suggestion! 👍
   
   



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] akalash commented on a diff in pull request #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

Posted by "akalash (via GitHub)" <gi...@apache.org>.
akalash commented on code in PR #22447:
URL: https://github.com/apache/flink/pull/22447#discussion_r1184841754


##########
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/buffer/LocalBufferPool.java:
##########
@@ -470,7 +467,7 @@ private MemorySegment requestOverdraftMemorySegmentFromGlobal() {
 
         MemorySegment segment = networkBufferPool.requestPooledMemorySegment();
         if (segment != null) {
-            numberOfRequestedOverdraftMemorySegments++;
+            numberOfRequestedMemorySegments++;

Review Comment:
   I think It is better if we increase/decrease numberOfRequestedMemorySegments only in one place to avoid future mistakes. Right now we have `requestMemorySegmentFromGlobal` and `requestOverdraftMemorySegmentFromGlobal`. Perhaps, it makes sense to extract common code from them, etc.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] reswqa commented on pull request #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

Posted by "reswqa (via GitHub)" <gi...@apache.org>.
reswqa commented on PR #22447:
URL: https://github.com/apache/flink/pull/22447#issuecomment-1534754347

   Thanks @akalash for carefully confirming this change! I have been extracted the logic related to request buffer from global pool and increase the `numberOfRequestedMemorySegments` into a common method in the fix-up commit. PTAL~


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] akalash commented on a diff in pull request #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

Posted by "akalash (via GitHub)" <gi...@apache.org>.
akalash commented on code in PR #22447:
URL: https://github.com/apache/flink/pull/22447#discussion_r1185009999


##########
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/buffer/LocalBufferPool.java:
##########
@@ -460,17 +454,25 @@ private boolean requestMemorySegmentFromGlobal() {
     private MemorySegment requestOverdraftMemorySegmentFromGlobal() {
         assert Thread.holdsLock(availableMemorySegments);
 
-        if (numberOfRequestedOverdraftMemorySegments >= maxOverdraftBuffersPerGate) {
+        // if overdraft buffers(i.e. buffers exceeding poolSize) is greater than or equal to
+        // maxOverdraftBuffersPerGate, no new buffer can be requested.
+        if (numberOfRequestedMemorySegments - currentPoolSize >= maxOverdraftBuffersPerGate) {
             return null;
         }
 
         checkState(
                 !isDestroyed,
                 "Destroyed buffer pools should never acquire segments - this will lead to buffer leaks.");

Review Comment:
   I think you can move `checkState` to your newly created method `requestPooledMemorySegment` as well



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] 1996fanrui commented on a diff in pull request #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on code in PR #22447:
URL: https://github.com/apache/flink/pull/22447#discussion_r1175969922


##########
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/buffer/LocalBufferPoolTest.java:
##########
@@ -878,6 +874,18 @@ private void assertRequestedBufferAndIsAvailable(
     // Helpers
     // ------------------------------------------------------------------------
 
+    private static int getNumberRequestedOverdraftBuffers(LocalBufferPool bufferPool) {
+        return bufferPool.getNumberOfRequestedMemorySegments() > bufferPool.getNumBuffers()
+                ? bufferPool.getNumberOfRequestedMemorySegments() - bufferPool.getNumBuffers()
+                : 0;

Review Comment:
   How about this?
   
   ```suggestion
           return Math.max(bufferPool.getNumberOfRequestedMemorySegments() - bufferPool.getNumBuffers(), 0);
   ```



##########
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/buffer/LocalBufferPoolTest.java:
##########
@@ -878,6 +874,18 @@ private void assertRequestedBufferAndIsAvailable(
     // Helpers
     // ------------------------------------------------------------------------
 
+    private static int getNumberRequestedOverdraftBuffers(LocalBufferPool bufferPool) {
+        return bufferPool.getNumberOfRequestedMemorySegments() > bufferPool.getNumBuffers()
+                ? bufferPool.getNumberOfRequestedMemorySegments() - bufferPool.getNumBuffers()
+                : 0;
+    }
+
+    private static int getNumberRequestedOrdinaryBuffers(LocalBufferPool bufferPool) {
+        return bufferPool.getNumberOfRequestedMemorySegments() > bufferPool.getNumBuffers()
+                ? bufferPool.getNumBuffers()
+                : bufferPool.getNumberOfRequestedMemorySegments();

Review Comment:
   ```suggestion
           return Math.min(bufferPool.getNumBuffers(), bufferPool.getNumberOfRequestedMemorySegments());
   ```



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] reswqa commented on pull request #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

Posted by "reswqa (via GitHub)" <gi...@apache.org>.
reswqa commented on PR #22447:
URL: https://github.com/apache/flink/pull/22447#issuecomment-1535049629

   @flinkbot run azure


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] reswqa commented on pull request #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

Posted by "reswqa (via GitHub)" <gi...@apache.org>.
reswqa commented on PR #22447:
URL: https://github.com/apache/flink/pull/22447#issuecomment-1536997003

   CI passed, merging...


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] reswqa commented on a diff in pull request #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

Posted by "reswqa (via GitHub)" <gi...@apache.org>.
reswqa commented on code in PR #22447:
URL: https://github.com/apache/flink/pull/22447#discussion_r1185020735


##########
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/buffer/LocalBufferPool.java:
##########
@@ -460,17 +454,25 @@ private boolean requestMemorySegmentFromGlobal() {
     private MemorySegment requestOverdraftMemorySegmentFromGlobal() {
         assert Thread.holdsLock(availableMemorySegments);
 
-        if (numberOfRequestedOverdraftMemorySegments >= maxOverdraftBuffersPerGate) {
+        // if overdraft buffers(i.e. buffers exceeding poolSize) is greater than or equal to
+        // maxOverdraftBuffersPerGate, no new buffer can be requested.
+        if (numberOfRequestedMemorySegments - currentPoolSize >= maxOverdraftBuffersPerGate) {
             return null;
         }
 
         checkState(
                 !isDestroyed,
                 "Destroyed buffer pools should never acquire segments - this will lead to buffer leaks.");

Review Comment:
   Ah, good catch! I somehow missed this 😄 PR updated.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] reswqa commented on pull request #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

Posted by "reswqa (via GitHub)" <gi...@apache.org>.
reswqa commented on PR #22447:
URL: https://github.com/apache/flink/pull/22447#issuecomment-1534797190

   Squashed the fix-up commit, let's waiting for the CI green.


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] reswqa merged pull request #22447: [FLINK-31764][runtime] Get rid of numberOfRequestedOverdraftMemorySegments in LocalBufferPool

Posted by "reswqa (via GitHub)" <gi...@apache.org>.
reswqa merged PR #22447:
URL: https://github.com/apache/flink/pull/22447


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org