[FLINK-25819][runtime] Reordered requesting and recycling buffers in order to avoid race condition in testIsAvailableOrNotAfterRequestAndRecycleMultiSegments #18865

akalash · 2022-02-21T15:49:46Z

What is the purpose of the change

This PR fixes NetworkBufferPoolTest.testIsAvailableOrNotAfterRequestAndRecycleMultiSegments

Brief change log

Reordered requesting and recycling buffers in order to avoid race condition in testIsAvailableOrNotAfterRequestAndRecycleMultiSegments
*Added new test for 'Insufficient number of network buffers' scenario into NetworkBufferPoolTest *
CodeStyle correction for NetworkBufferPoolTest

Verifying this change

It is test

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

akalash · 2022-02-21T15:50:22Z

Please pay attention to the last commit where the global test timeout was removed

flinkbot · 2022-02-21T15:53:01Z

CI report:

4c8d8ff Azure: SUCCESS
01e4300 Azure: PENDING

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

flinkbot · 2022-02-21T15:53:55Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 31fce89 (Mon Feb 21 15:53:54 UTC 2022)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

akalash · 2022-02-22T11:33:49Z

@flinkbot run azure

smattheis · 2022-02-22T15:36:19Z

...-runtime/src/test/java/org/apache/flink/runtime/io/network/buffer/NetworkBufferPoolTest.java

