Skip to content

Verify tests do not allocate resources early and do not leave them behind#15165

Merged
findepi merged 39 commits intomasterfrom
findepi/resourceful-tests
Nov 28, 2022
Merged

Verify tests do not allocate resources early and do not leave them behind#15165
findepi merged 39 commits intomasterfrom
findepi/resourceful-tests

Conversation

@findepi
Copy link
Copy Markdown
Member

@findepi findepi commented Nov 23, 2022

When running TestNG tests with surefire, the test class instance
initialization (class initializer, test instance construction) happens
before tests are run, for all instances. Allocating resources during
this phase is bad for these reasons:

  • resources are allocated long before they are needed, so tests may
    exhaust available memory even if every single test is not memory-hungry
  • failure reporting during this phase is imperfect (it was observed that
    actual failure stacktrace may not be reported in the logs), so
    diagnosing failures is hard.

This commit adds a runtime verification attempting to ensure that test
instances do not hold on to any "resourceful" classes before the test is
started. Of course, we don't want to prohibit any field initialization
in test instances (that would affect readability and would be
frustrating), so "resourceful" classes are some known ones. The list can
be extended in the future.

Besides checking test instances before test is run, it also checks
instances after test class is completed, to catch any resources that
were left behind and not closed.

Adds test coverage for #15133

Already approved at #15151

@cla-bot cla-bot Bot added the cla-signed label Nov 23, 2022
@findepi findepi mentioned this pull request Nov 23, 2022
Closed
@github-actions github-actions Bot added the jdbc Relates to Trino JDBC driver label Nov 23, 2022
@findepi findepi force-pushed the findepi/resourceful-tests branch 2 times, most recently from a5deb2a to f5524c0 Compare November 23, 2022 16:26
@findepi findepi force-pushed the findepi/resourceful-tests branch 2 times, most recently from cd9062e to 0f1693d Compare November 24, 2022 11:18
@findepi findepi mentioned this pull request Nov 24, 2022
Merged
@findepi findepi force-pushed the findepi/resourceful-tests branch 8 times, most recently from db97397 to 5a32f0c Compare November 25, 2022 10:17
@findepi findepi changed the title Verify tests do not allocate resources early Verify tests do not allocate resources early and do not leave them behind Nov 25, 2022
ksobolew
ksobolew reviewed Nov 25, 2022
View reviewed changes
Comment thread testing/trino-testing-services/src/main/java/io/trino/testng/services/Listeners.java Outdated
ksobolew
ksobolew reviewed Nov 25, 2022
View reviewed changes
Comment thread client/trino-jdbc/src/test/java/io/trino/jdbc/TestJdbcVendorCompatibility.java Outdated
@findepi findepi force-pushed the findepi/resourceful-tests branch from 5a32f0c to 6da68d1 Compare November 25, 2022 11:18
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Nov 25, 2022 •
edited
Loading

CI #14814 , #15195

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Nov 28, 2022

(just rebased)

@MiguelWeezardo
Copy link
Copy Markdown
Member

