You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "zstan (via GitHub)" <gi...@apache.org> on 2023/09/06 13:00:52 UTC
[GitHub] [ignite-3] zstan commented on a diff in pull request #2549: IGNITE-20149: Revise use INTERNAL_ERR in sql module
zstan commented on code in PR #2549:
URL: https://github.com/apache/ignite-3/pull/2549#discussion_r1317117338
##########
modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcBatchSelfTest.java:
##########
@@ -172,7 +172,7 @@ public void testPreparedStatementBatchException() throws Exception {
assertEquals(0, updCnts.length, "Invalid update counts size");
- assertThat(e.getMessage(), containsString("Given statement type does not match that declared by JDBC driver"));
+ assertThat(e.getMessage(), containsString("Invalid SQL statement type of a statement. Expected [DML, DDL] but got QUERY."));
Review Comment:
```suggestion
assertThat(e.getMessage(), containsString("Invalid SQL statement type. Expected [DML, DDL] but got QUERY."));
```
##########
modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcBatchSelfTest.java:
##########
@@ -217,24 +217,21 @@ public void testBatchException() throws Exception {
stmt.addBatch("insert into Person (id, firstName, lastName, age) values "
+ generateValues(100, 7));
- try {
- stmt.executeBatch();
-
- fail("BatchUpdateException must be thrown");
- } catch (BatchUpdateException e) {
- int[] updCnts = e.getUpdateCounts();
-
- assertEquals(successUpdates, updCnts.length, "Invalid update counts size");
+ BatchUpdateException e = JdbcTestUtils.assertThrowsSqlException(
+ BatchUpdateException.class,
+ "Invalid SQL statement type of a statement. Expected [DML, DDL] but got QUERY.",
Review Comment:
the same
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlSynchronousApiTest.java:
##########
@@ -293,16 +294,16 @@ public void errors() throws InterruptedException {
// No result set error.
{
ResultSet rs = ses.execute(null, "CREATE TABLE TEST3 (ID INT PRIMARY KEY)");
- assertThrowsPublicException(rs::next, NoRowSetExpectedException.class, Sql.QUERY_NO_RESULT_SET_ERR, "Query has no result set");
+ assertThrowsSqlException(NoRowSetExpectedException.class, Sql.QUERY_NO_RESULT_SET_ERR, "Query has no result set", rs::next);
}
// Cursor closed error.
{
ResultSet rs = ses.execute(null, "SELECT * FROM TEST");
Thread.sleep(300); // ResultSetImpl fetches next page in background, wait to it to complete to avoid flakiness.
Review Comment:
We need to avoid sleep usage in code, is it acceptable for end user to obtain 2 different Exception if ResultSet is closed ? If this issue behind the scope of this issue - need another one or reject if i`m wrong.
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/JdbcQueryEventHandlerImpl.java:
##########
@@ -340,26 +336,14 @@ public CompletableFuture<JdbcFinishTxResult> finishTxAsync(long connectionId, bo
}
/**
- * Serializes the stack trace of given exception for further sending to the client.
+ * Get a message of given exception for further sending to the client.
*
* @param t Throwable.
- * @return StringWriter filled with exception.
+ * @return String filled with exception message.
*/
- private StringWriter getWriterWithStackTrace(Throwable t) {
+ private String getErrorMessage(Throwable t) {
Review Comment:
```suggestion
@Nullable private String getErrorMessage(Throwable t) {
```
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlSynchronousApiTest.java:
##########
@@ -344,6 +345,34 @@ public void ddlInTransaction() {
assertEquals(0, ((IgniteImpl) CLUSTER_NODES.get(0)).txManager().pending());
}
+ @Disabled("https://issues.apache.org/jira/browse/IGNITE-20342")
+ @Test
+ public void runtimeErrorInTransaction() {
Review Comment:
good catch ) save our common blood )
##########
modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcStatementSelfTest.java:
##########
@@ -482,13 +482,13 @@ public void testExecuteUpdateProducesResultSet() {
final String sqlText = "select * from TEST;";
JdbcTestUtils.assertThrowsSqlException(
- "Given statement type does not match that declared by JDBC driver",
+ "Invalid SQL statement type of a statement.",
Review Comment:
```suggestion
"Invalid SQL statement type.",
```
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItCreateTableDdlTest.java:
##########
@@ -89,6 +89,7 @@ public void emptyPk() {
public void tableWithInvalidColumns() {
assertThrowsSqlException(
Sql.STMT_PARSE_ERR,
+ "Failed to parse query: Encountered \")\"",
Review Comment:
this statement is possible in PG, i don`t know the reason, mysql rejects it, as for me - it not applicable too but ... probably we need to check standard?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/QueryCancelledException.java:
##########
@@ -35,4 +35,13 @@ public class QueryCancelledException extends IgniteException {
public QueryCancelledException() {
super(EXECUTION_CANCELLED_ERR, ERR_MSG);
}
+
+ /**
+ * Constructor.
+ *
+ * @param cause Optional nested exception (can be {@code null}).
Review Comment:
there is no optional params in java ) @Nullable annotation need to be present ?
##########
modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcBatchSelfTest.java:
##########
@@ -172,7 +172,7 @@ public void testPreparedStatementBatchException() throws Exception {
assertEquals(0, updCnts.length, "Invalid update counts size");
- assertThat(e.getMessage(), containsString("Given statement type does not match that declared by JDBC driver"));
+ assertThat(e.getMessage(), containsString("Invalid SQL statement type of a statement. Expected [DML, DDL] but got QUERY."));
Review Comment:
This is public exception, DML, DDL is common known definitions, but what is the meaning for: QUERY ?)
Not clear for end user.
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/JdbcQueryEventHandlerImpl.java:
##########
@@ -137,9 +133,9 @@ public CompletableFuture<JdbcConnectResult> connect() {
return CompletableFuture.completedFuture(new JdbcConnectResult(connectionId));
} catch (IgniteInternalCheckedException exception) {
- StringWriter sw = getWriterWithStackTrace(exception);
+ String msg = getErrorMessage(exception);
- return CompletableFuture.completedFuture(new JdbcConnectResult(Response.STATUS_FAILED, "Unable to connect: " + sw));
+ return CompletableFuture.completedFuture(new JdbcConnectResult(Response.STATUS_FAILED, "Unable to connect: " + msg));
Review Comment:
no one can throw exceptions in code above as i can see, thus try\catch can be easily removed.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org