+
+            try {
+                // request another 5 segments
+                globalPool.requestUnpooledMemorySegments(numberOfSegmentsToRequest);


Wouldn't be good to check that requesting even single one segment should fail to make sure it is empty? Or maybe both requesting one and five?

Yes, perhaps using the one instead of five is good idea

smattheis · 2022-02-22T15:37:14Z

...-runtime/src/test/java/org/apache/flink/runtime/io/network/buffer/NetworkBufferPoolTest.java

+            // Starting to requesting next segments only after previous segments were release
+            // otherwise the race condition between requesting new segments and releasing old ones
+            // are possible:
+            asyncRequest.start();


Does the async request actually make sense here? I'm not sure if the async request adds something relevant to the test case?

In addition, isn't the use of ArrayList in a concurrent thread non-thread-safe? (CopyOnWriteArrayList would be a thread-safe alternative.)

Actually, right now I see that this test was incorrect. This test supposed that requestUnpooledMemorySegments would be blocked until enough buffers will be available but it is just not true. I don't know it was the initial mistake in the test or the old behavior was different. Anyway I will remove this extra thread that doesn't make sense anymore.

smattheis

See comments in code.

akalash

@smattheis, thanks for the review. I made a couple of changes inside of the current commits. I think it would not be a problem since the commits are small

akalash · 2022-02-23T16:19:23Z

...-runtime/src/test/java/org/apache/flink/runtime/io/network/buffer/NetworkBufferPoolTest.java

+
+            try {
+                // request another 5 segments
+                globalPool.requestUnpooledMemorySegments(numberOfSegmentsToRequest);


Yes, perhaps using the one instead of five is good idea

akalash · 2022-02-23T16:36:37Z

...-runtime/src/test/java/org/apache/flink/runtime/io/network/buffer/NetworkBufferPoolTest.java

+            // Starting to requesting next segments only after previous segments were release
+            // otherwise the race condition between requesting new segments and releasing old ones
+            // are possible:
+            asyncRequest.start();


Actually, right now I see that this test was incorrect. This test supposed that requestUnpooledMemorySegments would be blocked until enough buffers will be available but it is just not true. I don't know it was the initial mistake in the test or the old behavior was different. Anyway I will remove this extra thread that doesn't make sense anymore.

smattheis

LGTM.

dawidwys · 2022-02-25T10:17:54Z

...-runtime/src/test/java/org/apache/flink/runtime/io/network/buffer/NetworkBufferPoolTest.java

@@ -757,7 +746,7 @@ public void testBlockingRequestFromMultiLocalBufferPool()
            // wait until all available buffers are requested
            while (segmentsRequested.size() + segments.size() + exclusiveSegments.size()
                    < numBuffers) {
-                Thread.sleep(100);


Is this a necessary change? Is it in the right commit?

Not necessary at all. I just notice that 100 ms is too high here considering we have a loop. But perhaps, it is indeed not a good idea to have these changes in this commit. I can remove this change at all.

We can leave it the way it is. Just wanted to check.

...-runtime/src/test/java/org/apache/flink/runtime/io/network/buffer/NetworkBufferPoolTest.java

…order to avoid race condition in testIsAvailableOrNotAfterRequestAndRecycleMultiSegments

…work buffers' scenario into NetworkBufferPoolTest

dawidwys · 2022-02-25T14:30:06Z

@akalash Do you mind creating a backport for 1.14?

akalash · 2022-02-25T14:39:03Z

@dawidwys, good idea. I will do that

rmetzger added the component=Runtime/Network label Feb 21, 2022

smattheis reviewed Feb 22, 2022

View reviewed changes

akalash force-pushed the flink-25819 branch from 31fce89 to 4c8d8ff Compare February 23, 2022 16:44

akalash commented Feb 23, 2022

View reviewed changes

smattheis approved these changes Feb 24, 2022

View reviewed changes

dawidwys reviewed Feb 25, 2022

View reviewed changes

...-runtime/src/test/java/org/apache/flink/runtime/io/network/buffer/NetworkBufferPoolTest.java Outdated Show resolved Hide resolved

akalash added 3 commits February 25, 2022 12:08

[FLINK-25819][runtime] Reordered requesting and recycling buffers in …

2ca0d9c

…order to avoid race condition in testIsAvailableOrNotAfterRequestAndRecycleMultiSegments

[FLINK-25819][runtime] Added new test for 'Insufficient number of net…

f8d4183

…work buffers' scenario into NetworkBufferPoolTest

[hotfix][runtime] CodeStyle correction for NetworkBufferPoolTest

01e4300

akalash force-pushed the flink-25819 branch from 4c8d8ff to 01e4300 Compare February 25, 2022 11:13

dawidwys merged commit 44da40f into apache:master Feb 25, 2022

akalash mentioned this pull request Feb 25, 2022

[FLINK-25819][runtime] Reordered requesting and recycling buffers in order to avoid race condition in testIsAvailableOrNotAfterRequestAndRecycleMultiSegments #18924

Merged

akalash mentioned this pull request Mar 9, 2022

[FLINK-25819][runtime] Reordered requesting and recycling buffers in order to avoid race condition in testIsAvailableOrNotAfterRequestAndRecycleMultiSegments #19018

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-25819][runtime] Reordered requesting and recycling buffers in order to avoid race condition in testIsAvailableOrNotAfterRequestAndRecycleMultiSegments #18865

[FLINK-25819][runtime] Reordered requesting and recycling buffers in order to avoid race condition in testIsAvailableOrNotAfterRequestAndRecycleMultiSegments #18865

akalash commented Feb 21, 2022

akalash commented Feb 21, 2022

flinkbot commented Feb 21, 2022 •

edited

flinkbot commented Feb 21, 2022

akalash commented Feb 22, 2022

smattheis Feb 22, 2022

akalash Feb 23, 2022

smattheis Feb 22, 2022

smattheis Feb 22, 2022

akalash Feb 23, 2022

smattheis left a comment

akalash left a comment

akalash Feb 23, 2022

akalash Feb 23, 2022

smattheis left a comment

dawidwys Feb 25, 2022

akalash Feb 25, 2022

dawidwys Feb 25, 2022

dawidwys commented Feb 25, 2022

akalash commented Feb 25, 2022

[FLINK-25819][runtime] Reordered requesting and recycling buffers in order to avoid race condition in testIsAvailableOrNotAfterRequestAndRecycleMultiSegments #18865

[FLINK-25819][runtime] Reordered requesting and recycling buffers in order to avoid race condition in testIsAvailableOrNotAfterRequestAndRecycleMultiSegments #18865

Conversation

akalash commented Feb 21, 2022

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

akalash commented Feb 21, 2022

flinkbot commented Feb 21, 2022 • edited

CI report:

flinkbot commented Feb 21, 2022

Automated Checks

Review Progress

akalash commented Feb 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smattheis left a comment

Choose a reason for hiding this comment

akalash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smattheis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawidwys commented Feb 25, 2022

akalash commented Feb 25, 2022

flinkbot commented Feb 21, 2022 •

edited