-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat(backup): [DO NOT MERGE] support importing multiplatform backups [WPB-11230] #2163
base: develop
Are you sure you want to change the base?
Conversation
@@ -208,6 +239,16 @@ extension CoreDataStack { | |||
let backupStoreFile = backupDirectory.appendingPathComponent(databaseDirectoryName).appendingStoreFile() | |||
let metadataURL = backupDirectory.appendingPathComponent(metadataFilename) | |||
|
|||
let multiplatformBackupFileName = MPBackup().ZIP_ENTRY_DATA |
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.
What is MBPBackup()
and ZIP_ENTRY_DATA
?
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 was supposed to be a static constant accessed by MPBackup.ZIP_ENTRY_DATA
. It holds the name of the file within the zip that all MP Backups should have.
So, if it exists: it's a MP Backup.
Considering that this lib might have to deal with encryption/decryption as well... this might change and we might make it so the whole file becomes completely opaque to clients.
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.
Yeah it would make sense to me to simply pass the url and the importer throws an error if it's not what it expects.
) { | ||
// TODO: Figure out the actual self-user domain before importing | ||
let importer = MPBackupImporter(selfUserDomain: "wire.com") | ||
let result = importer.import(multiplatformBackupFilePath: mpBackupFile.path()) |
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 would have expected the import
to be asynchronous. Is it blocking?
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 is blocking at the moment.
I am considering making it async as well in the next iterations.
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.
Would it be async
or would it be a completion handler?
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 would look something like this:
https://github.com/rickclephas/KMP-NativeCoroutines?tab=readme-ov-file#swift-concurrency
let backupData = success.backupData | ||
for user in backupData.users { | ||
// TODO: Import users | ||
print("Imported User \(user)") | ||
} | ||
for conversation in backupData.conversations { | ||
// TODO: Import conversations | ||
print("Imported Conversation \(conversation)") | ||
} | ||
for message in backupData.messages{ | ||
// TODO: Import messages | ||
print("Imported Message \(message)") | ||
} |
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.
Does this load the entire backup contents into memory? If so, do you think it could be a problem?
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.
This is a really good point.
I thought of it in the past and it was my main issue when opting for Protobuf as the underlying backup format.
Although small and efficient, there aren't many parsers that are able to read it as a stream. AFAIK it is not recommended to use it for big data sets.
Since then, my idea switched to instead of having a single entry within the Zip file, to have multiple entries. Each entry being a Protobuf with a chunk of messages.
I was planning on just having a single big protobuf for the POC and then work internally within the library to make this more optimal in the future.
However, I am now seeing the opportunity to attack this issue earlier 🤔
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.
Breaking it up in to chunks for manageable batches (i.e one file for a bunch of objects) makes sense to me. From the consumer point of view, I would imagine a kind of paging API:
for conversations in await backup.conversations {
import(conversations)
}
for users in await backup.users {
import(users)
}
for messages in await backup.messages {
import messages
}
Since these pages would be delivered asynchronously, the library could internally manage its memory consumption by parsing protobuf objects in batches.
Issue
We have different backup formats on different platforms. Not great for users.
The goal is to have a single format for all platforms, and in the future we can use this backup to sync history between clients. Like client A sending its history to client B when logging in, etc.
Important notes
Because of [2], the
backup.xcframework
is not properly published to a regular dependency repository, but rather being added to the project just to get quick feedback before actually setting up automated deployment.Testing
Checklist
[WPB-XXX]
.