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

Fewer queries when caching hearing request types #15478

Merged
merged 7 commits into from
Oct 26, 2020

Conversation

hschallhorn
Copy link
Contributor

@hschallhorn hschallhorn commented Oct 21, 2020

Resolves some of the runtime issues for the update_cached_appeals_attributes_job

Description

Do not build an appeal for each vacols case we cache. This produced 4 calls to vacols for every single appeal we cache and makes the runtime of the job quite high.

...
[2020-10-21 16:45:07 -0400]   LegacyAppeal Load (0.4ms)  SELECT  "legacy_appeals".* FROM "legacy_appeals" WHERE "legacy_appeals"."vacols_id" = $1 LIMIT $2  [["vacols_id", "6210367160"], ["LIMIT", 1]]
[2020-10-21 16:45:07 -0400]   VACOLS::Correspondent Load (3.0ms)  SELECT  "CORRES".* FROM "CORRES" WHERE "CORRES"."STAFKEY" = :a1 FETCH FIRST :a2 ROWS ONLY  [["stafkey", "1972342395"], ["LIMIT", 1]]
[2020-10-21 16:45:07 -0400]   VACOLS::Folder Load (3.1ms)  SELECT  "FOLDER".* FROM "FOLDER" WHERE "FOLDER"."TICKNUM" = :a1 FETCH FIRST :a2 ROWS ONLY  [["ticknum", "6210367160"], ["LIMIT", 1]]
[2020-10-21 16:45:07 -0400]   VACOLS::CaseIssue Load (4.1ms)  SELECT "ISSUES".* FROM "ISSUES" WHERE "ISSUES"."ISSKEY" = :a1  [["isskey", "6210367160"]]
...
# Four of these calls for each appeal we're caching
vacols_ids = LegacyAppeal.pluck(:vacols_id)
# only 31 appeals

def former_travel?(legacy_appeal)
  # the current request type is travel
  if legacy_appeal.current_hearing_request_type == :travel_board
    return false
  end

  # otherwise check if og request type was travel
  legacy_appeal.original_hearing_request_type == :travel_board
end

def old_call(vacols_ids)
  VACOLS::Case.where(bfkey: vacols_ids).pluck(:bfkey, :bfac, :bfcurloc).map do |bfkey, bfac, bfcurloc|
    [
      bfkey,
      {
        location: bfcurloc,
        status: VACOLS::Case::TYPES[bfac]
      }
    ]
  end.to_h
end

def new_call(vacols_ids)
  VACOLS::Case.where(bfkey: vacols_ids).map do |vacols_case|
    legacy_appeal = AppealRepository.build_appeal(vacols_case) # build non-persisting legacy appeal object

    [
      vacols_case.bfkey,
      {
        location: vacols_case.bfcurloc,
        status: VACOLS::Case::TYPES[vacols_case.bfac],
        hearing_request_type: legacy_appeal.current_hearing_request_type(readable: true),
        former_travel: former_travel?(legacy_appeal)
      }
    ]
  end.to_h
end

def new_new_call(vacols_ids)
  VACOLS::Case.where(bfkey: vacols_ids).map do |vacols_case|
    original_request = original_hearing_request_type_for_vacols_case(vacols_case)
    changed_request = changed_hearing_request_type_for_all_vacols_ids[vacols_case.bfkey]
    current_request = HearingDay::REQUEST_TYPES.key(changed_request)&.to_sym || original_request

    [
      vacols_case.bfkey,
      {
        location: vacols_case.bfcurloc,
        status: VACOLS::Case::TYPES[vacols_case.bfac],
        hearing_request_type: LegacyAppeal::READABLE_HEARING_REQUEST_TYPES[current_request],
        former_travel: original_request == :travel_board && current_request != :travel_board
      }
    ]
  end.to_h
end

def original_hearing_request_type_for_vacols_case(vacols_case)
  request_type = VACOLS::Case::HEARING_REQUEST_TYPES[vacols_case.bfhr]

  (request_type == :travel_board && vacols_case.bfdocind == "V") ? :video : request_type
end

