Sync performance fixes #1921
Merged
Sync performance fixes #1921
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| end | ||
| end | ||
|
|
||
| # this is performance-critical code, be careful while refactoring it |
There was a problem hiding this comment.
Let's link to the issue in this comment too.
| @@ -90,5 +92,13 @@ def sync_region_modified? | |||
| return if process_token[:sync_region_id].blank? | |||
| process_token[:sync_region_id] != requested_sync_region_id | |||
| end | |||
|
|
|||
| def time(method_name, &block) | |||
| raise ArgumentError, "You must supply a block" unless block | |||
There was a problem hiding this comment.
Is this necessary? Can we just let the yield below blow up with a Block not provided?
harimohanraj89
approved these changes
Dec 30, 2020
There was a problem hiding this comment.
I think we've done a ton of research and test driving of these changes. They look good!
I have one crazy idea - what happens if we replace current_facility_records and other_facility_records with current_facility_records and all_facility_records?
Sure, we'll be sending the current facility records twice, but is that worth it - given the alternative is a gnarly codepath that requires cautionary comments and performance observation every time we touch it?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Story card: 2086
Because
We discovered that the perf improvements for block level sync tend to slow down FG syncing. We reverted the improvements (#1920) to unblock deploys while a fix was figured out.
This addresses
This combines the reverted perf improvements (#1898 and #1897) and fixes them to be FG compatible.
Note
Please use #1926 as reference for the changes here.