isapego commented on code in PR #949:
URL: https://github.com/apache/ignite-3/pull/949#discussion_r925258877
##########
modules/client/src/main/java/org/apache/ignite/internal/client/ReliableChannel.java:
##########
@@ -296,9 +297,9 @@ private <T> void handleServiceAsync(final
CompletableFuture<T> fut,
* @return host:port_range address lines parsed as {@link
InetSocketAddress} as a key. Value is the amount of appearences of an address
* in {@code addrs} parameter.
*/
- private static Map<InetSocketAddress, Integer> parsedAddresses(String[]
addrs) throws IgniteClientException {
+ private static Map<InetSocketAddress, Integer> parsedAddresses(String[]
addrs) {
if (addrs == null || addrs.length == 0) {
- throw new IgniteClientException("Empty addresses");
+ throw new IgniteException(UNKNOWN_ERR, "Empty addresses");
Review Comment:
Maybe something like `USER_INPUT_ERR`?
##########
modules/client-handler/src/integrationTest/java/org/apache/ignite/client/handler/ItClientHandlerTest.java:
##########
@@ -177,21 +181,26 @@ void testHandshakeInvalidVersionReturnsError() throws
Exception {
// Read response.
var unpacker =
MessagePack.newDefaultUnpacker(sock.getInputStream());
var magic = unpacker.readPayload(4);
- unpacker.skipValue(3);
- var len = unpacker.unpackInt();
- var major = unpacker.unpackInt();
+ unpacker.readPayload(4); // Length.
Review Comment:
Why not `skipValue`?
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java:
##########
@@ -191,7 +194,7 @@ private void handshake(ChannelHandlerContext ctx,
ClientMessageUnpacker unpacker
var clientVer = ProtocolVersion.unpack(unpacker);
if (!clientVer.equals(ProtocolVersion.LATEST_VER)) {
- throw new IgniteException("Unsupported version: "
+ throw new IgniteException(PROTOCOL_ERR, "Unsupported version: "
Review Comment:
Sounds more like compatibility error or smth like that to me. Protocol works
just fine here.
##########
modules/client/src/main/java/org/apache/ignite/internal/client/ReliableChannel.java:
##########
@@ -557,7 +559,7 @@ private ClientChannel getDefaultChannel() {
}
}
- throw new IgniteClientConnectionException("Failed to connect",
failure);
+ throw new IgniteClientConnectionException(UNKNOWN_ERR, "Failed to
connect", failure);
Review Comment:
Why not `CONNECTION_ERR`?
##########
modules/client/src/main/java/org/apache/ignite/internal/client/sql/ClientAsyncResultSet.java:
##########
@@ -134,11 +136,11 @@ public CompletionStage<? extends AsyncResultSet>
fetchNextPage() {
requireResultSet();
if (closed) {
- return CompletableFuture.failedFuture(new
IgniteClientException("Cursor is closed."));
+ return CompletableFuture.failedFuture(new
IgniteException(UNKNOWN_ERR, "Cursor is closed."));
}
if (!hasMorePages()) {
- return CompletableFuture.failedFuture(new
IgniteClientException("No more pages."));
+ return CompletableFuture.failedFuture(new
IgniteException(UNKNOWN_ERR, "No more pages."));
Review Comment:
Do we use `UNKNOWN_ERR` as generic error code?
##########
modules/client/src/main/java/org/apache/ignite/internal/client/ReliableChannel.java:
##########
@@ -683,7 +683,8 @@ private ClientChannel getOrCreateChannel(boolean
ignoreThrottling)
}
if (!ignoreThrottling && applyReconnectionThrottling()) {
- throw new IgniteClientConnectionException("Reconnect
is not allowed due to applied throttling");
+ //noinspection
NonPrivateFieldAccessedInSynchronizedContext
+ throw new IgniteClientConnectionException(UNKNOWN_ERR,
"Reconnect is not allowed due to applied throttling");
Review Comment:
`CONNECTION_ERR`?
##########
modules/client/src/main/java/org/apache/ignite/internal/client/TcpClientChannel.java:
##########
@@ -291,30 +271,58 @@ private void processNextMessage(ByteBuf buf) throws
IgniteClientException {
var type = unpacker.unpackInt();
if (type != ServerMessageType.RESPONSE) {
- throw new IgniteClientException("Unexpected message type: " +
type);
+ throw new IgniteClientConnectionException(PROTOCOL_ERR,
"Unexpected message type: " + type);
}
Long resId = unpacker.unpackLong();
- int status = unpacker.unpackInt();
-
ClientRequestFuture pendingReq = pendingReqs.remove(resId);
if (pendingReq == null) {
- throw new IgniteClientException(String.format("Unexpected response
ID [%s]", resId));
+ throw new IgniteClientConnectionException(PROTOCOL_ERR,
String.format("Unexpected response ID [%s]", resId));
}
- if (status == 0) {
+ if (unpacker.tryUnpackNil()) {
pendingReq.complete(unpacker);
} else {
- var errMsg = unpacker.unpackString();
+ IgniteException err = readError(unpacker);
+
unpacker.close();
- var err = new IgniteClientException(errMsg, status);
pendingReq.completeExceptionally(err);
}
}
+ /**
+ * Unpacks request error.
+ *
+ * @param unpacker Unpacker.
+ * @return Exception.
+ */
+ private IgniteException readError(ClientMessageUnpacker unpacker) {
+ var traceId = unpacker.tryUnpackNil() ? UUID.randomUUID() :
unpacker.unpackUuid();
Review Comment:
Is random UUID a proper way to deal with this situation? Won't it cause any
issues? Did you discuss it with guys who designed it?
--
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]
