Introduce Backups #794

stephenplusplus · 2020-09-02T17:13:36Z

API Changes

Access or create Backup objects through a Cluster reference

cluster.backup(id) // references a pre-existing "Backup" object (no remote calls)
cluster.createBackup({ id: 'backup-id', table: 'table-id' }) // creates and returns a "Backup" object
cluster.getBackups() // returns "Backup" objects
cluster.getBackupsStream() // returns "Backup" objects as a readable stream

Access Backup objects through an Instance reference

instance.createTableFromBackup({ table: 'table-id', backup: 'backup-id' }, gaxOptions)
instance.getBackups() // returns "Backup" objects
instance.getBackupsStream() // returns "Backup" objects as a readable stream

Create a Backup through a Table reference

table.createBackup(id, metadata) // creates and returns a "Backup" object

Once you have a reference to a Backup object:

All methods below besides "restore()" are common CRUD methods defined throughout our API

backup.create({ table: 'table-id', metadata: {...} })
backup.delete()
backup.get()
backup.getMetadata()
backup.restore('table-id')
backup.setMetadata({...})

References

To Dos

This PR splits the work from #761 into three parts-- source code, tests, and then documentation and samples to come in a follow-up PR. This should make reviewing easier, as well as lets us lock in the source code changes before attempting to adapt the previously written tests and samples.

Currently, this PR has only the source code. Most of it was carried from #761, but there were some structural changes to follow our conventions:

- Backup(id, cluster, fields)
+ Backup(cluster, id)

get backup.expireDate()
get backup.startDate()
get backup.endDate()

- backup.create(table, metadata)
+ backup.create({table, metadata})
backup.delete()
backup.get()
+ backup.getMetadata()
backup.restore(tableId, gaxOptions)
- backup.update(metadata, gaxOptions)
+ backup.setMetadata(metadata, gaxOptions)

+ cluster.backup(id)
+ cluster.createBackup({id, table}, gaxOptions)
cluster.getBackups(options)
cluster.getBackupsStream(options)

- instance.backup(id, cluster, fields)
- instance.createTableFromBackup(tableId, backup, gaxOptions)
+ instance.createTableFromBackup({table, backup}, gaxOptions)
+ instance.getBackups(options, callback)
+ instance.getBackupsStream(options, callback)

- table.backup(id, metadata, gaxOptions)
+ table.createBackup(id, metadata, gaxOptions)

@AVaksman could you please review this when you get a chance? I will likely need special attention to the TypeScript, making sure I'm properly compliant with our conventions and use cases I may have missed. Thank you!

src/backup.ts

src/cluster.ts

src/instance.ts

src/cluster.ts

stephenplusplus · 2020-09-03T15:17:01Z

@AVaksman thank you very much for the thorough review! I've made the changes (thank you for all of the suggestions) and left just one thing: #794 (comment).

package.json

JustinBeckwith · 2020-09-03T17:58:52Z

src/cluster.ts

+        gaxOpts: options.gaxOptions,
+      },
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      (err, ...resp: any[]) => {


The proper type for this is likely represented in the generated types from protobufjs - would be nice to use those please :)

@AVaksman could you advise here? This was taken from your suggestion on the other PR. I'm fighting a lot of TypeScript errors when I tamper with this.

I took a stab at it as a part of an existing PR #731 where I am differentiating bigtable.request signature between NormalCallback, LROCallback and PaginatedCallback

export type RequestCallback<T = void, R = void> = T extends void ? LROCallback : R extends void ? NormalCallback<T> : PagedCallback<T, R>; export interface NormalCallback<TResponse> { (err: ServiceError | null, res?: TResponse): void; } export interface PagedCallback<Item, Response> { ( err: ServiceError | null, results?: Item[] | null, nextQuery?: {} | null, response?: Response | null ): void; } export interface LROCallback { ( err: ServiceError | null, operation?: Operation, apiResponse?: IOperation ): void; }

Then here the something like this would work

this.bigtable.request< google.bigtable.admin.v2.IBackup, google.bigtable.admin.v2.IListBackupsResponse >( { client: 'BigtableInstanceAdminClient', method: 'listAppProfiles', reqOpts, gaxOpts, }, (err, IBackups, nextPageRequest, apiResponse) => { if (err) { callback(err); return; } const backups = IBackups!.map(backup => { const backupInstance = this.backup(backup.name!.split('/').pop()!); backupInstance.metadata = backup; return backupInstance; }); const nextQuery = nextPageRequest! ? extend({}, options, nextPageRequest!) : null; callback(null, appProfiles, nextQuery, apiResponse!); } );

Is this an overkill?
bigtable.request signature changes could be done in a separate PR in a non breaking way.
WDYT?

If agree, I can do the bigtable.request PR?

Curious for @JustinBeckwith's thoughts. Whatever makes sense to you guys :)

For now, should I leave the code as-is? Also, please LMK if everything else looks correct so far and I'll get the tests going.

I am ok with leaving this as is for now, and coming back around to improve the under types here later. I'm ok with it because I don't think they affect the public types. Not a big deal.

src/table.ts

google-cla · 2020-09-03T19:07:30Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-09-03T19:07:33Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-09-03T19:07:36Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-09-03T19:07:40Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-09-03T19:07:43Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-09-03T19:07:50Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-09-03T19:07:56Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-09-03T19:07:59Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-09-03T19:08:32Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

stephenplusplus · 2020-09-03T19:08:48Z

