-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
Generate changelog in
|
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 looks good, haven't double-checked that it matches internal logic. If it's not difficult, we could smoke test it (I guess this is the task you mentioned in standup?), but it's also ok to go ahead with this and do an integration test down the line
@@ -91,6 +89,7 @@ public static AtlasBackupService create( | |||
|
|||
private void storeBackupToken(InProgressBackupToken backupToken) { | |||
inProgressBackups.put(backupToken.getNamespace(), backupToken); | |||
backupPersister.storeImmutableTimestamp(backupToken); |
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.
Let's add a todo to eventually stop doing this. We want it now for backwards compatibility, but if there is not a valid use case for it (maybe it's used to repair targeted sweep tables, haven't checked?), we should get rid of it when reasonable.
Goals (and why): Persist backup metadata in the same way as in the internal backup product.
Implementation Description (bullets):
Testing (What was existing testing like? What have you done to improve it?): Added ExternalBackupPersisterTest
Concerns (what feedback would you like?): Storing the immutable timestamp seems a little janky - it appears to be used during the restore process though, so it would be needed at least for back-compatibility.
Where should we start reviewing?: ExternalBackupPersister
Priority (whenever / two weeks / yesterday): today 🙏