java: don't use checked exceptions for our own programming errors #1423
Conversation
RequestException is thrown when the underlying C library detects that the request is invalid --- the op or the size of message body is wrong. However, the public API of of our Java client ensures that these are always correct: it is impossible to trigger these errors unless: * there's a bug in our C or Java code * there's a version mismatch between our Java and C code (which is unlikely, as they come from the same jar) So, there's absolutely no reason to tax the caller with declaring and handling this exception.
matklad
force-pushed
the
matklad/docs-generate-5
branch
from
7c27c29
to
e28c610
Compare
| else if (status == PacketStatus.InvalidDataSize.value) | ||
| return "Invalid data size. Check if this client is compatible with the server's version."; |
There was a problem hiding this comment.
As far as I understand, this is just wrong.
| @@ -143,7 +143,7 @@ private void waitForCompletionUninterruptibly() { | |||
| } | |||
| } | |||
|
|
|||
| TResponse getResult() throws RequestException { | |||
| TResponse getResult() { | |||
|
|
|||
| assertTrue(result != null || exception != null, "Unexpected request result: result=null"); | |||
There was a problem hiding this comment.
I am wondering... Do we actually need this exception as a separate class even? From that
Check if this client is compatible with the server's version.
It feels like the original intention behind the exception was to signal error response from the server, and then it mutated to signal local programming errors. In this new role, perhaps we can just throw an AssertionError for non-ok packet statuses?
There was a problem hiding this comment.
The original intention was to map result codes into Java exceptions: InitializationException for tb_client_init, RequestException for packet.status, and ConcurrencyExceededException for tb_client_acquire_packet.
Check if this client is compatible with the server's version.
This message is indeed wrong, we never planned to return error response from the server at the protocol level (e.g. invalid size or operation), I think the C# client uses the same string.
(...)
I completely agree with RequestException extends RuntimeException, but I don't have a strong opinion on using AssertionError for it (feels like we are losing information if we reduce the packet status to an assertion error message).
| @@ -143,7 +143,7 @@ private void waitForCompletionUninterruptibly() { | |||
| } | |||
| } | |||
|
|
|||
| TResponse getResult() throws RequestException { | |||
| TResponse getResult() { | |||
|
|
|||
| assertTrue(result != null || exception != null, "Unexpected request result: result=null"); | |||
There was a problem hiding this comment.
The original intention was to map result codes into Java exceptions: InitializationException for tb_client_init, RequestException for packet.status, and ConcurrencyExceededException for tb_client_acquire_packet.
Check if this client is compatible with the server's version.
This message is indeed wrong, we never planned to return error response from the server at the protocol level (e.g. invalid size or operation), I think the C# client uses the same string.
(...)
I completely agree with RequestException extends RuntimeException, but I don't have a strong opinion on using AssertionError for it (feels like we are losing information if we reduce the packet status to an assertion error message).
Yeah, same here. The argument "for" removing the dedicated exception is reducing the public API surface area by one class. I guess:
|
|
I agree. Let's wait for the client refactoring King is working on. I think
Additionally:
|
RequestException is thrown when the underlying C library detects that
the request is invalid --- the op or the size of message body is wrong.
However, the public API of of our Java client ensures that these are
always correct: it is impossible to trigger these errors unless:
unlikely, as they come from the same jar)
So, there's absolutely no reason to tax the caller with declaring and
handling this exception.