-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[messaging] Add message deletion during partial sync #4972
[messaging] Add message deletion during partial sync #4972
Conversation
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.
@Weiko I cannot test the behavior but the code looks good to me. It would be good that @bosiraphael tests it.
Regarding full-sync, can we go over each existing message and query google? If we get 404 we remove?
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.
LGTM, left a comment
@@ -81,9 +81,9 @@ import { MessageChannelObjectMetadata } from 'src/modules/messaging/standard-obj | |||
MessageChannelObjectMetadata, | |||
EventObjectMetadata, | |||
]), | |||
GmailFullSynV2Module, | |||
GmailFullSynModule, |
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.
Typo GmailFullSynModule
Context
Note: deletion for full-sync is a bit tricky because we need the full list of message ids to compare with the DB and make sure we don't over-delete. Currently, to keep memory, we don't have a variable that holds all ids as we flush it after each page. Easier solution would be to wipe everything before each full sync but it's probably not great for the user experience if they are currently manipulating messages since full-sync can happen without a user intervention (if a partial sync fails due to historyId being invalidated by google for some reason)