Copilot commented on code in PR #17181:
URL: https://github.com/apache/iotdb/pull/17181#discussion_r2785404263
##########
iotdb-client/cli/src/main/java/org/apache/iotdb/cli/AbstractCli.java:
##########
@@ -611,6 +676,18 @@ private static int executeQuery(CliContext ctx,
IoTDBConnection connection, Stri
} else {
ctx.getPrinter().println("Msg: " + SUCCESS_MESSAGE);
}
+ } catch (SQLException e) {
+ if (isConnectionRelated(e)) {
+ throw e;
+ }
+ if (isSessionOrStatementError(e)) {
+ ctx.getPrinter()
+ .println(
+ "Reconnected, but the previous command could not be completed.
Please run your command again.");
Review Comment:
`executeQuery` prints "Reconnected, but the previous command could not be
completed..." for session/statement-state errors, but this method cannot know
whether a reconnect actually happened. If the same server-side error occurs
without any reconnect, this message is misleading. Suggestion: make the message
not assume reconnection (e.g., "Session state was reset..."), or only print
this in the reconnect-and-retry path where reconnection is known to have
occurred.
```suggestion
"Session state was reset and the previous command could not
be completed. Please run your command again.");
```
##########
iotdb-client/cli/src/main/java/org/apache/iotdb/cli/Cli.java:
##########
@@ -195,36 +247,87 @@ private static void executeSql(CliContext ctx) throws
TException {
}
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();
-
AGGREGRATE_TIME_LIST.addAll(properties.getSupportedTimeAggregationOperations());
- 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) {
+ if (isSessionOrStatementError(e)) {
+ // Reconnect succeeded but retry failed due to
session/statement state; ask user to
+ // run the command again.
+ ctx.getPrinter()
+ .println(
+ "Reconnected, but the previous command could not be
completed. Please run your command again.");
+ reconnected = true;
+ break;
+ }
+ if (attempt == RECONNECT_RETRY_NUM) {
+ ctx.getErr()
+ .printf(
+ "%s: Could not reconnect after %d attempts. Please
check that the server is running and try again.%n",
+ IOTDB_ERROR_PREFIX, RECONNECT_RETRY_NUM);
+ ctx.exit(CODE_ERROR);
+ }
+ }
Review Comment:
On reconnect attempts, if `openConnection()` succeeds but
`setupConnection()` or `processCommand()` throws, the newly created
`connection` is not closed before the next attempt (the variable is overwritten
on the next iteration). This can leak sockets/sessions across retries.
Suggestion: in the catch path, call `closeConnectionQuietly(connection)` and
set `connection = null` before continuing to the next attempt.
##########
iotdb-client/cli/src/main/java/org/apache/iotdb/cli/Cli.java:
##########
@@ -195,36 +247,87 @@ private static void executeSql(CliContext ctx) throws
TException {
}
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();
-
AGGREGRATE_TIME_LIST.addAll(properties.getSupportedTimeAggregationOperations());
- 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;
Review Comment:
The reconnect retry `try { connection = openConnection();
setupConnection(connection); ... }` can throw `TException` from
`setupConnection()`, but the retry loop only catches `SQLException`. This means
some connection-loss cases can bypass the reconnect logic and bubble out
immediately. Suggestion: include `TException` (or a common supertype) in the
reconnect-attempt catch and handle it as a reconnect failure (with the same
retry/backoff), while still distinguishing non-connection SQL errors from
command execution.
##########
iotdb-client/cli/src/main/java/org/apache/iotdb/cli/Cli.java:
##########
@@ -195,36 +247,87 @@ private static void executeSql(CliContext ctx) throws
TException {
}
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();
-
AGGREGRATE_TIME_LIST.addAll(properties.getSupportedTimeAggregationOperations());
- 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;
Review Comment:
In the reconnect-and-retry path, the return value of `processCommand(...)`
is ignored. If the retried input contains an `exit`/`quit` statement (or
otherwise returns `false`), the CLI will continue the main loop instead of
stopping as it normally would. Suggestion: capture the boolean return from
`processCommand` and convert it into a `result.stop` / break out of the outer
loop when it indicates the user requested exit.
##########
iotdb-client/cli/src/main/java/org/apache/iotdb/cli/Cli.java:
##########
@@ -195,36 +247,87 @@ private static void executeSql(CliContext ctx) throws
TException {
}
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();
-
AGGREGRATE_TIME_LIST.addAll(properties.getSupportedTimeAggregationOperations());
- 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) {
+ if (isSessionOrStatementError(e)) {
+ // Reconnect succeeded but retry failed due to
session/statement state; ask user to
+ // run the command again.
+ ctx.getPrinter()
+ .println(
+ "Reconnected, but the previous command could not be
completed. Please run your command again.");
+ reconnected = true;
+ break;
+ }
+ if (attempt == RECONNECT_RETRY_NUM) {
+ ctx.getErr()
+ .printf(
+ "%s: Could not reconnect after %d attempts. Please
check that the server is running and try again.%n",
+ IOTDB_ERROR_PREFIX, RECONNECT_RETRY_NUM);
+ ctx.exit(CODE_ERROR);
+ }
+ }
+ }
+ if (!reconnected) {
+ break;
+ }
+ }
}
} catch (SQLException e) {
ctx.getErr().printf("%s: %s%n", IOTDB_ERROR_PREFIX, e.getMessage());
ctx.exit(CODE_ERROR);
+ } finally {
+ closeConnectionQuietly(connection);
}
}
- private static boolean readerReadLine(CliContext ctx, IoTDBConnection
connection) {
+ private static ReadLineResult readerReadLine(CliContext ctx, IoTDBConnection
connection) {
String s;
try {
s = ctx.getLineReader().readLine(cliPrefix + "> ", null);
- boolean continues = processCommand(ctx, s, connection);
- if (!continues) {
- return true;
+ try {
+ boolean continues = processCommand(ctx, s, connection);
+ if (!continues) {
+ return ReadLineResult.stopLoop();
+ }
+ } catch (SQLException e) {
+ if (isConnectionRelated(e)) {
+ return ReadLineResult.reconnectAndRetry(s);
+ }
Review Comment:
When a connection-related SQLException occurs, the CLI retries the entire
raw input line `s`. If the line contains multiple statements separated by `;`,
some statements may already have executed successfully before the failure, and
retrying the whole line can re-run those statements (duplicate writes / side
effects). Suggestion: either only retry the specific statement that failed
(track progress inside `processCommand`), or disable auto-retry for
multi-statement input and ask the user to rerun manually.
##########
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();
Review Comment:
`setupConnection()` appends to `AGGREGRATE_TIME_LIST` every time a reconnect
happens, but the list is static and never cleared/deduped. During repeated
reconnects this will grow unbounded and may introduce duplicate entries.
Suggestion: clear the list before adding, or change it to a `Set` (if ordering
is not required).
```suggestion
properties = connection.getServerProperties();
AGGREGRATE_TIME_LIST.clear();
```
##########
iotdb-client/cli/src/main/java/org/apache/iotdb/cli/Cli.java:
##########
@@ -195,36 +247,87 @@ private static void executeSql(CliContext ctx) throws
TException {
}
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();
-
AGGREGRATE_TIME_LIST.addAll(properties.getSupportedTimeAggregationOperations());
- 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) {
+ if (isSessionOrStatementError(e)) {
+ // Reconnect succeeded but retry failed due to
session/statement state; ask user to
+ // run the command again.
+ ctx.getPrinter()
+ .println(
+ "Reconnected, but the previous command could not be
completed. Please run your command again.");
+ reconnected = true;
+ break;
+ }
+ if (attempt == RECONNECT_RETRY_NUM) {
+ ctx.getErr()
+ .printf(
+ "%s: Could not reconnect after %d attempts. Please
check that the server is running and try again.%n",
+ IOTDB_ERROR_PREFIX, RECONNECT_RETRY_NUM);
+ ctx.exit(CODE_ERROR);
+ }
+ }
+ }
+ if (!reconnected) {
+ break;
+ }
+ }
}
} catch (SQLException e) {
ctx.getErr().printf("%s: %s%n", IOTDB_ERROR_PREFIX, e.getMessage());
ctx.exit(CODE_ERROR);
+ } finally {
+ closeConnectionQuietly(connection);
}
}
- private static boolean readerReadLine(CliContext ctx, IoTDBConnection
connection) {
+ private static ReadLineResult readerReadLine(CliContext ctx, IoTDBConnection
connection) {
String s;
try {
s = ctx.getLineReader().readLine(cliPrefix + "> ", null);
- boolean continues = processCommand(ctx, s, connection);
- if (!continues) {
- return true;
+ try {
+ boolean continues = processCommand(ctx, s, connection);
+ if (!continues) {
+ return ReadLineResult.stopLoop();
+ }
+ } catch (SQLException e) {
+ if (isConnectionRelated(e)) {
+ return ReadLineResult.reconnectAndRetry(s);
+ }
Review Comment:
The reconnection trigger/loop is a significant behavior change but there are
no unit tests exercising it (e.g., simulating a connection-related SQLException
from `processCommand`, verifying retry count/backoff, and ensuring
non-connection SQLExceptions do not trigger reconnect or an incorrect "Could
not reconnect" exit). Consider adding focused tests around this new flow to
prevent regressions.
##########
iotdb-client/cli/src/main/java/org/apache/iotdb/cli/Cli.java:
##########
@@ -195,36 +247,87 @@ private static void executeSql(CliContext ctx) throws
TException {
}
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();
-
AGGREGRATE_TIME_LIST.addAll(properties.getSupportedTimeAggregationOperations());
- 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++) {
Review Comment:
Test is always true, because of [this condition](1).
Test is always true, because of [this condition](2).
##########
iotdb-client/cli/src/main/java/org/apache/iotdb/cli/Cli.java:
##########
@@ -195,36 +247,87 @@ private static void executeSql(CliContext ctx) throws
TException {
}
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();
-
AGGREGRATE_TIME_LIST.addAll(properties.getSupportedTimeAggregationOperations());
- 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) {
+ if (isSessionOrStatementError(e)) {
+ // Reconnect succeeded but retry failed due to
session/statement state; ask user to
+ // run the command again.
+ ctx.getPrinter()
+ .println(
+ "Reconnected, but the previous command could not be
completed. Please run your command again.");
+ reconnected = true;
+ break;
+ }
+ if (attempt == RECONNECT_RETRY_NUM) {
+ ctx.getErr()
+ .printf(
+ "%s: Could not reconnect after %d attempts. Please
check that the server is running and try again.%n",
+ IOTDB_ERROR_PREFIX, RECONNECT_RETRY_NUM);
+ ctx.exit(CODE_ERROR);
+ }
Review Comment:
In the reconnect loop, any SQLException thrown while retrying the failed
command is treated as a reconnect failure. This can incorrectly trigger further
reconnect attempts (and eventually exit with "Could not reconnect") for
non-connection SQL errors (e.g., syntax/permission errors) that happen after
the reconnect succeeds. Suggestion: only retry when `isConnectionRelated(e)` is
true; otherwise report the SQL error from the retried command and continue the
main loop (or stop) without attempting further reconnects.
--
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]
