feat: DAH-2556 check translations cache LastModifiedDate #2283
Conversation
cade-exygy
changed the title
| def cache_listing_translations(listing_id, keys, translations, last_modified) | ||
| Force::Request.new(parse_response: true).cached_get( | ||
| "/ListingDetails/#{CGI.escape(listing_id)}", nil, true | ||
| ) |
There was a problem hiding this comment.
question: It's not entirely clear to me why cached_get was added here. Don't we do that earlier in the chain of method calls?
There was a problem hiding this comment.
Adding it here would guarantee that the listing is refreshed anytime we run translations for a listing. (Either by event trigger or a full process call).
This solves the issue of https://sfgovdt.jira.com/browse/DAH-2575, as well as ensures the last modified date of the listing is immediately correct for when the listing service is hit.
Testing through the different cases in the review instructions may provide some clarity to my above open question.
There was a problem hiding this comment.
Based on our convo in the translations sync, can we remove the force update listing call here?
| @@ -53,7 +54,10 @@ def self.listing(id, opts = {}) | |||
| listing | |||
| end | |||
|
|
|||
| def self.translations_valid?(listing_translations) | |||
| def self.translations_valid?(listing_translations, listing_last_modified) | |||
| return false unless listing_translations[:LastModifiedDate] == listing_last_modified | |||
There was a problem hiding this comment.
question: This seems to duplicate the method CacheService#listing_unchanged?, which gets run every 10 minutes via the prefetch job. Does this make the prefetch job redundant now? I.e. do we still need a background process to update caches for all modified listings every 10 minutes, when we are now doing it on every visit to a listing details page?
There was a problem hiding this comment.
This is a good question and is related to the open question in the description.
This function compares the listing LastModifiedDate and the translations cache LastModifiedDate.
This returns false when:
-
A listing has been updated but the event subscriber failed to update the translations.
-
With the force=true call in the google_translate_service, we should always refresh the listing object when we update translations. So in theory, the LastModifiedDate of the listing should be guaranteed to be accurate for the purposes of the comparison.
If this function is false, then we process translations -> force update the listing -> translate all fields.
So yes, if the event subscriber is working -> then everytime we update a listing -> we translate and force update the listing. We could probably remove the prefetch job in this case.
If the event subscriber is down for some reason, then we would not update translations or force refresh the listing.
If we disable the prefetch job under this scenario: The listing wouldn't reflect the new changes until a manual force refresh. And we wouldn't catch the disagreement on the listing service call.
We could remove the force=true call from the google translate service, and then this check in the listing detail would return false due to the translations being newer. (Unless the prefetch job gets it first). The intended behavior would be to refresh the listing. Then the dates should match.
This ticket primarily aims to resolve the potential complications of the event subscriber failing. We do plan on implementing event replays so that is a future consideration.
Hopefully this provides some insight into the potential paths forward. We could keep the prefetch job and introduce some redundancy that would cover all of the potential scenarios.
We could remove the force=true call from the google_translation service, and just let the listing_service be responsible for updating the listing cache. Perhaps that would simplify things for us.
Maybe we should make the LastModifiedDate comparison have two outcomes: if the listing date is older than the translations -> refresh the listing. If the translation date is older than the listing -> refresh the translations.
| if listing_is_outdated?(listing_translations[:LastModifiedDate], | ||
| listing['LastModifiedDate']) | ||
| Rails.logger.info("Listing is outdated for #{listing_id}") | ||
| refresh_listing_cache(listing_id) |
There was a problem hiding this comment.
I thought we discussed not refreshing the listing cache in the translations flow?
There was a problem hiding this comment.
We removed it from the translation service. We do want to refresh the listing when we know that the listing is outdated.
There was a problem hiding this comment.
That wasn't my understanding. I thought we wanted to avoid updating the listing cache on page load - if we update on page load than the cache is moot.
There was a problem hiding this comment.
I discussed this above:
Maybe we should make the LastModifiedDate comparison have two outcomes: if the listing date is older than the translations -> refresh the listing. If the translation date is older than the listing -> refresh the translations.
But it is a fair question. Relates to this ticket as well: https://sfgovdt.jira.com/browse/DAH-2575
We can wait for the 10 minutes: which means english values will be out of date until a listing recache if we update translations via an event. If we have an event miss, then we would have to wait the 10 minutes to resolve those differences as well.
There was a problem hiding this comment.
We discussed in a meeting and decided to move forward with updating the listing cache!
Description
This PR adds methods to compare the LastModifiedDate of the listing to the LastModifiedDate of the translations cache. Current behavior is when the listing modified date is newer than the translation cache, reprocess translations. If the listing modified date is older than the translations, refresh the listing.
Includes refactoring that breaks down existing functionality into smaller methods.
DAH-2556
Checklist before requesting review
Version Control
angularif it contains updates to Angular codetype: TICKET-NUMBER Descriptionformat, e.g.feat: DAH-123 New FeatureCode quality
Review instructions
Request review
needs reviewlabelHousing Enggroup to automatically assign reviewers, and/or assign specific engineersReview
For engineers locally:
Run
memcachedRun
yarn startRun
bundle exec rake translate:subscribeLoad a listing detail page -> since a listing will initially have no translations cache, this should trigger the translations to be processed.
Refresh page -> The listing's translation cache should be valid -> no translations call
Update a field in Salesforce -> Event processor updates cache -> LastModifiedDates should match when the listing cache is refreshed next
Refresh page -> (May refresh listing) -> Should not reprocess translations
Kill the Eventsubscriber process -> Update a field in salesforce -> No translation call
Refresh page -> LastModifiedDates should be different -> translations are reprocessed