Skip to content

Conversation

@hantangwangd
Copy link
Member

@hantangwangd hantangwangd commented Mar 20, 2025

Description

This PR fix flaky test: TestMemoryRevokingScheduler.testTaskRevokingOrderForRevocableBytes.

The operation LocalMemoryContext.setBytes(bytes) will trigger MemoryRevokingScheduler.onMemoryReserved(...), which will then try to request memory revoking asynchronously in thread pool memoryRevocationExecutor.

Considering this, the following code in this flaky test:

operatorContext1.localRevocableMemoryContext().setBytes(11);
operatorContext2.localRevocableMemoryContext().setBytes(12);

May have different sequences of actions:

operator1 reserve 11 bytes
operator2 reserve 12 bytes
request memory revoking(asynchronously)
request memory revoking(asynchronously)
......

Or:

operator1 reserve 11 bytes
request memory revoking(asynchronously)
operator2 reserve 12 bytes
request memory revoking(asynchronously)

Although the first scenario may occur in most cases because asynchronous calls take much longer than sequential execution, but we cannot rely on it. When the second scenario occur, the test will fail. To make it more apparent, we can simply add a Thread.sleep(10) between the two lines.

This PR figure out a way to ensure that there will definitely no memory revoking between the two memory reserving actions.

Motivation and Context

Fix: #24691

Impact

N/A

Test Plan

  • Add from Thread.sleep(10) to Thread.sleep(10000) between the two memory revoking lines in testTaskRevokingOrderForRevocableBytes, manually make sure that it works well.

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

If release note is NOT required, use:

== NO RELEASE NOTE ==

Copy link
Contributor

@rschlussel rschlussel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes look good. a comment would be helpful for anyone looking at this in the future.


scheduler.awaitAsynchronousCallbacksRun();
assertMemoryRevokingNotRequested();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment explaining what's going on here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rschlussel Thanks for your review. Sure, I have added some comments to explain this fix, please take a look when convenient.

@hantangwangd hantangwangd merged commit d5c285e into prestodb:master Mar 28, 2025
94 checks passed
@hantangwangd hantangwangd deleted the fix_flaky_test branch March 28, 2025 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TestMemoryRevokingScheduler.testTaskRevokingOrderForRevocableBytes flakes

2 participants