Skip to content

Additional integration test checks#14328

Merged
arhimondr merged 3 commits intotrinodb:masterfrom
arhimondr:additional-integration-test-checks
Sep 29, 2022
Merged

Additional integration test checks#14328
arhimondr merged 3 commits intotrinodb:masterfrom
arhimondr:additional-integration-test-checks

Conversation

@arhimondr
Copy link
Copy Markdown
Contributor

Description

  • Ensure queries run by the AbstractTestQueryFramework have their final query info set and no tasks are left behind abandoned
  • Limit query execution thread pool size to detect potential thread leaks (if a query scheduler gets abandoned upon query completion)

Non-technical explanation

N/A

Release notes

(X) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@findepi
Copy link
Copy Markdown
Member

findepi commented Sep 28, 2022

Will it be not flaky as assertMemoryPoolReleased was?

Can you stress test on CI?

@arhimondr
Copy link
Copy Markdown
Contributor Author

@findepi I introduced these checks when working on #14205 and have done many runs. I haven't noticed any tests being flaky due to these additional checks. But there's always a chance for a rare race condition.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

there may be some tests which schedule queries in the background and do not necessarily wait for their completion. But we can address that if we see this becoming flaky.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: move to previous line

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When I move it to the previous line it doesn't pass the style check

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting. I have checkstyle set up in IntelliJ and it did not complain. 🤷

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, my Intellij doesn't complain either (though it is trying to format it with a new line when I try to autoformat). But then the check during the build fails.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: look unrelated - separate commit?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If the statement is not closed the query never finishes and doesn't receive a final TaskInfo making the newly introduced check fail

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still should be a preparatory commit. So adding checks and making project actually pass the checks are separate commits. (fixing makes sense even if we do not have checks).

@arhimondr arhimondr force-pushed the additional-integration-test-checks branch from 61a95e9 to 7e950e0 Compare September 28, 2022 21:05
@losipiuk
Copy link
Copy Markdown
Member

CI: #14368

Otherwise the query keeps being active after the tests is finished
Verify all queries report final query info and no tasks are abandoned
In tests further decrease thread pool size to detect potential thread
leaks
@arhimondr arhimondr force-pushed the additional-integration-test-checks branch from 7e950e0 to 33a9771 Compare September 29, 2022 14:23
@arhimondr arhimondr merged commit 6b48fd0 into trinodb:master Sep 29, 2022
@arhimondr arhimondr deleted the additional-integration-test-checks branch September 29, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants