tmgodinho commented on code in PR #7854:
URL: https://github.com/apache/ignite-3/pull/7854#discussion_r3008895855
##########
modules/client/src/main/java/org/apache/ignite/internal/client/TcpClientChannel.java:
##########
@@ -647,12 +647,39 @@ private static Throwable readError(ClientMessageUnpacker
unpacker) {
var errClassName = unpacker.unpackString();
var errMsg = unpacker.tryUnpackNil() ? null : unpacker.unpackString();
- boolean retriable = false;
+ @Nullable String causeStr = unpacker.tryUnpackNil() ? null :
unpacker.unpackString();
+
+ String msg;
+ if (causeStr == null) {
+ msg = errMsg;
+ } else if (errMsg == null) {
+ msg = causeStr;
+ } else {
+ // Remove some duplication between errorMsg and cause.
Review Comment:
That is a good question.
I've done it on the client because, I was trying not to change the server
side.
However, I think that we could probably make sure the server-side does not
send duplicated stuff between cause and errMsg. It should not be a big problem
for other clients, even if they rely on it for something, they could also join
the messages themselves. Having that said, I don't know if the other clients
need the errMsg in the stacktrace message for displaying.
There's other improvement we could make on the server side, which is fully
serialize the causes and StackElements. We would be able to do better
reconstructions on the client side. However, I'm not sure if there's any added
value use case for doing that as opposed to the current string.
What do you think??
- Move forward with removing the errMsg and cause duplication on the server
side in this ticket.
- Create a spin-off ticket to do that, add mention here.
##########
modules/client/src/main/java/org/apache/ignite/internal/client/TcpClientChannel.java:
##########
@@ -647,12 +647,39 @@ private static Throwable readError(ClientMessageUnpacker
unpacker) {
var errClassName = unpacker.unpackString();
var errMsg = unpacker.tryUnpackNil() ? null : unpacker.unpackString();
- boolean retriable = false;
+ @Nullable String causeStr = unpacker.tryUnpackNil() ? null :
unpacker.unpackString();
+
+ String msg;
+ if (causeStr == null) {
+ msg = errMsg;
+ } else if (errMsg == null) {
+ msg = causeStr;
+ } else {
+ // Remove some duplication between errorMsg and cause.
+ int idx = causeStr.indexOf(errMsg);
+ msg = (idx == -1) ? errMsg + '\n' + causeStr :
causeStr.substring(idx);
+ }
+
+ Function<Boolean, Throwable> exceptionFactory = isRetriable -> {
Review Comment:
Yes, agreed.
##########
modules/client/src/integrationTest/java/org/apache/ignite/internal/client/ItThinClientConnectionTest.java:
##########
@@ -126,17 +129,14 @@ void testHeartbeat() {
@Test
void testExceptionHasHint() {
// Execute on all nodes to collect all types of exception.
- List<String> causes = IntStream.range(0,
client().configuration().addresses().length)
+ List<IgniteException> causes = IntStream.range(0,
client().configuration().addresses().length)
.mapToObj(i -> {
- IgniteException ex = assertThrows(IgniteException.class,
() -> client().sql().execute("select x from bad"));
-
- return
ex.getCause().getCause().getCause().getCause().getMessage();
+ return assertThrows(IgniteException.class, () ->
client().sql().execute("select x from bad"));
})
.collect(Collectors.toList());
assertThat(causes,
- hasItem(containsString("To see the full stack trace, "
- + "set
clientConnector.sendServerExceptionStackTraceToClient:true on the server")));
+ hasItem(publicExceptionWithHint(SqlException.class,
Sql.STMT_VALIDATION_ERR, "Object 'BAD' not found")));
Review Comment:
Hi @ptupitsyn
`publicExceptionWIthHint` is an attempt to make that assertion in a single
place. Is that ok??
--
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]
