You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "xtern (via GitHub)" <gi...@apache.org> on 2023/07/24 07:32:01 UTC
[GitHub] [ignite-3] xtern opened a new pull request, #2345: IGNITE-14865
xtern opened a new pull request, #2345:
URL: https://github.com/apache/ignite-3/pull/2345
(no comment)
--
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
[GitHub] [ignite-3] ygerzhedovich commented on a diff in pull request #2345: IGNITE-14865 Improve error handling in SQL module.
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #2345:
URL: https://github.com/apache/ignite-3/pull/2345#discussion_r1279127656
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDynamicParameterTest.java:
##########
@@ -159,8 +160,11 @@ public void testWithDifferentParametersTypes() {
*/
@Test
public void testWithDifferentParametersTypesMismatch() {
- assertThrows(IgniteException.class, () -> assertQuery("SELECT COALESCE(12.2, ?)").withParams("b").check());
- assertThrows(IgniteException.class, () -> assertQuery("SELECT COALESCE(?, ?)").withParams(12.2, "b").check());
+ SqlException t = assertThrows(SqlException.class, () -> assertQuery("SELECT COALESCE(12.2, ?)").withParams("b").check());
Review Comment:
there is org.apache.ignite.internal.sql.engine.util.SqlTestUtils#assertThrowsSqlException
--
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
[GitHub] [ignite-3] xtern merged pull request #2345: IGNITE-14865 Improve error handling in SQL module.
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern merged PR #2345:
URL: https://github.com/apache/ignite-3/pull/2345
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2345: IGNITE-14865 Improve error handling in SQL module.
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2345:
URL: https://github.com/apache/ignite-3/pull/2345#discussion_r1279532147
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/api/SessionImpl.java:
##########
@@ -327,7 +346,7 @@ public Publisher<Long> executeBatchReactive(@Nullable Transaction transaction, S
@Override
public void close() {
try {
- closeAsync().toCompletableFuture().get();
+ closeAsync().get();
} catch (ExecutionException e) {
sneakyThrow(IgniteExceptionUtils.copyExceptionWithCause(e));
Review Comment:
Since "closeAsync" is part of the public API, it must map its internal exceptions to the public ones on its own.
--
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
[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2345: IGNITE-14865 Improve error handling in SQL module.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2345:
URL: https://github.com/apache/ignite-3/pull/2345#discussion_r1279448604
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/api/SessionImpl.java:
##########
@@ -327,7 +346,7 @@ public Publisher<Long> executeBatchReactive(@Nullable Transaction transaction, S
@Override
public void close() {
try {
- closeAsync().toCompletableFuture().get();
+ closeAsync().get();
} catch (ExecutionException e) {
sneakyThrow(IgniteExceptionUtils.copyExceptionWithCause(e));
Review Comment:
Shoud we map to public exception here?
--
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
[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2345: IGNITE-14865 Improve error handling in SQL module.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2345:
URL: https://github.com/apache/ignite-3/pull/2345#discussion_r1279466254
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExchangeServiceImpl.java:
##########
@@ -112,45 +109,28 @@ public CompletableFuture<Void> request(String nodeName, UUID queryId, long fragm
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> sendError(String nodeName, UUID queryId, long fragmentId, Throwable error) {
- IgniteException errorWithCode = wrapIfNecessary(error);
+ Throwable errorWithCode = IgniteExceptionMapperUtil.mapToPublicException(ExceptionUtils.unwrapCause(error));
Review Comment:
Why do we convert exception here?
Seems, we can convert internal Traceble exception to default one with INTERNAL_ERR code and get lost traceID.
--
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
[GitHub] [ignite-3] xtern closed pull request #2345: IGNITE-14865
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern closed pull request #2345: IGNITE-14865
URL: https://github.com/apache/ignite-3/pull/2345
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2345: IGNITE-14865 Improve error handling in SQL module.
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2345:
URL: https://github.com/apache/ignite-3/pull/2345#discussion_r1279536745
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDynamicParameterTest.java:
##########
@@ -159,8 +160,11 @@ public void testWithDifferentParametersTypes() {
*/
@Test
public void testWithDifferentParametersTypesMismatch() {
- assertThrows(IgniteException.class, () -> assertQuery("SELECT COALESCE(12.2, ?)").withParams("b").check());
- assertThrows(IgniteException.class, () -> assertQuery("SELECT COALESCE(?, ?)").withParams(12.2, "b").check());
+ SqlException t = assertThrows(SqlException.class, () -> assertQuery("SELECT COALESCE(12.2, ?)").withParams("b").check());
Review Comment:
Thanks, I've fixed all the places in the PR where applicable.
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2345: IGNITE-14865 Improve error handling in SQL module.
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2345:
URL: https://github.com/apache/ignite-3/pull/2345#discussion_r1279558254
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExchangeServiceImpl.java:
##########
@@ -112,45 +109,28 @@ public CompletableFuture<Void> request(String nodeName, UUID queryId, long fragm
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> sendError(String nodeName, UUID queryId, long fragmentId, Throwable error) {
- IgniteException errorWithCode = wrapIfNecessary(error);
+ Throwable errorWithCode = IgniteExceptionMapperUtil.mapToPublicException(ExceptionUtils.unwrapCause(error));
Review Comment:
Thanks, seems there is no need to convert exception here. fixed.
--
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