Skip to content
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

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

Merged
merged 39 commits into from
Nov 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
d69f190
Remove redundant null check
findepi Nov 22, 2022
dc2e26a
Remove redundant closeAfterClass call
findepi Nov 24, 2022
26470f1
Use try-with-resources in TestHiveTransactionalTable
findepi Nov 25, 2022
acc98ce
Annotate reportListenerFailure as `@FormatMethod`
findepi Nov 22, 2022
07a85f4
Refactor TestLdapAuthenticator cleanup
findepi Nov 23, 2022
c1f5f82
Increase robustness of TestJdbcVendorCompatibility cleanup
findepi Nov 25, 2022
b3d4777
Fix indentation
findepi Nov 23, 2022
b5ede8b
Reorder methods for readability
findepi Nov 23, 2022
3a4d930
Fix resource leak in TestAnonymizeJsonRepresentation
findepi Nov 23, 2022
c000675
Fix resource leak in TestJsonRepresentation
findepi Nov 23, 2022
5e55f2f
Fix resource leak in TestSetSessionTask
findepi Nov 23, 2022
2ec7ae4
Fix resource leak in TestResetSessionTask
findepi Nov 23, 2022
dc29639
Fix resource leak in TestSetTimeZoneTask
findepi Nov 23, 2022
5f0fcab
Fix resource leak in TestStatsCalculator
findepi Nov 23, 2022
3853550
Fix resource leak in TestValidateScaledWritersUsage
findepi Nov 23, 2022
a244669
Fix resource leak in TestMemoryRevokingScheduler
findepi Nov 23, 2022
8d820a8
Fix resource leak in TestIcebergGetTableStatisticsOperations
findepi Nov 24, 2022
3841830
Fix resource leak in BaseJdbcConnectionCreationTest extenders
findepi Nov 24, 2022
370c458
Fix resource leak in TestPrometheusSplit
findepi Nov 24, 2022
75ca786
Fix resource leak in TestDruidTypeMapping
findepi Nov 24, 2022
6ae6b93
Fix resource leaks in Kudu connector tests
findepi Nov 24, 2022
bad7a65
Fix resource leak in TestGetTableStatisticsOperations
findepi Nov 24, 2022
819d2ac
Fix resource leak in TestPrometheusQueryMatrixResponseParse
findepi Nov 25, 2022
dcce266
Fix resource leak in TestPrometheusQueryScalarResponseParse
findepi Nov 25, 2022
5dbb14b
Fix resource leak in TestPrometheusQueryVectorResponseParse
findepi Nov 25, 2022
a8f3df7
Fix resource leak in TestIcebergSparkCompatibility
findepi Nov 25, 2022
d176c45
Fix CassandraSession resource leak in tests
findepi Nov 24, 2022
df2b3a7
Delay resource allocation in TestSqlTaskManager
findepi Nov 23, 2022
ad44778
Delay resource allocation in TestSqlTask
findepi Nov 23, 2022
9aab8be
Delay initialization in TestDeltaLakeGcsConnectorSmokeTest
findepi Nov 23, 2022
83a906c
Delay resource allocation in TestIcebergGcsConnectorSmokeTest
findepi Nov 23, 2022
f3e962e
Ensure TaskExecutor gets closed
findepi Nov 23, 2022
8233aa4
Free TestingTrinoServer and more instances in tests
findepi Nov 23, 2022
1eae356
Fix `@AfterClass` to include `alwaysRun = true`
findepi Nov 24, 2022
fc0ad15
Prevent use of closeAfterClass in a test constructor
findepi Nov 24, 2022
420bafa
Verify tests do not allocate resources early
findepi Nov 22, 2022
3826cd4
Log configuration methods too in ProgressLoggingListener
findepi Nov 25, 2022
08b24b6
Release memory in DistributedQueryRunner.close
findepi Nov 25, 2022
aa0e114
Fix field declaration placement in TestSetSessionTask
findepi Nov 28, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ public void teardown()
throws Exception
{
server.close();
server = null;
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public void teardown()
throws IOException
{
server.close();
server = null;
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ public void tearDown()
closeAll(
server,
executor::shutdownNow);
server = null;
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ public void tearDown()
throws Exception
{
server.close();
server = null;
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ public void tearDownServer()
throws Exception
{
server.close();
server = null;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ public void tearDown()
closeAll(
server,
executor::shutdownNow);
server = null;
}

@Test(timeOut = 60_000)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@
package io.trino.jdbc;

import com.google.common.collect.ImmutableList;
import com.google.common.io.Closer;
import io.airlift.log.Logger;
import io.airlift.log.Logging;
import io.trino.server.testing.TestingTrinoServer;
import io.trino.util.AutoCloseableCloser;
import oracle.jdbc.OracleType;
import org.testcontainers.containers.OracleContainer;
import org.testcontainers.containers.PostgreSQLContainer;
Expand Down Expand Up @@ -88,16 +88,34 @@ public void setupServer()
Logging.initialize();
log = Logger.get(TestJdbcVendorCompatibility.class);
server = TestingTrinoServer.create();
referenceDrivers = ImmutableList.of(new PostgresqlReferenceDriver(), new OracleReferenceDriver());

// Capture resources as soon as they are allocated. Ensure all allocate resources are cleaned up even if e.g. the last one fails to start.
referenceDrivers = new ArrayList<>();
referenceDrivers.add(new PostgresqlReferenceDriver());
referenceDrivers.add(new OracleReferenceDriver());
findepi marked this conversation as resolved.
Show resolved Hide resolved
}

@AfterClass(alwaysRun = true)
public void tearDownServer()
throws Exception
{
try (Closer closer = Closer.create()) {
referenceDrivers.forEach(closer::register);
closer.register(server);
try (AutoCloseableCloser closer = AutoCloseableCloser.create()) {
if (referenceDrivers != null) {
referenceDrivers.forEach(closer::register);
referenceDrivers.clear();
}
if (server != null) {
closer.register(server);
server = null;
}
if (connection != null) {
closer.register(connection);
connection = null;
}
if (statement != null) {
closer.register(statement);
statement = null;
}
}
}

Expand All @@ -116,8 +134,14 @@ public void setUp()
public void tearDown()
throws Exception
{
statement.close();
connection.close();
if (statement != null) {
statement.close();
statement = null;
}
if (connection != null) {
connection.close();
connection = null;
}
for (ReferenceDriver driver : referenceDrivers) {
try {
driver.tearDown();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ public void tearDownServer()
throws Exception
{
server.close();
server = null;
}

@SuppressWarnings("JDBCResourceOpenedButNotSafelyClosed")
Expand All @@ -117,8 +118,11 @@ public void teardown()
throws Exception
{
executor.shutdownNow();
executor = null;
statement.close();
statement = null;
connection.close();
connection = null;
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ public void teardown()
throws IOException
{
server.close();
server = null;
}

private List<String> createResults()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ public void tearDown()
throws Exception
{
connection.close();
connection = null;
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ public void teardown()
executorService.shutdownNow();
executorService = null;
server.close();
server = null;
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ public void teardown()
throws Exception
{
server.close();
server = null;
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ public void teardown()
throws Exception
{
server.close();
server = null;
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import io.trino.sql.planner.assertions.PlanMatchPattern;
import io.trino.sql.planner.plan.TableScanNode;
import io.trino.testing.LocalQueryRunner;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;

import static io.trino.sql.planner.LogicalPlanner.Stage.OPTIMIZED_AND_VALIDATED;
Expand All @@ -32,11 +34,12 @@

public class TestStatsCalculator
{
private final LocalQueryRunner queryRunner;
private LocalQueryRunner queryRunner;

public TestStatsCalculator()
@BeforeClass
public void setUp()
{
this.queryRunner = LocalQueryRunner.create(testSessionBuilder()
queryRunner = LocalQueryRunner.create(testSessionBuilder()
findepi marked this conversation as resolved.
Show resolved Hide resolved
.setCatalog(TEST_CATALOG_NAME)
.setSchema("tiny")
.setSystemProperty("task_concurrency", "1") // these tests don't handle exchanges from local parallel
Expand All @@ -48,6 +51,13 @@ public TestStatsCalculator()
ImmutableMap.of());
}

@AfterClass(alwaysRun = true)
public void tearDown()
{
queryRunner.close();
queryRunner = null;
}

@Test
public void testStatsCalculatorUsesLayout()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,14 @@ public void tearDown()
{
if (queryRunner != null) {
queryRunner.close();
queryRunner = null;
}
testSession = null;
metadata = null;
plannerContext = null;
materializedViewPropertyManager = null;
transactionManager = null;
queryStateMachine = null;
}

protected static QualifiedObjectName qualifiedObjectName(String objectName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public void close()
{
if (queryRunner != null) {
queryRunner.close();
queryRunner = null;
}
executor.shutdownNow();
executor = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,15 @@ public void tearDown()
if (queryRunner != null) {
queryRunner.close();
}
testSession = null;
metadata = null;
plannerContext = null;
transactionManager = null;
parser = null;
queryStateMachine = null;
analyzerFactory = null;
materializedViewPropertyManager = null;
queryRunner = null;
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,12 @@ public void tearDown()
if (queryRunner != null) {
queryRunner.close();
}
queryRunner = null;
transactionManager = null;
tablePropertyManager = null;
columnPropertyManager = null;
metadata = null;
plannerContext = null;
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,11 @@ public class TestMemoryRevokingScheduler
private final SpillSpaceTracker spillSpaceTracker = new SpillSpaceTracker(DataSize.of(10, GIGABYTE));
private final Map<QueryId, QueryContext> queryContexts = new HashMap<>();

private MemoryPool memoryPool;
private TaskExecutor taskExecutor;
private ScheduledExecutorService executor;
private ScheduledExecutorService scheduledExecutor;
private SqlTaskExecutionFactory sqlTaskExecutionFactory;
private MemoryPool memoryPool;

private Set<OperatorContext> allOperatorContexts;

Expand All @@ -83,7 +84,7 @@ public void setUp()
{
memoryPool = new MemoryPool(DataSize.ofBytes(10));

TaskExecutor taskExecutor = new TaskExecutor(8, 16, 3, 4, Ticker.systemTicker());
taskExecutor = new TaskExecutor(8, 16, 3, 4, Ticker.systemTicker());
taskExecutor.start();

// Must be single threaded
Expand All @@ -107,8 +108,12 @@ public void tearDown()
{
queryContexts.clear();
memoryPool = null;
taskExecutor.stop();
Copy link
Member

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
Member Author

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.

taskExecutor = null;
executor.shutdownNow();
scheduledExecutor.shutdownNow();
sqlTaskExecutionFactory = null;
allOperatorContexts = null;
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ public void setUp()
public void tearDown()
{
queryRunner.close();
queryRunner = null;
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import io.trino.testing.LocalQueryRunner;
import io.trino.transaction.TransactionManager;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;

import java.net.URI;
Expand All @@ -48,14 +49,16 @@ public class TestResetSessionTask
{
private static final String CATALOG_NAME = "my_catalog";
private ExecutorService executor = newCachedThreadPool(daemonThreadsNamed(getClass().getSimpleName() + "-%s"));
private final TransactionManager transactionManager;
private final AccessControl accessControl;
private final Metadata metadata;
private final SessionPropertyManager sessionPropertyManager;
private LocalQueryRunner queryRunner;
private TransactionManager transactionManager;
private AccessControl accessControl;
private Metadata metadata;
private SessionPropertyManager sessionPropertyManager;

public TestResetSessionTask()
@BeforeClass
public void setUp()
{
LocalQueryRunner queryRunner = LocalQueryRunner.builder(TEST_SESSION)
queryRunner = LocalQueryRunner.builder(TEST_SESSION)
.withExtraSystemSessionProperties(ImmutableSet.of(() -> ImmutableList.of(
stringProperty(
"foo",
Expand Down Expand Up @@ -86,6 +89,12 @@ public void tearDown()
{
executor.shutdownNow();
executor = null;
queryRunner.close();
queryRunner = null;
transactionManager = null;
accessControl = null;
metadata = null;
sessionPropertyManager = null;
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import io.trino.sql.tree.SetPath;
import io.trino.transaction.TransactionManager;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;

import java.net.URI;
Expand All @@ -44,13 +45,14 @@

public class TestSetPathTask
{
private final TransactionManager transactionManager;
private final AccessControl accessControl;
private final Metadata metadata;
private TransactionManager transactionManager;
private AccessControl accessControl;
private Metadata metadata;

private ExecutorService executor = newCachedThreadPool(daemonThreadsNamed(getClass().getSimpleName() + "-%s"));

public TestSetPathTask()
@BeforeClass
public void setUp()
{
transactionManager = createTestTransactionManager();
accessControl = new AllowAllAccessControl();
Expand All @@ -65,6 +67,9 @@ public void tearDown()
{
executor.shutdownNow();
executor = null;
transactionManager = null;
accessControl = null;
metadata = null;
}

@Test
Expand Down
Loading