Copilot commented on code in PR #17855:
URL: https://github.com/apache/pinot/pull/17855#discussion_r2921602329
##########
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();
+ stopController();
Review Comment:
In this teardown, table drops and cluster shutdown are in the same `try`
block. If either `dropOfflineTable(...)` or `dropRealtimeTable(...)` throws,
the subsequent `stop*()` calls won’t execute, which can leak resources into
other integration tests. Consider making drops best-effort (catch/log) and
ensuring the stop sequence runs in a `finally` (or nested `try/finally`).
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ExactlyOnceKafkaRealtimeClusterIntegrationTest.java:
##########
@@ -94,9 +104,8 @@ protected long getDocsLoadedTimeoutMs() {
protected void pushAvroIntoKafka(List<File> avroFiles)
throws Exception {
String kafkaBrokerList = getKafkaBrokerList();
- // Use System.err for diagnostics - log4j2 console appender is filtered to
ERROR in CI
- System.err.println("[ExactlyOnce] Pushing transactional data to Kafka at:
" + kafkaBrokerList);
- System.err.println("[ExactlyOnce] Avro files count: " + avroFiles.size());
+ LOGGER.info("Pushing transactional data to Kafka at: {}", kafkaBrokerList);
+ LOGGER.info("Avro files count: {}", avroFiles.size());
Review Comment:
These diagnostic messages were changed from `System.err` to `LOGGER.info`,
but `pinot-integration-tests/src/test/resources/log4j2-test.xml` config sets
`org.apache.pinot` to WARN and root to ERROR, so INFO logs from this test won’t
show up in CI output. Consider logging these diagnostics at WARN (or ERROR only
on failure) so they remain visible when the test flakes/fails.
```suggestion
LOGGER.warn("Avro files count: {}", avroFiles.size());
```
##########
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();
Review Comment:
In this teardown, the table drop and the `stop*()` calls share the same
`try` block. If `dropRealtimeTable(...)` throws, the stop sequence won’t run,
which can leak integration test resources. Consider making the drop best-effort
(catch/log) and ensuring the stop sequence always runs in `finally` (or nested
`try/finally`).
##########
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:
This error log drops the stack trace by logging only `e.getMessage()`. That
makes Kafka consumer failures much harder to debug (especially in CI where INFO
is filtered). Log the exception itself (e.g., pass `e` to the logger) and/or
fail the test when counting records fails so the root cause isn’t hidden behind
a later timeout.
```suggestion
LOGGER.error("Error counting records with {}: {}", isolationLevel, e);
```
--
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]
