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: Add feature for copying backups #1153
feat: Add feature for copying backups #1153
Conversation
system-test/bigtable.ts
Outdated
assert.deepStrictEqual(backup.expireDate, sourceExpireTime); | ||
} | ||
// Create client, instance, cluster for second project | ||
const bigtable = new Bigtable( |
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.
nit - I would rename this to something that signifies it's the second project. Maybe bigtableSecondaryProject
?
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.
or bigtableDestinationProject
?
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
system-test/bigtable.ts
Outdated
? {projectId: process.env.GCLOUD_PROJECT2} | ||
: {} | ||
); | ||
const instanceId = generateId('instance'); |
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.
nit - I think we can inline this to the line below
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
system-test/bigtable.ts
Outdated
: {} | ||
); | ||
const instanceId = generateId('instance'); | ||
const instance = bigtable.instance(instanceId); |
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.
as above, I would rename this to show it's the second project instance
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
@@ -1414,6 +1421,269 @@ describe('Bigtable', () => { | |||
|
|||
Object.keys(policy).forEach(key => assert(key in updatedPolicy)); | |||
}); | |||
describe('copying backups', () => { | |||
const sourceExpireTimeMilliseconds = | |||
PreciseDate.now() + (8 + 300) * 60 * 60 * 1000; |
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's the reasoning behind this arithmetic? I'm sure there is some, but to an outsider, all I think of is PEMDAS 🙂 Consider adding a comment
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.
My local system tests are hitting quotas so it's hard to recall exactly by testing it out. But if I recall correctly, the create backup expire time has to be sufficiently far ahead of the current time and the copied backup expire time has to be sufficiently ahead of the create backup expire time to avoid server errors specific to the copy backup feature. I added a comment for this.
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.
Thanks for the comment! Makes sense as to why it needs to be a large number. My remaining confusion is why the arithmetic needs to be there rather than adding a fixed integer to PreciseDate.now()?
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.
To your point, I think we can change this to 308 * 60 * 60 * 1000
and 608 * 60 * 60 * 1000
, but I want to keep the 60 * 60 * 1000 in there because it is beneficial to the reader to know that 308 is the number of hours (milliseconds/second * seconds/minute * minutes/hour).
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.
ah, thank you! I agree with the 60 * 60 * 1000 making sense. I made a suggestion to modify the comment
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.
Suggestions applied
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.
approving once suggestions with comments are added and with the assumption that the failing conformance tests are unrelated
@@ -1414,6 +1421,269 @@ describe('Bigtable', () => { | |||
|
|||
Object.keys(policy).forEach(key => assert(key in updatedPolicy)); | |||
}); | |||
describe('copying backups', () => { | |||
const sourceExpireTimeMilliseconds = | |||
PreciseDate.now() + (8 + 300) * 60 * 60 * 1000; |
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.
ah, thank you! I agree with the 60 * 60 * 1000 making sense. I made a suggestion to modify the comment
Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>
Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>
This PR includes changes in the veneer layer that implement the new copy backup feature which allows users to copy a backup of a table to a cluster of their choice.
A config interface is introduced which is used to specify details about the copied backup. On a backup object, a copy function is added which copies the backup to a destination specified by the config passed into the function parameter. Code that parses expiry time is reused for the copy backup function.
Unit tests are added to make sure source and destination information get passed along to the Gapic layer. System tests are added to check that copying a backup works when copying to the same cluster, different cluster and different project. System tests are also added to be sure that expiry times can be passed in using different formats and tests are added so that we can confirm restoring the copied backup works properly.