-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor TestMemoryManager.testOutOfMemoryKiller #11818
Refactor TestMemoryManager.testOutOfMemoryKiller #11818
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx
If one query finishes successfully, give another query a chance to fail.
`assertNotNull` is redundant since we also do `assertEquals`.
22cc8e3
to
f041992
Compare
It was observed on CI that test query may complete successfully. This may be because OOM killer has 5s minimal delay.
Within the test, one query should get killed, but the other may not be killed and run to completion. If the first submitted query happens to run successfully, the test takes longer to complete. Inspect queries in order of their completion, not submission.
f041992
to
8eca03d
Compare
@losipiuk thanks for review. i added |
@@ -115,6 +116,12 @@ public void testOutOfMemoryKiller() | |||
.buildOrThrow(); | |||
|
|||
try (DistributedQueryRunner queryRunner = createQueryRunner(TINY_SESSION, properties)) { | |||
queryRunner.installPlugin(new BlackHolePlugin()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be because OOM killer has 5s minimal delay
This will be dropped (as does not play well with task-level retries. (part of #11800).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the test be updated to use smaller delay? This could make the test run shorter.
Still, the change here should remain. If query can succeed in low number of seconds, this feels racy.
This probably doesn't fix the #11016maybe fixes #11016