-
Notifications
You must be signed in to change notification settings - Fork 15
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
refactor: integrate new incremental sync - WPB-15440 #2579
base: develop
Are you sure you want to change the base?
Conversation
…wireapp/wire-ios into refactor/integrate-sync-agent-wpb-10801
# Conflicts: # WireDomain/Sources/WireDomain/Components/ClientSessionComponent.swift # WireDomain/Sources/WireDomain/Components/UserSessionComponent.swift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like Github is getting confused by a deleted file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Encoding and decoding between UpdateEventEnvelope
and Data
moved inside the local store.
/// Delete the event envelope with the given index. | ||
/// - parameter index: The index of the envelope to delete | ||
|
||
func deleteEventEnvelope( | ||
atIndex index: Int64 | ||
) async throws |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this so we can store and delete live events (events that come through the push channel one by one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted because it's dead and no longer needed.
do { | ||
if let incrementalSyncTask { | ||
WireLogger.sync.info("incremental sync already running, waiting for it instead") | ||
try await incrementalSyncTask.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: interesting, when does the task is set to nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I forgot to clear it after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 532b5d8
logger.debug("pulling pending events") | ||
try await updateEventsSync.pull() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: this is just a fetch /notifications here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't have expected we fetch nor process the events before opening the channel, but I might forget something here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fetches events from backend, decrypts, and stores them in the event database. The sequence is:
- Open push channel, but buffer events (don't even decrypt them)
- Fetch, decrypt, and store events in the DB
- Fetch and process events from the DB
- Decrypt and process buffered events from push channel
- Decrypt and process new events from push channel
try await processStoredEvents() | ||
|
||
return Task { @Sendable [logger, decryptor, store, processor] in | ||
let jsonEncoder = JSONEncoder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: shouldn't we wrapped in a do catch here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think much about how we handle errors in event processing at the moment because we have another ticket for that. I suppose here we want to handle errors differently, for example:
- Decryption errors: log and ignore?
- Processing errors: keep in DB, log and continue?
Issue
Please describe the issue.
Optional: add details about technical approach, solutions etc.
Optional: reference dependencies to other pull requests etc.
Testing
Describe how to test.
Optional: attachments like images, videos, etc.
Checklist
[WPB-XXX]
.UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: