Add UDS read/write error handling and tests with glide core mock. #76
Conversation
Signed-off-by: Yury-Fridlyand <[email protected]>
Yury-Fridlyand
requested review from
aaron-congo,
jonathanl-bq,
acarbonetto,
Elen-Ghulam and
SanHalacogluImproving
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
java/client/src/main/java/glide/connectors/handlers/ChannelHandler.java
Outdated
Show resolved
Hide resolved
java/client/src/main/java/glide/connectors/resources/ThreadPoolResource.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public void operationComplete(@NonNull ChannelFuture channelFuture) throws Exception { | ||
| if (!channelFuture.isSuccess()) { |
There was a problem hiding this comment.
According to https://netty.io/4.0/api/io/netty/channel/ChannelFuture.html, !isSuccess just means that the future did not complete successfully.
We should check for cause != null to determine if it failed.
In the case of isCancelled, do we feel it is necessary to terminate the client?
There was a problem hiding this comment.
I added extra checks, but I have no idea how to test them. Any clue?
Signed-off-by: Yury-Fridlyand <[email protected]>
| import redis_request.RedisRequestOuterClass.RedisRequest; | ||
| import response.ResponseOuterClass.Response; | ||
|
|
||
| public class ConnectionWithGlideMockTests extends RustCoreLibMockTestBase { |
There was a problem hiding this comment.
Can we add tests for InterruptedExceptions on the thread?
There was a problem hiding this comment.
What should I do to get one? Kill a thread?
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
|
|
||
| private static ConnectionRequest createConnectionRequest() { | ||
| return ConnectionRequest.newBuilder() | ||
| .addAddresses(NodeAddress.newBuilder().setHost("dummyhost").setPort(42).build()) |
There was a problem hiding this comment.
Let's add a comment about why fake values are used.
|
|
||
| // Not a public class, can't import | ||
| assertEquals( | ||
| "io.netty.channel.StacklessClosedChannelException", |
There was a problem hiding this comment.
This seems brittle if Netty's version changes.
There was a problem hiding this comment.
hopefully they make it public when they update?
There was a problem hiding this comment.
I hope it won't happen without CI verification to catch failures.
|
|
||
| public class RustCoreMock { | ||
|
|
||
| public abstract static class GlideMock { |
There was a problem hiding this comment.
This could be an interface with a default isRaw() function (as well as be made into a @FunctionalInterface) to make this a bit more flexible.
| buf.release(); | ||
| if (messageProcessor.isRaw()) { | ||
| ch.writeAndFlush( | ||
| Unpooled.copiedBuffer(messageProcessor.handle(bytes))); |
There was a problem hiding this comment.
This has alot of nesting. Anyway it can be reduced? It's deep enough that it's pretty unreadable in GitHub's diff view.
There was a problem hiding this comment.
The linter adds 4/8 spaces for each next identation.
In client side the same code is split over 5 classes, but here we have a testing tool. I decided to keep all together to avoid resolving dependencies in those classes.
There was a problem hiding this comment.
@Yury-Fridlyand at least create a separate class for ChannelInboundHandlerAdapter?
Signed-off-by: Yury-Fridlyand <[email protected]>
| buf.release(); | ||
| if (messageProcessor.isRaw()) { | ||
| ch.writeAndFlush( | ||
| Unpooled.copiedBuffer(messageProcessor.handle(bytes))); |
There was a problem hiding this comment.
@Yury-Fridlyand at least create a separate class for ChannelInboundHandlerAdapter?
Signed-off-by: Yury-Fridlyand <[email protected]>
Yury-Fridlyand
merged commit 7ade9d6
into
java/integ_yuryf_handle_uds_errors
valkey-io#949) Add UDS read/write error handling and tests with glide core mock. (#76) * Add UDS read/write error handling and tests with glide core mock. Signed-off-by: Yury-Fridlyand <[email protected]>
valkey-io#949) Add UDS read/write error handling and tests with glide core mock. (#76) * Add UDS read/write error handling and tests with glide core mock. Signed-off-by: Yury-Fridlyand <[email protected]>
Features:
Missing:
ChannelHandler.