Looks like you can't delete old @google-cla posts-- it'll just post it again 🤣

AVaksman · 2020-09-03T19:41:11Z

I've seen commenting "@googlebot I consent." have helped, @stephenplusplus

google-cla · 2020-09-03T19:41:34Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-09-03T19:41:56Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

This reverts commit 4c1b46b.

google-cla · 2020-09-09T17:30:05Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-09-09T17:50:08Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

JustinBeckwith · 2020-09-09T17:55:30Z

src/backup.ts

+  /**
+   * An expression for specifying the sort order of the results of the request.
+   * The string value should specify one or more fields in
+   * {@link google.bigtable.admin.v2.Backup|Backup}. The full syntax is


Curiosity - have you generated the docs locally to make sure these links get rendered right?

Hmm, well I don't actually know where that gets rendered. That's part of interface GetBackupsOptions, and I can't find where it went. It's not linked to where we use it from the getBackups() methods :. All of the other links from descriptions seem to work, though!

JustinBeckwith · 2020-09-09T18:02:41Z

src/cluster.ts

+      },
+    };
+
+    if (reqOpts.backup.expireTime instanceof Date) {


There used to be all kinds of nuance on when it was ok to use instanceof Date, and you had to do weird tricks to detect date objects. It looks like is now does exactly what you're doing here though:
https://github.com/sindresorhus/is/blob/master/source/index.ts

So I'm good with it :)

JustinBeckwith · 2020-09-09T18:03:48Z

src/cluster.ts

+        gaxOpts: options.gaxOptions,
+      },
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      (err, ...resp: any[]) => {


I am ok with leaving this as is for now, and coming back around to improve the under types here later. I'm ok with it because I don't think they affect the public types. Not a big deal.

JustinBeckwith · 2020-09-09T18:04:35Z

src/cluster.ts

+    const callback =
+      typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!;
+
+    options = extend(true, {}, options);


I spent a year trying to remove extend in favor of Object.assign, and I just want to say out loud that @stephenplusplus was right all along and I was wrong.

Hahaha, stdlib got so close on this one!

stephenplusplus added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 2, 2020

stephenplusplus requested a review from AVaksman September 2, 2020 17:13

stephenplusplus requested a review from a team as a code owner September 2, 2020 17:13

google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Sep 2, 2020

AVaksman suggested changes Sep 3, 2020

View reviewed changes

product-auto-label bot added the api: bigtable Issues related to the googleapis/nodejs-bigtable API. label Sep 3, 2020

JustinBeckwith reviewed Sep 3, 2020

View reviewed changes

googleapis deleted a comment from google-cla bot Sep 3, 2020

stephenplusplus added 21 commits September 9, 2020 13:28

switch logic between get/getMetadata to match other files

aa834b4

pr feedback!

4a9bdf7

correct JSdocs and simplify setMetadata

587c6c3

rename types

5d15c5f

update promisify to support accessor methods

012ce04

test: intance.ts

974a02e

test: index.ts

25b8c30

alphabetize

26ad89e

test: cluster.ts

370128a

Revert "alphabetize"

a24e6f6

This reverts commit 4c1b46b.

test: table.ts

ee39d98

test: backup.ts

fcc1355

remove as anys

ed10b3c

pr review changes

8994430

strict equal check

405e4e1

add test

02d4937

pop the name

8b3ecc6

add system tests & correct restore API call

2408f6b

add instance#getBackups and instance#getBackupsStream

549b575

address pr feedback

9f0145c

pr comments

c96aad1

stephenplusplus force-pushed the spp--backups branch from 532920a to c96aad1 Compare September 9, 2020 17:29

address test coverage

880f3d6

JustinBeckwith added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Sep 9, 2020

JustinBeckwith approved these changes Sep 9, 2020

View reviewed changes

kolea2 approved these changes Sep 10, 2020

View reviewed changes

stephenplusplus merged commit 7979e39 into googleapis:master Sep 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Backups #794

Introduce Backups #794

stephenplusplus commented Sep 2, 2020 •

edited

Loading

stephenplusplus commented Sep 3, 2020

JustinBeckwith Sep 3, 2020

stephenplusplus Sep 3, 2020

AVaksman Sep 3, 2020

AVaksman Sep 3, 2020

stephenplusplus Sep 3, 2020

JustinBeckwith Sep 9, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

stephenplusplus commented Sep 3, 2020

AVaksman commented Sep 3, 2020 •

edited

Loading

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 9, 2020

google-cla bot commented Sep 9, 2020

JustinBeckwith Sep 9, 2020

stephenplusplus Sep 9, 2020

JustinBeckwith Sep 9, 2020

JustinBeckwith Sep 9, 2020

JustinBeckwith Sep 9, 2020

stephenplusplus Sep 9, 2020

Introduce Backups #794

Introduce Backups #794

Conversation

stephenplusplus commented Sep 2, 2020 • edited Loading

API Changes

Access or create Backup objects through a Cluster reference

Access Backup objects through an Instance reference

Create a Backup through a Table reference

Once you have a reference to a Backup object:

References

To Dos

stephenplusplus commented Sep 3, 2020

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

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

stephenplusplus commented Sep 3, 2020

AVaksman commented Sep 3, 2020 • edited Loading

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 3, 2020

google-cla bot commented Sep 9, 2020

google-cla bot commented Sep 9, 2020

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

stephenplusplus commented Sep 2, 2020 •

edited

Loading

AVaksman commented Sep 3, 2020 •

edited

Loading