Copilot commented on code in PR #17181:
URL: https://github.com/apache/iotdb/pull/17181#discussion_r2815123891
##########
iotdb-client/cli/src/main/java/org/apache/iotdb/cli/Cli.java:
##########
@@ -155,6 +184,29 @@ private static boolean parseCommandLine(
return true;
}
+ private static IoTDBConnection openConnection() throws SQLException {
+ return (IoTDBConnection)
+ DriverManager.getConnection(Config.IOTDB_URL_PREFIX + host + ":" +
port + "/", info);
+ }
+
+ private static void setupConnection(IoTDBConnection connection)
+ throws java.sql.SQLException, org.apache.thrift.TException {
+ connection.setQueryTimeout(queryTimeout);
+ properties = connection.getServerProperties();
+
AGGREGRATE_TIME_LIST.addAll(properties.getSupportedTimeAggregationOperations());
+ timestampPrecision = properties.getTimestampPrecision();
Review Comment:
`AGGREGRATE_TIME_LIST` is referenced in `setupConnection()` but is not
declared in `Cli` or inherited from `AbstractCli` (no such field exists), so
this will not compile. Either define the intended collection (and ensure the
spelling/name matches existing conventions) or remove this line if it isn’t
needed for CLI startup/reconnect.
##########
iotdb-client/cli/src/main/java/org/apache/iotdb/cli/Cli.java:
##########
@@ -188,41 +240,94 @@ private static void executeSql(CliContext ctx) throws
TException {
processCommand(ctx, execute, connection);
ctx.exit(lastProcessStatus);
} catch (SQLException e) {
- ctx.getPrinter().println(IOTDB_ERROR_PREFIX + "Can't execute sql
because" + e.getMessage());
+ ctx.getPrinter()
+ .println(IOTDB_ERROR_PREFIX + ": Can't execute sql because " +
e.getMessage());
ctx.exit(CODE_ERROR);
}
}
private static void receiveCommands(CliContext ctx) throws TException {
- try (IoTDBConnection connection =
- (IoTDBConnection)
- DriverManager.getConnection(Config.IOTDB_URL_PREFIX + host + ":" +
port + "/", info)) {
- connection.setQueryTimeout(queryTimeout);
- properties = connection.getServerProperties();
- timestampPrecision = properties.getTimestampPrecision();
-
+ IoTDBConnection connection = null;
+ try {
+ connection = openConnection();
+ setupConnection(connection);
echoStarting(ctx);
displayLogo(ctx, properties.getLogo(), properties.getVersion(),
properties.getBuildInfo());
ctx.getPrinter().println(String.format("Successfully login at %s:%s",
host, port));
while (true) {
- boolean readLine = readerReadLine(ctx, connection);
- if (readLine) {
+ ReadLineResult result = readerReadLine(ctx, connection);
+ if (result.stop) {
break;
}
+ if (result.failedCommand != null) {
+ // Connection failed during processCommand; try to reconnect and
retry the command.
+ closeConnectionQuietly(connection);
+ connection = null;
+ boolean reconnected = false;
+ for (int attempt = 1; attempt <= RECONNECT_RETRY_NUM; attempt++) {
+ if (attempt > 1) {
+ try {
+ Thread.sleep(RECONNECT_RETRY_INTERVAL_MS);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ ctx.getErr().printf("%s: Reconnection interrupted.%n",
IOTDB_ERROR_PREFIX);
+ ctx.exit(CODE_ERROR);
+ }
+ }
+ try {
+ connection = openConnection();
+ setupConnection(connection);
+ ctx.getPrinter().println("Connection lost. Reconnected. Retrying
command.");
+ processCommand(ctx, result.failedCommand, connection);
+ reconnected = true;
+ break;
+ } catch (SQLException e) {
Review Comment:
The reconnect attempt `try { openConnection(); setupConnection(); ... }
catch (SQLException e)` does not handle `TException`, but `setupConnection()`
declares it can throw. If `getServerProperties()` fails due to a transient
RPC/network issue during reconnect, the `TException` will escape and abort the
CLI instead of retrying. Catch `TException` in the reconnect loop and treat it
as a reconnect failure (with the same retry/backoff behavior).
##########
iotdb-client/cli/src/main/java/org/apache/iotdb/cli/AbstractCli.java:
##########
@@ -55,6 +55,59 @@
public abstract class AbstractCli {
+ /**
+ * Returns true if the SQLException is likely due to connection loss. Used
so that CLI can rethrow
+ * and trigger reconnection.
+ */
+ static boolean isConnectionRelated(SQLException e) {
+ if (e == null) {
+ return false;
+ }
+ if (matchesConnectionFailure(e.getMessage())) {
+ return true;
+ }
+ Throwable cause = e.getCause();
+ return cause != null && matchesConnectionFailure(cause.getMessage());
+ }
+
+ private static boolean matchesConnectionFailure(String msg) {
+ if (msg == null) {
+ return false;
+ }
+ String lower = msg.toLowerCase();
+ return lower.contains("connection")
+ || lower.contains("refused")
+ || lower.contains("timeout")
+ || lower.contains("closed")
+ || lower.contains("reset")
+ || lower.contains("network")
+ || lower.contains("broken pipe");
+ }
+
+ /**
+ * Returns true if the SQLException indicates a session/statement state
error (e.g. statement ID
+ * no longer valid after reconnect). Used to show a friendly message instead
of the raw exception.
+ */
+ static boolean isSessionOrStatementError(SQLException e) {
+ if (e == null) {
+ return false;
+ }
+ if (e.getMessage() != null &&
matchesSessionOrStatementFailure(e.getMessage())) {
+ return true;
+ }
+ Throwable cause = e.getCause();
+ return cause != null
+ && cause.getMessage() != null
+ && matchesSessionOrStatementFailure(cause.getMessage());
+ }
+
+ private static boolean matchesSessionOrStatementFailure(String msg) {
+ String lower = msg.toLowerCase();
+ return lower.contains("doesn't exist in this session")
+ || lower.contains("statementid")
+ || lower.contains("statement id");
+ }
Review Comment:
The new connection/session failure detection helpers (`isConnectionRelated`,
`matchesConnectionFailure`, `isSessionOrStatementError`) introduce key behavior
but currently have no unit tests. Since this module already has unit tests,
please add focused tests that cover representative messages/cause-chains and
ensure false positives/negatives are minimized.
--
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]
