Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#95986 rescue db connection error #19457

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kayline
Copy link
Contributor

@kayline kayline commented Nov 13, 2024

Summary

  • This work is behind a feature toggle (flipper): NO
  • Add rescue to status polling job for fetching initial set of records
  • Decision Review, yes

Related issue(s)

department-of-veterans-affairs/va.gov-team#95986

Testing done

  • New code is covered by unit tests
  • If fetching the saved claims failed, the entire job would fail and not retry. Now it rescues, logs, and the job will run again in 1 hour

What areas of the site does it impact?

Better error handling for background jobs

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No error nor warning in the console.
  • Events are being sent to the appropriate logging solution
  • Documentation has been updated (link to documentation)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • Feature/bug has a monitor built into Datadog (if applicable)
  • If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
  • I added a screenshot of the developed feature

- secondary form handling needs more work
- TODO: shared examples for tests
- now depends on a method in each subclass of the status updater
- add tests for correct statsd increment value
- add rescus to base class
- add tests to each subclass
Copy link

1 Warning
⚠️ This PR changes 493 LoC (not counting whitespace/newlines).

In order to ensure each PR receives the proper attention it deserves, we recommend not exceeding
200. Expect some delays getting reviews.

File Summary

Files

  • app/sidekiq/decision_review/hlr_status_updater_job.rb (+30/-0)

  • app/sidekiq/decision_review/nod_status_updater_job.rb (+33/-0)

  • app/sidekiq/decision_review/saved_claim_hlr_status_updater_job.rb (+0/-59)

  • app/sidekiq/decision_review/saved_claim_nod_status_updater_job.rb (+0/-94)

  • app/sidekiq/decision_review/saved_claim_status_updater_job.rb (+161/-0)

  • app/sidekiq/decision_review/sc_status_updater_job.rb (+36/-0)

  • lib/periodic_jobs.rb (+3/-3)

  • spec/sidekiq/decision_review/{saved_claim_hlr_status_updater_job_spec.rb => hlr_status_updater_job_spec.rb} (+21/-1)

  • spec/sidekiq/decision_review/{saved_claim_nod_status_updater_job_spec.rb => nod_status_updater_job_spec.rb} (+25/-1)

  • spec/sidekiq/decision_review/{saved_claim_sc_status_updater_job_spec.rb => sc_status_updater_job_spec.rb} (+25/-1)

    Note: We exclude files matching the following when considering PR size:

    *.csv, *.json, *.tsv, *.txt, *.md, Gemfile.lock, app/swagger, modules/mobile/docs, spec/fixtures/, spec/support/vcr_cassettes/, modules/mobile/spec/support/vcr_cassettes/, db/seeds, modules/vaos/app/docs, modules/meb_api/app/docs, modules/appeals_api/app/swagger/, *.bru, *.pdf
    

Big PRs are difficult to review, often become stale, and cause delays.

Generated by 🚫 Danger

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants