Improve Sync To User performance (batch 2) #1897
Conversation
kitallis
force-pushed
the
sync-perf-2
branch
from
a6a8bd4 to
d28b044
Compare
kitallis
force-pushed
the
sync-perf-2
branch
from
d28b044 to
c0dc93f
Compare
kitallis
force-pushed
the
sync-perf-2
branch
from
c0dc93f to
886f8d3
Compare
kitallis
force-pushed
the
sync-perf-2
branch
from
886f8d3 to
1ef492c
Compare
However, this code-path for now should not really be called atm.
| def records_to_sync | ||
| current_facility_records + other_facility_records | ||
| def model | ||
| controller_name.classify.constantize.for_sync |
There was a problem hiding this comment.
I think I understand why you are returning a AR scope here instead of the actual model, but its a confusing mismatch with the name and conventions around something named model. Maybe rename the method to something like scope to better represent that its the base scope for other methods to build off of? Or maybe region_scope, sync_scope, default_sync_scope ?
There was a problem hiding this comment.
Yeah, I wasn't happy with continuing to calling it model. We'd removed model earlier because I disliked its hanging presence with invisible meanings. But I think sync_scope makes sense 👍🏼
kitallis
force-pushed
the
sync-perf-2
branch
from
45fa4ad to
4e18d68
Compare
kitallis
force-pushed
the
sync-perf-2
branch
from
4e18d68 to
5f0a890
Compare
| scope :syncable_to_region, ->(region) { | ||
| with_discarded.where(patient: Patient.syncable_to_region(region)) | ||
| } | ||
| scope :for_sync, -> { with_discarded } |
There was a problem hiding this comment.
Can we add a couple tiny tests for this scope across these models?
|
|
||
| def syncable_patients | ||
| registered_patients.with_discarded | ||
| end |
There was a problem hiding this comment.
Is this a bit dissonant? "Syncable" in general means you are registered in, assigned to, or plan to visit. We're using a simplified definition here because it doesn't really matter when used for facility prioritization. However, does this set us up to do things like facility-level sync incorrectly?
There was a problem hiding this comment.
To address the dissonance, I suspect we'd either:
- update the query here to include all three clauses - This doesn't seem correct because it doesn't maintain feature parity in the sync API, and is also less performant probably
- update the name of this method? - Something along the lines of
prioritized_patientsor something? Making it clear that this boundary is not a facility-level sync boundary, but a looser boundary within another block-or-FG-level sync?
There was a problem hiding this comment.
prioritized_patients works I think 👍🏼
| region_records | ||
| .where(patient: prioritized_patients) | ||
| model_sync_scope | ||
| .where(patient: current_facility.syncable_patients.pluck(:id)) |
There was a problem hiding this comment.
Would a couple semantic methods help with readability?
def current_facility_patient_ids
@current_facility_patient_ids ||= current_facility.syncable_patients.pluck(:id)
end
def other_facility_patient_ids
@other_facility_patient_ids ||= current_sync_region.syncable_patients.pluck(:id) - current_facility_patient_ids
endThere was a problem hiding this comment.
They might, I can play around with this. But our general approach was to reduce the number of methods in this mixin. Every method that we add, is an opportunity for someone to override it in some random place in some controller that inherits from it. The thinner this mixin is in terms of pure method volume, the cleaner the internal API is.
I'll take a shot at this and see if it helps anyway. Will respond back here.
Story card: ch2086
Because
Whilst perf-testing block-syncs, we discovered various inefficencies in our sync (GET) code-paths that get exacerbated when many users re-sync at the same time.
A full list of improvements and the rationale is here.
This addresses
This improves how the overall query for fetching the records that are required to syncing are queried. Making some structural changes to construction of the overall query avoids making inefficient sub-selects.