Copilot commented on code in PR #17855:
URL: https://github.com/apache/pinot/pull/17855#discussion_r2921423091
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/RetentionManagerIntegrationTest.java:
##########
@@ -95,6 +97,21 @@ protected void setupTable()
waitForDocsLoaded(600_000L, true, tableConfig.getTableName());
}
+ @AfterClass
+ public void tearDown()
+ throws Exception {
+ try {
+ dropRealtimeTable(getTableName());
+ stopServer();
+ stopBroker();
+ stopController();
+ stopKafka();
+ stopZk();
+ } finally {
+ FileUtils.deleteQuietly(_tempDir);
+ }
Review Comment:
In this teardown, only the temp dir deletion is guaranteed via finally. If
dropRealtimeTable() (or any subsequent stop call) throws, the remaining stop*
calls won’t run, which can leave ZK/Kafka/Pinot processes running and interfere
with other integration tests. Consider doing best-effort shutdown in a finally
block (e.g., stop components in finally and suppress/aggregate exceptions), and
keep table-drop failures from preventing cluster shutdown.
```suggestion
Exception firstException = null;
try {
try {
dropRealtimeTable(getTableName());
} catch (Exception e) {
firstException = e;
LOGGER.warn("Failed to drop realtime table during teardown", e);
}
} finally {
try {
stopServer();
} catch (Exception e) {
if (firstException == null) {
firstException = e;
}
LOGGER.warn("Failed to stop server during teardown", e);
}
try {
stopBroker();
} catch (Exception e) {
if (firstException == null) {
firstException = e;
}
LOGGER.warn("Failed to stop broker during teardown", e);
}
try {
stopController();
} catch (Exception e) {
if (firstException == null) {
firstException = e;
}
LOGGER.warn("Failed to stop controller during teardown", e);
}
try {
stopKafka();
} catch (Exception e) {
if (firstException == null) {
firstException = e;
}
LOGGER.warn("Failed to stop Kafka during teardown", e);
}
try {
stopZk();
} catch (Exception e) {
if (firstException == null) {
firstException = e;
}
LOGGER.warn("Failed to stop Zookeeper during teardown", e);
}
FileUtils.deleteQuietly(_tempDir);
}
if (firstException != null) {
throw firstException;
}
```
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/logicaltable/BaseLogicalTableIntegrationTest.java:
##########
@@ -94,13 +94,13 @@ public void setUpSuite()
public void tearDownSuite()
throws Exception {
LOGGER.info("Tearing down integration test suite");
- // Stop Kafka
- LOGGER.info("Stop Kafka in the integration test suite");
- stopKafka();
// Shutdown the Pinot cluster
stopServer();
stopBroker();
stopController();
+ // Stop Kafka
+ LOGGER.info("Stop Kafka in the integration test suite");
+ stopKafka();
stopZk();
FileUtils.deleteDirectory(_tempDir);
LOGGER.info("Finished tearing down integration test suite");
Review Comment:
tearDownSuite() can exit early if any stop* call throws (e.g.,
stopServer/stopBroker/stopController), which can leave Kafka/ZK running for the
remainder of the suite and impact other tests. Since this method was modified,
consider a try/finally with best-effort stops (suppressing exceptions) before
deleting the temp dir.
```suggestion
try {
// Shutdown the Pinot cluster
try {
stopServer();
} catch (Exception e) {
LOGGER.warn("Failed to stop servers during integration test suite
teardown", e);
}
try {
stopBroker();
} catch (Exception e) {
LOGGER.warn("Failed to stop broker during integration test suite
teardown", e);
}
try {
stopController();
} catch (Exception e) {
LOGGER.warn("Failed to stop controller during integration test suite
teardown", e);
}
// Stop Kafka
LOGGER.info("Stop Kafka in the integration test suite");
try {
stopKafka();
} catch (Exception e) {
LOGGER.warn("Failed to stop Kafka during integration test suite
teardown", e);
}
try {
stopZk();
} catch (Exception e) {
LOGGER.warn("Failed to stop Zookeeper during integration test suite
teardown", e);
}
} finally {
try {
FileUtils.deleteDirectory(_tempDir);
} catch (IOException e) {
LOGGER.warn("Failed to delete temp directory during integration test
suite teardown: {}", _tempDir, e);
}
LOGGER.info("Finished tearing down integration test suite");
}
```
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/QueryWorkloadIntegrationTest.java:
##########
@@ -115,6 +117,22 @@ public void setUp()
waitForAllDocsLoaded(100_000L);
}
+ @AfterClass
+ public void tearDown()
+ throws Exception {
+ try {
+ dropOfflineTable(getTableName());
+ dropRealtimeTable(getTableName());
+ stopServer();
+ stopBroker();
Review Comment:
This teardown wraps everything in one try block, so if
dropOfflineTable()/dropRealtimeTable() throws, the subsequent
stopServer/stopBroker/stopController/stopKafka/stopZk calls will be skipped and
services may leak into other tests. Consider restructuring so the cluster
shutdown happens in finally (with best-effort stops), and handle table-drop
errors separately.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/StaleSegmentCheckIntegrationTest.java:
##########
@@ -193,6 +193,7 @@ public void tearDown() {
stopServer();
stopBroker();
stopController();
Review Comment:
In this teardown, stop* calls run inside the try body; if
stopMinion()/stopServer()/etc throws, remaining shutdown steps will be skipped
(only temp dir cleanup is guaranteed). For best isolation between integration
tests, consider moving the stop* calls into finally with best-effort cleanup
and suppressing exceptions so teardown continues.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/CLPEncodingRealtimeIntegrationTest.java:
##########
@@ -103,6 +104,21 @@ public void testValues()
.getLong(0), 53);
}
+ @AfterClass
+ public void tearDown()
+ throws Exception {
+ try {
+ dropRealtimeTable(getTableName());
+ stopServer();
+ stopBroker();
+ stopController();
Review Comment:
Similar to other tearDown updates: because the stop calls are inside the try
body, any exception from dropRealtimeTable() or an earlier stop will prevent
the rest of the shutdown sequence from running. To avoid leaking Kafka/ZK/Pinot
between tests, consider making the stop* calls best-effort in a finally block
and suppressing exceptions rather than short-circuiting.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ExactlyOnceKafkaRealtimeClusterIntegrationTest.java:
##########
@@ -270,7 +278,7 @@ private int countRecords(String brokerList, String
isolationLevel) {
totalRecords += partitionRecords;
}
} catch (Exception e) {
- System.err.println("[ExactlyOnce] Error counting records with " +
isolationLevel + ": " + e.getMessage());
+ LOGGER.error("Error counting records with {}: {}", isolationLevel,
e.getMessage());
Review Comment:
The error log drops the exception stack trace by only logging
e.getMessage(). Including the Throwable as the last parameter will preserve the
full stack trace, which is especially useful for diagnosing flaky integration
test failures.
```suggestion
LOGGER.error("Error counting records with {}", isolationLevel, e);
```
##########
pinot-connectors/pinot-flink-connector/src/test/java/org/apache/pinot/connector/flink/sink/PinotSinkUpsertTableIntegrationTest.java:
##########
@@ -223,6 +224,22 @@ private TableConfig setupTable(String tableName, String
kafkaTopicName, String i
return tableConfig;
}
+ @AfterClass
+ public void tearDown()
+ throws Exception {
+ try {
+ dropRealtimeTable(getTableName());
+ stopMinion();
+ stopServer();
+ stopBroker();
+ stopController();
+ stopKafka();
+ stopZk();
+ } finally {
+ FileUtils.deleteQuietly(_tempDir);
+ }
Review Comment:
Because dropRealtimeTable() and the stop* calls are all in the try body, any
exception from the drop (or an early stop) will skip the remaining shutdown
steps and can leak running components into subsequent tests. Consider moving
the stopMinion/stopServer/stopBroker/stopController/stopKafka/stopZk sequence
into finally (best-effort) and treating table drop as a separate, non-blocking
cleanup step.
```suggestion
Exception tearDownException = null;
try {
dropRealtimeTable(getTableName());
} catch (Exception e) {
tearDownException = e;
} finally {
try {
stopMinion();
} catch (Exception e) {
if (tearDownException == null) {
tearDownException = e;
}
}
try {
stopServer();
} catch (Exception e) {
if (tearDownException == null) {
tearDownException = e;
}
}
try {
stopBroker();
} catch (Exception e) {
if (tearDownException == null) {
tearDownException = e;
}
}
try {
stopController();
} catch (Exception e) {
if (tearDownException == null) {
tearDownException = e;
}
}
try {
stopKafka();
} catch (Exception e) {
if (tearDownException == null) {
tearDownException = e;
}
}
try {
stopZk();
} catch (Exception e) {
if (tearDownException == null) {
tearDownException = e;
}
}
FileUtils.deleteQuietly(_tempDir);
}
if (tearDownException != null) {
throw tearDownException;
}
```
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BrokerQueryLimitTest.java:
##########
@@ -167,18 +167,16 @@ public void setUp()
public void tearDown()
throws Exception {
LOGGER.warn("Tearing down integration test class: {}",
getClass().getSimpleName());
- dropOfflineTable(getTableName());
- FileUtils.deleteDirectory(_tempDir);
-
- // Stop Kafka
- LOGGER.warn("Stop Kafka in the integration test class");
- stopKafka();
- // Shutdown the Pinot cluster
- stopServer();
- stopBroker();
- stopController();
- stopZk();
- FileUtils.deleteDirectory(_tempDir);
+ try {
+ dropOfflineTable(getTableName());
+ stopServer();
+ stopBroker();
+ stopController();
+ stopKafka();
+ stopZk();
+ } finally {
Review Comment:
The shutdown sequence is inside the try body, so if dropOfflineTable()
throws, the subsequent stopServer/stopBroker/stopController/stopKafka/stopZk
calls won’t run. Consider ensuring the stop* calls execute in finally as
best-effort cleanup (and optionally suppress/aggregate exceptions), so table
drop failures don’t leave the cluster running.
```suggestion
} finally {
stopServer();
stopBroker();
stopController();
stopKafka();
stopZk();
```
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/CustomDataQueryClusterIntegrationTest.java:
##########
@@ -85,13 +85,13 @@ public void setUpSuite()
public void tearDownSuite()
throws Exception {
LOGGER.warn("Tearing down integration test suite");
- // Stop Kafka
- LOGGER.warn("Stop Kafka in the integration test suite");
- stopKafka();
// Shutdown the Pinot cluster
stopServer();
stopBroker();
stopController();
+ // Stop Kafka
+ LOGGER.warn("Stop Kafka in the integration test suite");
+ stopKafka();
stopZk();
FileUtils.deleteDirectory(_tempDir);
LOGGER.warn("Finished tearing down integration test suite");
Review Comment:
tearDownSuite() runs stop calls sequentially without a finally/best-effort
cleanup pattern, so an exception from stopServer/stopBroker/stopController can
skip stopping Kafka/ZK and skip temp-dir deletion. Consider wrapping shutdown
in try/finally and stopping remaining components even if earlier stops fail
(collecting/suppressing exceptions).
```suggestion
List<Exception> exceptions = new ArrayList<>();
// Shutdown the Pinot cluster
try {
stopServer();
} catch (Exception e) {
exceptions.add(e);
LOGGER.warn("Failed to stop server during integration test suite
teardown: {}", e.toString());
}
try {
stopBroker();
} catch (Exception e) {
exceptions.add(e);
LOGGER.warn("Failed to stop broker during integration test suite
teardown: {}", e.toString());
}
try {
stopController();
} catch (Exception e) {
exceptions.add(e);
LOGGER.warn("Failed to stop controller during integration test suite
teardown: {}", e.toString());
}
// Stop Kafka
LOGGER.warn("Stop Kafka in the integration test suite");
try {
stopKafka();
} catch (Exception e) {
exceptions.add(e);
LOGGER.warn("Failed to stop Kafka during integration test suite
teardown: {}", e.toString());
}
try {
stopZk();
} catch (Exception e) {
exceptions.add(e);
LOGGER.warn("Failed to stop Zookeeper during integration test suite
teardown: {}", e.toString());
}
try {
FileUtils.deleteDirectory(_tempDir);
} catch (IOException e) {
exceptions.add(e);
LOGGER.warn("Failed to delete temporary directory '{}' during
integration test suite teardown: {}",
_tempDir, e.toString());
}
LOGGER.warn("Finished tearing down integration test suite");
if (!exceptions.isEmpty()) {
Exception first = exceptions.get(0);
for (int i = 1; i < exceptions.size(); i++) {
first.addSuppressed(exceptions.get(i));
}
throw first;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
