-
Notifications
You must be signed in to change notification settings - Fork 24
feat(sync): add ID-targeted loading and per-dependency lazy loading #545
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
Changes from all commits
e74ae98
4d8ea86
89349ad
4ddaf6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -625,7 +625,21 @@ def _resource_connections(self, resource_type: str, _id: str) -> Tuple[Set[Tuple | |
| # After retrieving all of the failed connections, we check if | ||
| # the resources are imported. Otherwise append to missing with its type. | ||
| for f_id in failed: | ||
| # With --minimize-reads, dependency types may not be in the | ||
| # initial scoped load. Lazily load this specific dependency | ||
| # (source+destination) so the source check below is accurate, | ||
| # and so connect_resources() in _apply_resource_cb() can | ||
| # successfully remap the ID in the destination. | ||
| self.config.state.ensure_resource_loaded(resource_to_connect, f_id) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR description calls out a known limitation: Silent ID-remapping failure is very hard to detect in production — the sync appears to succeed but references in the destination resource are stale. Even if fixing the root cause is deferred, the failure should be surfaced: # After ensure_resource_loaded + source check:
if f_id in self.config.state.source[resource_to_connect]:
# it loaded successfully
else:
if self.config.state._minimize_reads:
log.warning(
'minimize-reads: dependency %s.%s not found in storage; '
'ID remapping may be incomplete', resource_to_connect, f_id
)
missing_resources.add((resource_to_connect, f_id))At minimum, the test plan should include a case that syncs a monitor widget in a dashboard (which exercises the monitors override path) to confirm it doesn't silently break.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Added a |
||
|
|
||
| if f_id not in self.config.state.source[resource_to_connect]: | ||
| if self.config.state._minimize_reads: | ||
| self.config.logger.warning( | ||
| "minimize-reads: dependency %s.%s not found in storage; " | ||
| "ID remapping may be incomplete", | ||
| resource_to_connect, | ||
| f_id, | ||
| ) | ||
| missing_resources.add((resource_to_connect, f_id)) | ||
|
|
||
| failed_connections.add((resource_to_connect, f_id)) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.