DNN-21973: retry the search when there have exception throw during search process. #2198
Conversation
|
what are the reasons for the search to throw an exception and does it really helps just to retry 5 times immediately in a row? |
|
This is an edge case in azure instances, exception will thrown during migrate new instances, the drive need to prepare for a while, and in that time when request index files will get IOException, so need to close the reader and try again. actually its not retry immediately but wait for 100ms then retry. also the retry times can config to a larger number if we found the drive initialize will take longer time. |
|
We are seeing this exception at customer's site. Please note most of the PR from @zyhfish come from real customer issues. |
| return luceneResults; | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
If possible, please catch a more specific exception. You mention IOException in the ticket. Any other exception probably should not cause a retry, right?
There was a problem hiding this comment.
yep, there have more exceptions but not only this one after deploy the patch and get the error log, have create several sub tasks to handle these errors.
There was a problem hiding this comment.
Then please add a specific catch clause for each "expected exception". Let's start by catching the IOExcpetion in this PR and subsequent submissions will add more handling
| Logger.Error($"Search Index Folder Is Not Available: {ex.Message}, Retry {retryTimes} time(s)."); | ||
| Thread.Sleep(100); | ||
|
|
||
| return Search(searchContext, retryTimes); |
There was a problem hiding this comment.
Please do not call Search recursively from try..catch. Instead please wrap error handling in a loop and break out of that loop when the results have been successfully returned.
|
|
||
| //Page doesn't exist | ||
| if (luceneResults.TotalHits < minResults) | ||
| return luceneResults; |
There was a problem hiding this comment.
Why increase the nesting depth with this change from a return if the total hits were less than mins?
There was a problem hiding this comment.
this is not a new change in this PR.
There was a problem hiding this comment.
@zyhfish Yes, this is a change in this PR. You removed the old process which returned early, to an inversed logic that increases the IL nesting depth of the code
There was a problem hiding this comment.
It's amusing that with the logical expression reversed the comment above is not incorrect :)
|
is this PR able to merge? there have more changes was required based on this change, so need it merged then can continue that part work, thx |
fixes #2183