feat: Add feature for copying backups #1153

danieljbruce · 2022-09-09T19:08:46Z

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.

This reverts commit 576550d.

This reverts commit 46ce5b8.

system-test/bigtable.ts

test/backup.ts

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

kolea2 · 2024-04-30T17:47:12Z

system-test/bigtable.ts

+          assert.deepStrictEqual(backup.expireDate, sourceExpireTime);
+        }
+        // Create client, instance, cluster for second project
+        const bigtable = new Bigtable(


nit - I would rename this to something that signifies it's the second project. Maybe bigtableSecondaryProject?

or bigtableDestinationProject?

kolea2 · 2024-04-30T17:48:08Z

system-test/bigtable.ts

+            ? {projectId: process.env.GCLOUD_PROJECT2}
+            : {}
+        );
+        const instanceId = generateId('instance');


nit - I think we can inline this to the line below

kolea2 · 2024-04-30T17:48:23Z

system-test/bigtable.ts

+            : {}
+        );
+        const instanceId = generateId('instance');
+        const instance = bigtable.instance(instanceId);


as above, I would rename this to show it's the second project instance

…com/danieljbruce/nodejs-bigtable into copy-backup-2-relevant-changes-only

src/cluster.ts

leahecole · 2024-05-13T14:59:06Z

system-test/bigtable.ts

@@ -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;


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

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.

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()?

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).

ah, thank you! I agree with the 60 * 60 * 1000 making sense. I made a suggestion to modify the comment

Suggestions applied

system-test/bigtable.ts

test/backup.ts

leahecole

approving once suggestions with comments are added and with the assumption that the failing conformance tests are unrelated

system-test/bigtable.ts

leahecole · 2024-05-21T18:05:30Z

system-test/bigtable.ts

@@ -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;


ah, thank you! I agree with the 60 * 60 * 1000 making sense. I made a suggestion to modify the comment

system-test/bigtable.ts

Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>

danieljbruce added 16 commits July 29, 2022 13:34

src test and protos copied over

46ce5b8

lint fix

576550d

implemented copy backup

31a1d9a

part of test done

90e290e

workaround

7bce08d

corrections to get code working

29a736f

Add expire time

edc30d3

This allows the copy backup endpoint to work

c8a16fc

Working system test

2d5320c

delete try block

adabd71

tests pass with new refactor

89f742e

Create backup and copy on another cluster

d4f5d47

modifications to second test

ea43929

copy backup unit test

bb6e708

Revert "lint fix"

c96442a

This reverts commit 576550d.

Revert "src test and protos copied over"

701badc

This reverts commit 46ce5b8.

danieljbruce requested review from a team as code owners September 9, 2022 19:08

product-auto-label bot added size: m Pull request size is medium. api: bigtable Issues related to the googleapis/nodejs-bigtable API. labels Sep 9, 2022

First correction

448e41d

kolea2 requested changes Sep 15, 2022

View reviewed changes

system-test/bigtable.ts Outdated Show resolved Hide resolved

test/backup.ts Outdated Show resolved Hide resolved

danieljbruce added 4 commits September 15, 2022 16:28

Check the list of backups

7407fa2

fetched backup

4eba7d3

get name and id

9c21e29

Remove only

c87a4d9

danieljbruce changed the title ~~Copy backup 2 relevant changes only~~ fix: Add feature for copying backups Sep 21, 2022

danieljbruce added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 21, 2022

danieljbruce added 2 commits September 28, 2022 11:55

refactor request mock

a5c1ce2

Add unit test for copying backup to another projec

3ee1ec3

danieljbruce requested a review from bhshkh March 25, 2024 15:45

Merge branch 'main' into copy-backup-2-relevant-changes-only

9253c9c

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 12, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 12, 2024

🦉 Updates from OwlBot post-processor

4e49ab7

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

kolea2 reviewed Apr 30, 2024

View reviewed changes

danieljbruce added 3 commits May 6, 2024 10:06

Rename bigtable to bigtableSecondaryProject

ff3e603

Merge branch 'copy-backup-2-relevant-changes-only' of https://github.…

fcae645

…com/danieljbruce/nodejs-bigtable into copy-backup-2-relevant-changes-only

PR follow-ups - inline code and rename variables

307d110

danieljbruce requested a review from kolea2 May 6, 2024 14:28

Merge branch 'main' into copy-backup-2-relevant-changes-only

69318a0

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label May 10, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 10, 2024

kolea2 approved these changes May 10, 2024

View reviewed changes

leahecole reviewed May 13, 2024

View reviewed changes

danieljbruce added 5 commits May 13, 2024 15:25

Remove unused import

1b334a1

Add comments for expire time

781516f

Add better garbage cleanups of the backups

9a48759

These comments are not needed anymore.

71c5bef

Remove only

2089c64

danieljbruce requested a review from leahecole May 13, 2024 21:38

leahecole approved these changes May 21, 2024

View reviewed changes

danieljbruce and others added 2 commits May 21, 2024 15:28

Update system-test/bigtable.ts

4bc7a7d

Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>

Update system-test/bigtable.ts

661c927

Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label May 21, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 21, 2024

danieljbruce added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 22, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 22, 2024

danieljbruce merged commit 91f85b5 into googleapis:main May 22, 2024
15 of 19 checks passed

release-please bot mentioned this pull request May 8, 2024

chore(main): release 5.1.0 #1321

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add feature for copying backups #1153

feat: Add feature for copying backups #1153

danieljbruce commented Sep 9, 2022 •

edited

kolea2 Apr 30, 2024

kolea2 Apr 30, 2024

danieljbruce May 6, 2024

kolea2 Apr 30, 2024

danieljbruce May 6, 2024

kolea2 Apr 30, 2024

danieljbruce May 6, 2024

leahecole May 13, 2024

danieljbruce May 13, 2024

leahecole May 14, 2024

danieljbruce May 21, 2024

leahecole May 21, 2024

danieljbruce May 21, 2024

leahecole left a comment

leahecole May 21, 2024

feat: Add feature for copying backups #1153

feat: Add feature for copying backups #1153

Conversation

danieljbruce commented Sep 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leahecole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieljbruce commented Sep 9, 2022 •

edited