def changed_hearing_request_type_for_all_vacols_ids
  @changed_hearing_request_type_for_all_vacols_ids ||= LegacyAppeal.pluck(:vacols_id, :changed_request_type).to_h
end

Benchmark.measure { old_call(vacols_ids) }
=> #<Benchmark::Tms:0x00007ff5c6cd9100
 @cstime=0.0,
 @cutime=0.0,
 @label="",
 @real=0.3174909995868802,
 @stime=0.017438999999999538,
 @total=0.07847399999999816,
 @utime=0.06103499999999862>

Benchmark.measure { new_call(vacols_ids) }
=> #<Benchmark::Tms:0x00007ff5c16471e8
 @cstime=0.0,
 @cutime=0.0,
 @label="",
 @real=2.2098000003024936,
 @stime=0.14571100000000037,
 @total=1.3108650000000015,
 @utime=1.1651540000000011>

Benchmark.measure { new_new_call(vacols_ids) }
=> #<Benchmark::Tms:0x00007fec37238ba8
 @cstime=0.0,
 @cutime=0.0,
 @label="",
 @real=0.4909099997021258,
 @stime=0.029617000000000004,
 @total=0.19788100000000064,
 @utime=0.16826400000000064>

Acceptance Criteria

  • Cached appeal attributes are still correct
  • Runtime of update cached appeal attributes job is shorter

Testing Plan

  1. on master in dev
Benchmark.measure { UpdateCachedAppealsAttributesJob.new.perform_now }
=> #<Benchmark::Tms:0x00007fd1cdaed138
 @cstime=0.045598,
 @cutime=0.03593299999999999,
 @label="",
 @real=4.578323000110686,
 @stime=0.26611799999999985,
 @total=3.2173869999999996,
 @utime=2.869738>
  1. on this branch in dev, exit console and come back in
Benchmark.measure { UpdateCachedAppealsAttributesJob.new.perform_now }
=> #<Benchmark::Tms:0x00007ff557c029a8
 @cstime=0.033809,
 @cutime=0.032311999999999994,
 @label="",
 @real=3.9726599999703467,
 @stime=0.2307260000000002,
 @total=2.7981360000000004,
 @utime=2.501289>

If you're feeling fun

  1. In prod
# Grab 100 appeals
appeal_ids = Task.open.where(appeal_type: LegacyAppeal.name).pluck(:appeal_id).uniq.first(100)
vacols_ids = LegacyAppeal.where(id: appeal_ids).pluck(:vacols_id)

def new_call(vacols_ids)
  VACOLS::Case.where(bfkey: vacols_ids).map do |vacols_case|
    legacy_appeal = AppealRepository.build_appeal(vacols_case) # build non-persisting legacy appeal object

    [
      vacols_case.bfkey,
      {
        location: vacols_case.bfcurloc,
        status: VACOLS::Case::TYPES[vacols_case.bfac],
        hearing_request_type: legacy_appeal.current_hearing_request_type(readable: true),
        former_travel: former_travel?(legacy_appeal)
      }
    ]
  end.to_h
end

def former_travel?(legacy_appeal)
  # the current request type is travel
  if legacy_appeal.current_hearing_request_type == :travel_board
    return false
  end

  # otherwise check if og request type was travel
  legacy_appeal.original_hearing_request_type == :travel_board
end

Benchmark.measure { new_call(vacols_ids) }
=> #<Benchmark::Tms:0x000000000e426bd0 @label="", @real=48.00420006499917, @cstime=0.0, @cutime=0.0, @stime=0.06409600000000015, @utime=1.2459829999999998, @total=1.310079>
  1. Exit the console and reenter
appeal_ids = Task.open.where(appeal_type: LegacyAppeal.name).pluck(:appeal_id).uniq.first(100)
vacols_ids = LegacyAppeal.where(id: appeal_ids).pluck(:vacols_id)

