Do not allocate resources within test constructor

[v-jizhang]

Cherry-pick of trinodb/trino#2412

Co-authored-by: Piotr Findeisen findepi@users.noreply.github.com

Test plan - Make sure all CI tests pass.

Depends on https://github.com/facebookexternal/presto-facebook/pull/1542

== RELEASE NOTES ==
General Changes
* Do not allocate resources within test constructor for a cleaner code.

[findepi]

Co-authored-by: Piotr Findeisen findepi@users.noreply.github.com

this is the deprcated form of noreply GH emails
please copy my email from original commit, or at least use the current form (with a dash and number)

[v-jizhang]

Sure. Thanks

[aweisberg]

This doesn't make use of closeAfterClass which is a nice refactor, but lets say that is for later. This also still has some constructors that allocate resources/things which is avoided in several instances in Trino.

Still I wouldn't block on that since it is forward progress and the PR is already broken because it hasn't landed. Would like to get the API change in.

I don't understand why this is necessary both in and of itself (is it just for cleanliness and safe constructors?) and specifically as a dependency.

Since I rebased it doesn't build on some tests.

[aweisberg]

This is still creating a resource in the constructor. It also happens in 2-3 places above.

[v-jizhang]

I'll move it to @BeforeVlass

[v-jizhang]

That won't work because the resource is required in createdQueryRunner and it is called before @BeforeClass. I think the resource created in constructor is Ok here, we don't pass it to super class.

[aweisberg]

Unintended indentation?

[v-jizhang]

The original indentation was wrong.

[v-jizhang]

This doesn't make use of closeAfterClass which is a nice refactor, but lets say that is for later. This also still has some constructors that allocate resources/things which is avoided in several instances in Trino.

Still I wouldn't block on that since it is forward progress and the PR is already broken because it hasn't landed. Would like to get the API change in.

I don't understand why this is necessary both in and of itself (is it just for cleanliness and safe constructors?) and specifically as a dependency.

Since I rebased it doesn't build on some tests.

It's not just for cleanliness and safe constructor, it's a dependency because super class of TestElasticsearchIntegrationSmokeTest needs a QueryRunner created by EmbeddedElasticsearchNode. We can adapt it but I think it's better to remove the resource from constructors.

[aweisberg]

LGTM, TY!