RUBY-1896 Raise an actionable error message when retryWrites fails due to using an unsupported storage engine #1481
Conversation
lib/mongo/retryable.rb
Outdated
|
|
||
| # Retry writes on MMAPv1 should raise an actionable error, so we change the error message. | ||
| def raise_unsupported_error(e) | ||
| raise e, "This MongoDB deployment does not support retryable writes. Please add retryWrites=false to your connection string." |
There was a problem hiding this comment.
The original exception class and message need to be included in the final message following the #{e.class}: #{e} pattern.
There was a problem hiding this comment.
Since the class of exception is not changing, it may also make sense to preserve the backtrace which looks like this:
new_e = Error::OperationFailure.new(new_msg) new_e.set_backtrace(e.backtrace) raise new_e
There was a problem hiding this comment.
Is there another place in the driver where we compose multiple error messages like above? I would have assumed that the helpful error message here ("This MongoDB deployment does not support retryable writes...") should come first, but your comment about not including a trailing period is making me think it should come after the original exception class and message.
There was a problem hiding this comment.
The pattern I usually use is when translating one exception class to another, the message of the new class goes first, but when adding information to the existing exception and/or message, the information gets added at the end.
Example exceptions which get extended error messages are:
- NoServerAvailable (https://jira.mongodb.org/browse/RUBY-1787)
- SocketError/SocketTimeoutError (https://github.com/mongodb/mongo-ruby-driver/pull/988/files, https://jira.mongodb.org/browse/RUBY-1391)
- Mongo::Auth::Unauthorized
lib/mongo/retryable.rb
Outdated
| retry_write(e, session, txn_num, &block) | ||
| rescue Error::OperationFailure => e | ||
| if (session.in_transaction? && !ending_transaction) || !e.write_retryable? | ||
| if e.code == 20 && e.message.start_with?("Transaction numbers") |
There was a problem hiding this comment.
Given that OperationFailure already contains a bunch of logic for figuring out whether a specific error occurred, I think this logic should be moved to OperationFailure in a similar fashion to https://github.com/mongodb/mongo-ruby-driver/blob/master/lib/mongo/error/operation_failure.rb#L252.
spec/mongo/retryable_spec.rb
Outdated
| it 'raises an exception with the correct error message' do | ||
| expect { | ||
| retryable.write | ||
| }.to raise_error(Mongo::Error::OperationFailure, 'This MongoDB deployment does not support retryable writes. Please add retryWrites=false to your connection string.') |
There was a problem hiding this comment.
I think the Ruby option should also be mentioned, e.g. Please add retryWrites=false to your connection string or retry_writes: false Ruby client option. And we do not include trailing periods in exception messages for composability. (I am aware of the spec language mandating this exact message.)
|
If a client has retry_reads: true set on an mmapv1 deployment, do the reads work? |
|
From what I can see yes, the reads succeed. |
HanaPearlman
force-pushed
the
1896-actionable
branch
from
9f13119 to
1ce54d4
Compare
HanaPearlman
changed the title
HanaPearlman
force-pushed
the
1896-actionable
branch
from
1ce54d4 to
90f76f0
Compare
…e to using an unsupported storage engine (#1481) * initial actionable error message and tests * integration test for mmapv1 retryable write * remove unsupported error check from legacy retry writes, as it is not subject to current spec * implemented feedback
2 participants
No description provided.