def new_new_call(vacols_ids)
  VACOLS::Case.where(bfkey: vacols_ids).map do |vacols_case|
    original_request = original_hearing_request_type_for_vacols_case(vacols_case)
    changed_request = changed_hearing_request_type_for_all_vacols_ids[vacols_case.bfkey]
    current_request = HearingDay::REQUEST_TYPES.key(changed_request)&.to_sym || original_request

    [
      vacols_case.bfkey,
      {
        location: vacols_case.bfcurloc,
        status: VACOLS::Case::TYPES[vacols_case.bfac],
        hearing_request_type: LegacyAppeal::READABLE_HEARING_REQUEST_TYPES[current_request],
        former_travel: original_request == :travel_board && current_request != :travel_board
      }
    ]
  end.to_h
end

def original_hearing_request_type_for_vacols_case(vacols_case)
  request_type = VACOLS::Case::HEARING_REQUEST_TYPES[vacols_case.bfhr]

  (request_type == :travel_board && vacols_case.bfdocind == "V") ? :video : request_type
end

def changed_hearing_request_type_for_all_vacols_ids
  @changed_hearing_request_type_for_all_vacols_ids ||= LegacyAppeal.pluck(:vacols_id, :changed_request_type).to_h
end

Benchmark.measure { new_new_call(vacols_ids) }
=> #<Benchmark::Tms:0x0000000021a57258 @label="", @real=12.882008823999058, @cstime=0.0, @cutime=0.0, @stime=0.2183029999999999, @utime=3.3765739999999997, @total=3.5948769999999994>

@codeclimate
Copy link

codeclimate bot commented Oct 21, 2020

Code Climate has analyzed commit 3d9d49a and detected 0 issues on this pull request.

View more on Code Climate.

@@ -267,20 +267,34 @@ def case_fields_for_vacols_ids(vacols_ids)
# ...
# }
VACOLS::Case.where(bfkey: vacols_ids).map do |vacols_case|
legacy_appeal = AppealRepository.build_appeal(vacols_case) # build non-persisting legacy appeal object
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call will load the legacy appeal, vacols folder, vacols correspondent, and vacols issues for each vacols case here when the vacols case actually has almost all the information we need!

@hschallhorn hschallhorn self-assigned this Oct 21, 2020
@hschallhorn hschallhorn changed the title Fewer n+1 queries when grabbing hearing request types Fewer queries when grabbing hearing request types Oct 21, 2020
@hschallhorn hschallhorn changed the title Fewer queries when grabbing hearing request types Fewer queries when caching hearing request types Oct 21, 2020
…-of-veterans-affairs/caseflow into hschallhorn/ucaaj-performance
Copy link

@rubaiyat22 rubaiyat22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! the logic looks correct and the tests are all passing

def original_hearing_request_type_for_vacols_case(vacols_case)
request_type = VACOLS::Case::HEARING_REQUEST_TYPES[vacols_case.bfhr]

(request_type == :travel_board && vacols_case.bfdocind == "V") ? :video : request_type

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very short and concise!

@hschallhorn hschallhorn added the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Oct 26, 2020
@va-bot va-bot merged commit ecd819c into master Oct 26, 2020
@va-bot va-bot deleted the hschallhorn/ucaaj-performance branch October 26, 2020 16:40
va-bot pushed a commit that referenced this pull request Oct 28, 2020
Resolves #15426 

### Description

- Moved `FetchHearingLocationsForVeteransJob` logic to `GeomatchService`
- Moved `UpdateCachedAppealsAttributesJob` logic to `CachedAppealService`
  - Refactored some existing logic to get rid of some CodeClimate warnings. The remaining ones I think we can ignore
- Add `Hearings::GeomatchAndCacheAppealJob`, and start it when completing `ChangeHearingRequestTypeTask`
- Update and add tests
- Resolves merge conflicts from #15482 and #15478

### Acceptance Criteria
- [x] When a user completes the "Convert hearing to virtual" action on a Travel Board hearing, it immediately shows up in the geo-matched RO queue

### Testing Plan
1. Login as BVASYELLOW
2. Go to http://localhost:3000/queue/appeals/1986897
3. Work the change appeal type task
4. Go to the schedule veteran page for St. Petersburg, and ensure the case shows up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Eng: Performance Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants