Backport #2597 (Handle RSolr timeouts with its own class) #2636
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…class == context == Almost any RSolr exception is wrapped by Blacklight in a `Blacklight::Exceptions::InvalidRequest` A Blacklight Catalog controller [rescues](https://github.com/projectblacklight/blacklight/blob/main/app/controllers/concerns/blacklight/catalog.rb#L21) this exception with a [method](https://github.com/projectblacklight/blacklight/blob/main/app/controllers/concerns/blacklight/catalog.rb#L282-L299) that just re-displays the search page, with the message "Sorry, I don't understand your search." "Connection refused" errors are handled differently -- which is good, it would make no sense to prompt the user to search again with "Sorry, I don't understand your search" to network errors like connection refused. What about a timeout error, where Solr did not return in a response within the timeout value RSolr had configured? Prior to this PR, that was handled the same as any `Blacklight::Exceptions::InvalidRequest` -- redisplay search page to the user, with message "Sorry, I don't understand your search." That is not a desirable UX, and also causes Solr timeout errors to generally not make it to many error monitoring and logging systems, since Blacklight rescues them and displays a 200. == change == With the release of RSolr 2.4.0, RSolr uses a specific exception class to raise timeout errors, so Blacklight can rescue them and handle them differently. After this PR, they are handled analogously to connection refused errors, with a custom exception that won't be rescued as a `Blacklight::Exceptions::InvalidRequest`. a `defined?` check is done so Blacklight can still work with earlier versions of RSolr, and just won't do this new rescue. == weird code in test == RSolr::Error api has some really rough edges, which requires some weirdness in setting up the test. == to be done in future == I formatted the message of this exception just like the existing `Blacklight::Exceptions::ECONNREFUSED`... even though that formatting isn't necessarily so useful, there's a lot of stuff in there. But I opted to do it consistently.
… method Thanks @barmintor, I think you're right this is more clear.
…cklight::Solr::Repository
jcoyne
approved these changes
Feb 25, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.