search

avniproject / integration-service

Service for integration Avni with other systems
GNU Affero General Public License v3.0
0 stars 2 forks source link

Error handling for LAHI module #94

Closed vinayvenu closed 11 months ago

vinayvenu commented 1 year ago

Implement error handling as per the design document.

Additionally, when an error gets resolved, delete the errorRecord corresponding to that entity.

vinayvenu commented 12 months ago
  1. https://github.com/avniproject/integration-service/commit/77a6cac2d66c50ea8bec46b816b2fb72de46ce5c#diff-68aa5d2b8026378623c6044b51ff7faef03299e0b33662693c87ca7920a5c8e1R57 registration_flow_complete is a custom field that might not be present in other Glific FlowResult objects.
  2. Unused LahiStudentService.STUDENT_FETCH_QUERY and LahiStudentService.getStudent()
  3. Since we've moved out the filtering of students in the Students class, we can have a much simpler iterator and get rid of all the tests. Similar to https://github.com/avniproject/integration-service/blob/main/glific/src/main/java/org/avni_integration_service/glific/bigQuery/mapper/BigQueryResultsMapper.java#L11
  4. Erroring out incomplete rows can be troublesome because it will result in a lot of noise - https://github.com/avniproject/integration-service/commit/da06d6a0b964a0e46be40034b99d9dc8ed175812#diff-feadab25481805488a7f90d7189cb2168ef34c9283ca913e51e368b55799e123R94
  5. StudentWorker - For MessageUnprocessableException, log should be warn as these are expected scenarios that we are ignoring
  6. StudentErrorService.studentProcessingError and platformError have the same behaviour. Is this on purpose?
  7. Additionally, when an error gets resolved, delete the errorRecord corresponding to that entity.
petmongrels commented 12 months ago
  1. done
  2. this is required only for error flow, card for which is not played right now
  3. renamed stuff in the test to make it agnostic of the content of json. more test is fine.
  4. changed it so it is a single line message
  5. done
petmongrels commented 11 months ago

6 - to be taken care in the error retry card. have logged error so that admin can see what failed and fix mapping

himeshr commented 11 months ago

We need to handle 7.Additionally, when an error gets resolved, delete the errorRecord corresponding to that entity.

himeshr commented 11 months ago

Only review these commits https://github.com/avniproject/integration-service/commit/56f1513e75a7d9ebdea1fc4eec9727f786806b27 https://github.com/avniproject/integration-service/commit/2006fdd26cb9d04abef93b241033101b44fdc655

vinayvenu commented 11 months ago

For platform errors, it looks like the service will continue processing without updating any records. No tests, so cannot validate this. It will be useful to add unit tests to verify behaviour.

himeshr commented 11 months ago

For platform errors, it looks like the service will continue processing without updating any records. No tests, so cannot validate this. It will be useful to add unit tests to verify behaviour.

For platform errors, we'll stop the app as part of card #89, not doing it now, as we would not have a way to reprocess errored entities without having to manually move the offset past errored record dateTimes.

Tests for ErrorRecord will also have to be created in general as a separate card. We have very few Test cases setup for integration-service in general.

himeshr commented 11 months ago

I had deployed this onto staging env on 30 Nov 2023 and checked that integration works as expected after the change. And the corrected Subject's error records are indeed deleted after completing sync successfully.