-
Notifications
You must be signed in to change notification settings - Fork 229
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
Modified to handle errors during read path #143
Conversation
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.
Looks good in general, just comment on wrapping the errors. So this solves the silent error problem? I'm a bit intrigued because normally exceptions bubble up automatically.
@@ -20,6 +20,7 @@ object Util { | |||
if (resultSet.wasApplied) Success else notAppliedResponse | |||
}.recover { | |||
case e: DriverException => throw StorageEngineException(e) | |||
case e: Exception => throw e |
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 we should still wrap the error in StorageEngineException.
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.
Done.
@@ -30,6 +31,7 @@ object Util { | |||
// from invalid Enum strings, which should never happen, or some other parsing error | |||
case e: NoSuchElementException => throw MetadataException(e) | |||
case e: IllegalArgumentException => throw MetadataException(e) | |||
case e: Exception => throw e |
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.
Same here, wrap in StorageEngineException
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.
Done
Added this general exception to handle just in case of other errors than DriverException. But this is not the code actually solving the issue. The changes made to the ChunkRowMap table helped to throw exception back to the client. |
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.
LGTM
Related to issue: No error is displayed when cassandra nodes are down for read path #120