Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 136 commits
46ce5b8
576550d
31a1d9a
90e290e
7bce08d
29a736f
edc30d3
c8a16fc
2d5320c
adabd71
89f742e
d4f5d47
ea43929
bb6e708
c96442a
701badc
448e41d
7407fa2
4eba7d3
9c21e29
c87a4d9
a5c1ce2
3ee1ec3
1d80cf4
081ddac
2315c78
0c07e7c
2330a56
aa5bab3
967bd71
0075152
c97b89f
fbdf7d4
8c685fd
38d7961
e4d2229
77d105f
a69c8c6
d3387d3
452eef0
d7c4e94
c142deb
fbe0b0c
11148fb
e055215
463666a
3318e84
c932b4c
db741f2
0b91bb6
307a8cb
ee33f43
a3a4d9c
e554f92
3a1cd27
3f594b2
4ffb7a1
8073653
e1f2dde
f66ee04
13e128d
b1af9c1
4c0310c
d61e1cc
e2a8be5
763115b
1827059
3259115
4534399
0836841
49f693f
67d5eb0
2ef42e6
86a0311
f163aba
a27f29f
80f0154
3b95726
ae0e313
df282b4
99e3d8c
3dddc0b
5a0d07b
e4d38d4
6a26bde
e7e1eb3
d859a33
b2c728f
377d5bc
1520f22
74cbffa
26289af
c5ba924
ffb9588
1fe2ef0
9c79b43
7ab1c96
842b70b
3872ff0
02078e5
5ac03be
4bd4777
7ee2f18
1a0e153
e3fdbd2
b232315
9890fd8
56c7b7f
83b7e7e
af77ce8
b6fa45a
a3c83bd
d63f892
d2de338
9e8731d
2695d96
cc8ce59
45ef624
835c827
2518d3d
40f3cc2
dc149e6
165ce36
0720df1
22e9bec
dfc03bf
8cba026
d0a0e1f
3b16183
6a46c8c
9253c9c
4e49ab7
ff3e603
fcae645
307d110
69318a0
1b334a1
781516f
9a48759
71c5bef
2089c64
4bc7a7d
661c927
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 15 in src/cluster.ts
GitHub Actions / lint
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
and608 * 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.
Delete the instance here after testing?
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 instance is already deleted in
nodejs-bigtable/system-test/bigtable.ts
Lines 98 to 115 in d30e513