-
Notifications
You must be signed in to change notification settings - Fork 378
sample: add turbo replication samples #1618
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
sample: add turbo replication samples #1618
Conversation
Warning: This pull request is touching the following templated files:
|
Here is the summary of changes. You are about to add 4 region tags.
This comment is generated by snippet-bot.
|
4d53fb4
to
a4ebfdc
Compare
Does it make sense to also add this value to the getMetadata sample? |
); | ||
} | ||
|
||
createBucketWithTurboReplication().catch(console.error); |
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.
Instead of catching like this, please remove the .catch
, and add this at the end:
process.on('unhandledRejection', err => {
console.error(err.message);
process.exitCode = 1;
});
main(...process.argv.slice(2));
This follows the sample standards we've laid out elsewhere.
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 @JustinBeckwith I'll make a ticket to update the other tests to this format as most in this repo are doing .catch(console.error)
// https://cloud.google.com/storage/docs/storage-classes | ||
const [bucket] = await storage.createBucket(bucketName, { | ||
location, | ||
rpo, |
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 sample is called "create bucket with turbo replication", however it takes in rpo as a parameter, meaning theoretically you could set the rpo to whatever you want, for example DEFAULT, and then the sample wouldn't do what it's saying.
To make this clearer, wouldn't it be better to create a local variable named rpo and set it to ASYNC_TURBO, as part of the sample rather than as an argument? I know that you do that as a commented out line, but it might be clearer what this sample is communicating if it's hard-coded 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.
Good call. I will change this.
|
||
// Enable turbo replication for the bucket by setting rpo to ASYNC_TURBO. | ||
// The bucket must be a dual-region bucket. | ||
async function setRPOAsyncTurbo() { |
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 "enableTurboReplication()" be a better name?
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 originally had a name somewhat similar to that but TW thought this was a better name (see comments in the canonical sample document).
|
||
// Disable turbo replication for the bucket by setting RPO to default. | ||
// The bucket must be a dual-region bucket. | ||
async function setRPODefault() { |
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 "disableTurboReplication()" be a better name?
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.
Same comment as above.
30d57af
to
70caa80
Compare
a17f006
to
52ce8f6
Compare
e50a1ed
to
5120d52
Compare
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #1649 🦕