MiguelWeezardo commented Nov 28, 2022 •
edited
Loading 

 [check-commits-dispatcher (master, 7b4aba6c18a0e22ec7696b6a01e6e429cd1a5afb)](https://github.com/trinodb/trino/actions/runs/3555387076/jobs/5972063982#logs)
failed 2 days ago in 25s
Search logs
1s
24s
0s
Error: Can't find 'action.yml', 'action.yaml' or 'Dockerfile' under '/home/runner/work/trino/trino/.github/actions/compile-commit'. Did you forget to run actions/checkout before running your local action?

I probably beed to rebase? @nineinchnick @MiguelWeezardo

Sorry about that, I will try to fix this check (#15210) so that PRs opened before it was merged won't need to be rebased.

losipiuk
losipiuk reviewed Nov 28, 2022
View reviewed changes
Comment thread core/trino-main/src/test/java/io/trino/execution/TestSetSessionTask.java
losipiuk
losipiuk reviewed Nov 28, 2022
View reviewed changes
Comment thread core/trino-main/src/test/java/io/trino/execution/TestResetSessionTask.java Outdated
losipiuk
losipiuk reviewed Nov 28, 2022
View reviewed changes
Comment thread core/trino-main/src/test/java/io/trino/cost/TestStatsCalculator.java
losipiuk
losipiuk reviewed Nov 28, 2022
View reviewed changes
Comment thread core/trino-main/src/test/java/io/trino/execution/TestMemoryRevokingScheduler.java
{
queryContexts.clear();
memoryPool = null;
taskExecutor.stop();
Copy link
Copy Markdown
Member

@losipiuk losipiuk Nov 28, 2022

Choose a reason for hiding this comment

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

should we use closer here to make sure all cleanup routines are called?

Copy link
Copy Markdown
Member Author

@findepi findepi Nov 28, 2022

Choose a reason for hiding this comment

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

I don't see a need to. @AfterClass failure is a failure for tests execution so we won't ignore it.

losipiuk
losipiuk reviewed Nov 28, 2022
View reviewed changes
Comment thread core/trino-main/src/test/java/io/trino/execution/TestSqlTaskManager.java
losipiuk
losipiuk reviewed Nov 28, 2022
View reviewed changes
Comment thread core/trino-main/src/test/java/io/trino/execution/TestSqlTask.java
losipiuk
losipiuk approved these changes Nov 28, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

I agree this is somewhat fragile - but at the same time it proves helpful in current shape. If we notice logic in verifier class deteriorates over time we can either drop it or make it more up-to-date.

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Nov 28, 2022

I agree this is somewhat fragile - but at the same time it proves helpful in current shape. If we notice logic in verifier class deteriorates over time we can either drop it or make it more up-to-date.

agreed

@findepi
Free TestingTrinoServer and more instances in tests
8233aa4 
Ever after `TestingTrinoServer.close()` is called, the
`TestingTrinoServer` may hold on to quite some memory. Let's clear
references to these upon test completion.

This opportunistically includes freeing of a few more things that are of
lesser importance.
@findepi
Fix @AfterClass to include alwaysRun = true
1eae356
@findepi
Prevent use of closeAfterClass in a test constructor
fc0ad15 
It's never a good idea, so make it this fail cleanly.

As an added bonus, it prevents use of `closeAfterClass` after closing
was called. That could lead to resources being left behind.
@findepi
Verify tests do not allocate resources early
420bafa 
When running TestNG tests with surefire, the test class instance
initialization (class initializer, test instance construction) happens
before tests are run, for all instances. Allocating resources during
this phase is bad for these reasons:

- resources are allocated long before they are needed, so tests may
  exhaust available memory even if every single test is not memory-hungry
- failure reporting during this phase is imperfect (it was observed that
  actual failure stacktrace may not be reported in the logs), so
  diagnosing failures is hard.

This commit adds a runtime verification attempting to ensure that test
instances do not hold on to any "resourceful" classes before the test is
started. Of course, we don't want to prohibit any field initialization
in test instances (that would affect readability and would be
frustrating), so "resourceful" classes are some known ones. The list can
be extended in the future.

Besides checking test instances before test is run, it also checks
instances after test class is completed, to catch any resources that
were left behind and not closed.
@findepi
Log configuration methods too in ProgressLoggingListener
3826cd4
@findepi
Release memory in DistributedQueryRunner.close
08b24b6 
This also makes `DistributedQueryRunner.close` explicitly idempotent.
Indeed, some tests call it twice, for example once explicitly and once
via `QueryAssertions.close`.
@findepi
Fix field declaration placement in TestSetSessionTask
aa0e114
@findepi findepi force-pushed the findepi/resourceful-tests branch from 714c78c to aa0e114 Compare November 28, 2022 13:21
@findepi findepi merged commit 5d9024c into master Nov 28, 2022
@findepi findepi deleted the findepi/resourceful-tests branch November 28, 2022 13:22
This was referenced Nov 28, 2022
Closed
Closed
@github-actions github-actions Bot added this to the 404 milestone Nov 28, 2022
@colebow colebow mentioned this pull request Nov 28, 2022
Merged
@kokosing
Copy link
Copy Markdown
Member

kokosing commented Dec 7, 2022

@findepi Thank you for doing this! This is great!

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Dec 9, 2022

@kokosing with all due modesty, i think i agree!

@findepi findepi mentioned this pull request Dec 20, 2022
Merged
@jitheshtr jitheshtr mentioned this pull request Jan 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martint martint martint left review comments

@losipiuk losipiuk losipiuk approved these changes

@MiguelWeezardo MiguelWeezardo Awaiting requested review from MiguelWeezardo

@ebyhr ebyhr Awaiting requested review from ebyhr

@hashhar hashhar Awaiting requested review from hashhar

+2 more reviewers

@ksobolew ksobolew ksobolew left review comments

@nineinchnick nineinchnick nineinchnick approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed jdbc Relates to Trino JDBC driver

Milestone

405

Development

Successfully merging this pull request may close these issues.

7 participants

@findepi @MiguelWeezardo @kokosing @martint @nineinchnick @losipiuk @ksobolew