-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add more DriverExceptionHandler to new error code structure #1394
base: main
Are you sure you want to change the base?
Conversation
|
||
@Override | ||
public RuntimeException handle(SchemaT schemaObject, DriverTimeoutException exception) { | ||
// TODO(Hazel): Aaron said "DRIVER" is a bad code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that "DRIVER" would be too vague, but "DRIVER_TIMEOUT" might be misleading -- maybe something like "DRIVER_UKNOWN_FAILURE" or something? This is assuming that what we map here is something nothing else recognizes, and that should eventually be mapped to something specific.
} | ||
if (message.contains("vector<float,")) { | ||
// It is tricky to find the actual vector dimension from the message, include as-is | ||
// TODO(Hazel): the code VECTOR_SIZE_MISMATCH was added recently, should we keep using it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that'd be good because it is very common failure, from ops/metrics -- we do not want to lose this information wrt metrics (leads to unnecessary investigation for common failures).
return exception; | ||
} | ||
// Reuse the default method in the interface | ||
return DriverExceptionHandler.super.handle(schemaObject, exception); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems somehow like abuse of default interface methods, having to specify interface of which default impl to use (but I don't know of a better way without using base class instead of interface).
@Override | ||
public RuntimeException handle(SchemaT schemaObject, ReadFailureException exception) { | ||
return DatabaseException.Code.READ_FAILURE.get( | ||
errVars( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not part of this PR, but I really think it'd be cleaner to have errVars
that takes schema object and Map
for extra variables to add, instead of closure for adding variables.
The Data API timed out while reading from the database. | ||
|
||
Reading from the ${schemaType} ${keyspace}.${table} failed to complete successfully. | ||
#TODO(Hazel): ReadTimeoutException has multiple ways to format the error message. What message should we use? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably don't want to have comment within text block? (it will be included as text, not comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, will remove it later - just want to leave the comments at the nearest place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HOLD until we can discuss what errors we want to return to the users, see my one comment
@@ -9,8 +9,14 @@ public DatabaseException(ErrorInstance errorInstance) { | |||
} | |||
|
|||
public enum Code implements ErrorCode<DatabaseException> { | |||
CLOSED_CONNECTION, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hold on merging this - we need to work out how to hide errors like the ones below from the users.
- CLOSED_CONNECTION,
- DRIVER_TIMEOUT,
NO_NODE_AVAILABLE,
also, it is sloppy to have a TABLE_WRITE_TIMEOUT, and WRITE_TIMEOUT; we need to work out if these are table specific errors or not.
What this PR does:
This PR migrates exception handling logic from ThrowableToErrorMapper to DefaultDriverExceptionHandler
Details of What Was Done:
QueryConsistencyException
, handling all related read/write timeouts and failure exceptions.AuthException
underRequestException
, rather than categorizing it underDatabaseException
(not sure?)Unresolved Questions / Points for Discussion:
TODO(Hazel)
comments in the relevant parts of the code.Which issue(s) this PR fixes:
Fixes Jira C2-3681
Checklist