feat: use default ack mode for recommendation indexes and throw excep… #478
Conversation
…tions appropriately TIS21-5152: Process Change Stream messages quicker
...cation/src/main/java/uk/nhs/hee/tis/revalidation/service/ElasticSearchIndexUpdateHelper.java
Show resolved
Hide resolved
TIS21-5152: Process Change Stream messages quicker
TIS21-5152: Process Change Stream messages quicker
TIS21-5152: Process Change Stream messages quicker
TIS21-5152: Process Change Stream messages quicker
TIS21-5152: Process Change Stream messages quicker
TIS21-5152: Process Change Stream messages quicker
| @@ -50,8 +46,7 @@ public void saveRecommendationView(RecommendationView dataToSave) { | |||
| try { | |||
| recommendationElasticSearchRepository.save(dataToSave); | |||
| } catch (Exception ex) { | |||
| LOG.error("Exception in `saveRecommendationViews` (GmcId: {}; PersonId: {}): {}", | |||
| dataToSave.getGmcReferenceNumber(), dataToSave.getTcsPersonId(), ex); | |||
| throw new DoctorIndexUpdateException(ex.getMessage(), ex); | |||
There was a problem hiding this comment.
Not sure this is even needed?
| @@ -50,8 +46,7 @@ public void saveRecommendationView(RecommendationView dataToSave) { | |||
| try { | |||
| recommendationElasticSearchRepository.save(dataToSave); | |||
| } catch (Exception ex) { | |||
| LOG.error("Exception in `saveRecommendationViews` (GmcId: {}; PersonId: {}): {}", | |||
| dataToSave.getGmcReferenceNumber(), dataToSave.getTcsPersonId(), ex); | |||
| throw new DoctorIndexUpdateException(ex.getMessage(), ex); | |||
There was a problem hiding this comment.
If you think we should catch and add information, you've done the great thing of including the cause ex. That means the message will be included in any stacktrace etc. and you don't need to use it here. I'd create something specific to the context.
The stacktrace will show that it is thrown from RecommendationElasticSearchService.saveRecommendationView:49 but this message will be more visible so I like that the message logged included information about what was happening.
"Failed to save a recommendation view." is better than e.g. "Broken pipe (Write failed)", with or without extending that to include "Failed to save a recommendation view for doctor (GMC: %s, Person ID: %d)"
There was a problem hiding this comment.
I see you've also not certain about whether we should wrap any Exception. I think we'll be ok with the cause being thrown.
application/src/main/java/uk/nhs/hee/tis/revalidation/exception/DoctorIndexUpdateException.java
Show resolved
Hide resolved
TIS21-5152: Process Change Stream messages quicker
|
Do we want to include the retry config in the PR? If something does fail, the recommendation service will generate a lot of logs |
|
Kudos, SonarCloud Quality Gate passed!
|
It's worth looking at the retry config now but I'd like to be guided by whether you think this PR as-is forms a change that can run reliably. I'm happy it does represent an improvement to the reliability of the recommendation service. |
CaiWillis
deleted the
feat/useDefaultAckModeForRecommendationIndexUpdates
branch
…tions appropriately
TIS21-5152: Process Change Stream messages quicker