Fix/3994 sharepoint incremental sync skip sites - #4256
Fix/3994 sharepoint incremental sync skip sites#4256falscherwiener1-svg wants to merge 7 commits into
Conversation
Add per-item retry logic for transient HTTP status codes (429, 503) in the bulk ingestion sink. Previously, individual items that failed with transient errors inside a successful bulk response were sent directly to the ErrorMonitor without retry, which could trigger a premature TooManyErrors abort on a temporarily overloaded cluster. Changes: - Add TRANSIENT_STATUS_CODES constant (429, 503) - Add _determine_action() helper to extract the bulk action key from a response item - Add _extract_transient_failed_operations() to collect retryable items from a bulk response - Add _retry_transient_item_failures() with exponential backoff that retries only the failed items - Rewire _batch_bulk to call _retry_transient_item_failures before _process_bulk_response, so the ErrorMonitor only tracks failures after all retry attempts are exhausted - Remove the now-unused retryable import and the dead _bulk_api_call function that was never invoked Closes elastic#4002
…ns (elastic#4222) SharePoint list GUIDs are only unique within a site collection. When two site collections share a list GUID (e.g. via site templates or migration), list_item documents from different sites collide on the same _id and silently overwrite each other. Include site_id in the _id construction to guarantee global uniqueness.
…led (elastic#4139) The scheduling loop logs an ERROR every ~30s when a connector supports DLS but the Elasticsearch license is not Platinum. This is misleading when the user has not enabled access control sync at all. Only check the license when access control sync scheduling is actually enabled by the user. If it's disabled, skip silently.
…lastic#3994) Drive item deletions and modifications do not reliably update the parent site's lastModifiedDateTime in the Graph API. When check_timestamp=True, entire sites are skipped during incremental sync, silently losing drive item changes. Align with the existing pattern used for site_drives: set check_timestamp=False so all sites are visited, ensuring no changes are missed.
|
This PR incorporates the previous two (4254 and 4255). I assume you only want review of this one? |
|
|
||
| return failed_ops | ||
|
|
||
| async def _retry_transient_item_failures(self, operations, pipeline_name): |
There was a problem hiding this comment.
loses first-call successes from the return value. After a retry, res contains only the retried items. _batch_bulk then calls _process_bulk_response(res, ids_to_ops) where ids_to_ops maps all original operations, but res only has the retried subset. Items that succeeded on the first call won't be tallied in stats. Was that intentional?
There was a problem hiding this comment.
Good catch, that was not intentional. Items that succeeded on the first call were silently dropped from the returned response, which meant _process_bulk_response and _populate_stats never saw them.
I have updated _retry_transient_item_failures to track successful items across all calls and return a merged response containing both the originally successful items and the final retry outcome. See commit 564533c.
| if connector.features.document_level_security_enabled(): | ||
| if ( | ||
| connector.features.document_level_security_enabled() | ||
| and connector.access_control_sync_scheduling.get("enabled", False) |
There was a problem hiding this comment.
Guard against access_control_sync_scheduling not being set
There was a problem hiding this comment.
Yep, that is the intent. This guards against the case where DLS is enabled but access control sync scheduling has not been explicitly turned on. The .get("enabled", False) defaults to off, so the license check is only hit when the feature is actually configured.
| site_collection["siteCollection"]["hostname"], | ||
| self.configuration["site_collections"], | ||
| check_timestamp=True, | ||
| check_timestamp=False, |
There was a problem hiding this comment.
Do we have to worry about site_lists and site_list_items also?
There was a problem hiding this comment.
Good question. site_lists and site_list_items do not need this change, the issue is specific to the drive item path.
Drive items are fetched via delta links (Graph API delta queries, see self.client.drive_items() at line 717). The parent site/site_drive are only traversal containers to reach the drives. Since editing a drive_item does not update the parent lastModifiedDateTime, we must skip the timestamp check to always reach the delta-enabled drives.
Lists, list items, and pages do not use delta queries, they are fetched directly and rely on lastModifiedDateTime comparison against self.last_sync_time(). Modifying a list item does update the list item own lastModifiedDateTime, and creating/updating a list does update the list lastModifiedDateTime. So check_timestamp=True is correct for those.
The same reasoning already existed for site_drives() (line 705 comment), this PR just applies the same logic one level up to sites().
Previously, each retry call overwrote with only the retried subset, so items that succeeded on the first call were silently dropped from the returned response. This caused _process_bulk_response and _populate_stats to never see those items. Now successful items are tracked across all calls and a merged response is returned containing both the originally successful items and the final retry outcome. Addresses review feedback on elastic#4256.
|
@erikcurrin-elastic Yes, that is correct. PRs #4254 and #4255 have been closed in favor of this single PR which incorporates all changes. Please review only this one (#4256). I have also addressed the retry feedback in |
2 participants
What
Set
check_timestamp=Falseon thesites()call inget_docs_incrementallyso that all sites are visited during incremental sync.Closes #3994
Why
Drive item deletions and modifications do not reliably update the parent site's
lastModifiedDateTimein the Graph API. Withcheck_timestamp=True, entire sites are skipped — deleting drive items and modifications are silently lost until the next full sync.How
The codebase already uses this pattern for
site_drives()(line 705):