feature: idiomatic table backup management implementation by zamnuts · Pull Request #761 · googleapis/nodejs-bigtable

zamnuts · 2020-07-13T11:43:12Z

This is the hand-written implementation that introduces backup support for tables.

A backup represents a table on a specific cluster
Backups reside local to a cluster
A backup is restored as a table to the cluster from where it originated
proto methods have been mirrored to the relevant classes based on parent resource requirements:
- createBackup, getBackup, listBackups, updateBackup, and deleteBackup are in Cluster
- restoreTable is in Instance
Friendlier methods that are more intuitive wrap the aforementioned proto proxies requiring less input criteria:
- table.backup() to create a backup
- Introduced a Backup class with delete, get, restore, and update methods
- Dates are JS-friendly and params/responses convert to/from Timestamp Struct, Date, and PreciseDate
Some methods return a Backup or Backup[] where applicable, but Longrunning Operations (LROs) do not
- LROs do not transform their result into a Backup, this is left to the user
- LROs return POJOs instead
- Added cluster.asBackup casting helper to convert POJOs into Backup
Metadata results were sparse for the response structs for backups, the "backup" is the metadata in most cases
Aside: wrote tests using async/await
- promisify is refactored as pass through to allow for promises
- Changed this in unrelated test suites and fixed signature tests where they were incorrect
When testing, instances cannot be deleted if they have backups defined
- Remove all backups before removing the instances
- A fully parallel backup reaper was introduced to help, it runs before the instance reaper

PR checklist:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes ? 🦕

zamnuts · 2020-07-13T11:55:57Z

System and samples tests are both currently failing due to:

Error: 12 UNIMPLEMENTED: Method not found.

All necessary methods are present (in master) within v2.BigtableTableAdminClient, and these work locally against my test cluster. I expect this is an issue with the RPC version available to kokoro?

AVaksman · 2020-07-13T22:15:26Z

It seems the Backup class structure here is a diversion from the pattern followed by other classes (Instance, Cluster, Table, AppProfile) in the library, where the instance of a class represents an object that can interact with a Cloud Bigtable object.

Factory reference usually would take a pathway to service (like instance object, instance.bigtable.request) and an id, from here an object can interact with Cloud Bigtable.

const backup = instance.backup('my-backup', 'my-cluster');
backup.getMetadata();
backup.update(...);

AVaksman · 2020-07-14T04:01:11Z

src/backup.ts

+   * @param {Bigtable} bigtable
+   * @param {google.bigtable.admin.v2.IBackup} backup A Backup or
+   */
+  constructor(bigtable: Bigtable, backup: google.bigtable.admin.v2.IBackup) {


Looking at the pattern of other classes in the library, the constructor should take an instance of its parent class (seems Cluster in this case) and a string id.

User may pass just an 'my-backup' as an id or a full name as 'projects/{my-project}/instances/{my-instance}/clusters/{my-cluster}/backups/{my-backup}'. Then the pattern is to

let name; if (id.includes('/')) { if (id.startsWith(`${cluster.name}/backups/`)) { name = id; } else { throw new Error(`Backup id '${id}' is not formatted correctly. Please use the format 'my-backup' or '${cluster.name}/backups/my-backup'.`); } } else { name = `${cluster.name}/backups/${id}`; } this.id = name.split('/').pop()!; this.name = name;

AVaksman · 2020-07-14T04:02:59Z

src/backup.ts

+  constructor(bigtable: Bigtable, backup: google.bigtable.admin.v2.IBackup) {
+    Object.assign(this, backup);
+
+    this.metadata = backup.valueOf();


As per the pattern this.metadata usually gets set/updated during getMetadata call.

AVaksman · 2020-07-14T04:10:54Z

src/backup.ts

+    Object.assign(this, backup);
+
+    this.metadata = backup.valueOf();
+    this._bigtable = bigtable;


would be cluster.bigtable if following suggestion above.

also this.cluster = cluster is useful to delegate backup.create to.

AVaksman · 2020-07-14T04:12:43Z

src/backup.ts

+  sourceTable?: string | null;
+  expireTime?: google.protobuf.ITimestamp | null;
+  startTime?: google.protobuf.ITimestamp | null;
+  endTime?: google.protobuf.ITimestamp | null;
+  sizeBytes?: number | Long | string | null;
+  state?:


If follow the pattern of other classes in the library, these values are located in this.metadata?

@AVaksman I don't quite follow this comment. Could you explain a bit? Or maybe this comment is showing in the wrong place?

What I meant to say it usually all proto properties of a resource are located in the metadata property of the resource object.
Instance.metadata has type, labels, displayName, state
Cluster.metadata has defaultStorageType, serveNodes, location, state
Table.metadata has clusterStates, columnFamilies, granularity
Class objects usually have properties id, name, metadata, bigtable, instance(if it is not an instance of class Instance).

AVaksman · 2020-07-14T04:21:03Z

src/backup.ts

+   * @readonly
+   * @return {string}
+   */
+  get projectId(): string {


projectId might not be available at the time of instantiation of the object and usually is updated during the first network call in prepareGaxRequest via getProjectId_, and subsequently updated across reqOpts.

AVaksman · 2020-07-14T05:39:31Z

src/backup.ts

+   */
+  update(
+    fields: ModifiableBackupFields,
+    options?: UpdateBackupOptions


Could probably use top level gaxOptions?: CallOptions as in other setMetadata methods

AVaksman · 2020-07-14T05:39:49Z

src/backup.ts

+  update(
+    fields: ModifiableBackupFields,
+    options?: UpdateBackupOptions
+  ): Promise<UpdateBackupResponse> {


Would need overloads here as well

AVaksman · 2020-07-14T05:51:16Z

src/backup.ts

+    return this._bigtable
+      .instance(this.instanceId)
+      .cluster(this.clusterId)
+      .updateBackup(this.backupId, fields, options);
+  }


Usually setMetadata functions belong to the object itself.

AVaksman · 2020-07-14T05:54:23Z

src/backup.ts

+
+    return this._bigtable
+      .instance(this.instanceId)
+      .restoreTable(this.backupId, this.clusterId, tableId, options);


Please refer to comment for instance.restoreTable

AVaksman · 2020-07-14T06:03:08Z

src/backup.ts

+   * A plain-old-javascript-object representation of this backup.
+   * @return {google.bigtable.admin.v2.IBackup}
+   */
+  valueOf(): google.bigtable.admin.v2.IBackup {


I do not think we utilize this method in other classes. Usually this value would be assigned to this.metadata

AVaksman · 2020-07-14T16:04:53Z

src/cluster.ts

+export interface CreateBackupOptions {
+  gaxOptions?: CallOptions;
+}


Perhaps we should package all but id params in one object

export interface CreateBackupOptions { tableId: string; // tableId might be enough since we can build full table path expireTime: BackupTimestamp; gaxOptions?: CallOptions; }

Similar to creteInstance, createCluster, createTable functions.

AVaksman · 2020-07-14T16:08:33Z

src/cluster.ts

+   * minimum 6 hours from the time of the backup request and a maximum of 30
+   * days.
+   */
+  expireTime?: BackupTimestamp;


expireTime could be required here since it is required for creation of a backup, and is the only field that can be updated on an existing backup.

AVaksman · 2020-07-14T16:20:36Z

src/instance.ts

+export type RestoreTableCallback = (
+  err: ServiceError | null,
+  apiResponse?: LROperation<
+    google.bigtable.admin.v2.ITable,
+    google.bigtable.admin.v2.IRestoreTableMetadata
+  >
+) => void;


Long running operation functions that return a resource object usually have the return signatures as in this example

nodejs-bigtable/src/instance.ts

Lines 102 to 111 in 5e47bbc

export interface LongRunningResourceCallback<Resource> {

(

err: ServiceError | null,

resource?: Resource,

operation?: Operation,

apiResponse?: IOperation

): void;

}

export type CreateInstanceCallback = LongRunningResourceCallback<Instance>;

export type CreateInstanceResponse = [Instance, Operation, IOperation];

AVaksman · 2020-07-14T16:26:03Z

src/cluster.ts

+export type CreateBackupCallback = (
+  err: ServiceError | null,
+  apiResponse?: LROperation<
+    google.bigtable.admin.v2.IBackup,
+    google.bigtable.admin.v2.ICreateBackupMetadata
+  >
+) => void;
+export type CreateBackupResponse = [
+  LROperation<
+    google.bigtable.admin.v2.IBackup,
+    google.bigtable.admin.v2.ICreateBackupMetadata
+  >
+];


Similar LRO function return signatures as here

AVaksman · 2020-07-14T16:32:41Z

src/cluster.ts

+   *   described at https://aip.dev/132#ordering.
+   */
+  orderBy?: string;
+


should have autoPaginate, pageSize and pageToken as well.

AVaksman · 2020-07-14T17:06:30Z

src/cluster.ts

+export type ListBackupsResponse = [Backup[]];
+export type ListBackupsCallback = (
+  err: ServiceError | null,
+  metadata?: Backup[]
+) => void;


Paged functions callbacks/responses usually are of the form

export interface PagedCallback { ( err: ServiceError | null, results?: Backup[] | null, nextQuery?: GetBackupsOptions | null, response?: Response | null ): void; }

AVaksman · 2020-07-14T17:10:01Z

src/cluster.ts

+   *   `name` should be supplied to be useful in most cases.
+   * @return {Backup}
+   */
+  asBackup(backup: google.bigtable.admin.v2.IBackup): Backup {


Should we introduce a factory backup function similar to Instance.table or Insance.cluster?

AVaksman · 2020-07-14T17:25:15Z

src/cluster.ts

+    table: Table | string,
+    id: string,
+    fields: Required<ModifiableBackupFields>,
+    optionsOrCallback?: CreateBackupOptions | CreateBackupCallback,


Commented above about possibly combining params.

AVaksman · 2020-07-14T19:13:48Z

src/cluster.ts

+    );
+  }
+
+  listBackups(options?: ListBackupsOptions): Promise<ListBackupsResponse>;


Probably should name getBackups as other similar paged functions getTables, getClusters ...

are the other underlying rpcs similarly listTable and not getTables? Either way this may make sense for interface consistency.

Yes, API has getInstances, getClusters, getTables, getAppProfiles, ... that wrap underlying list RPCs.

AVaksman · 2020-07-14T19:20:56Z

src/cluster.ts

+   * @example <caption>include:samples/document-snippets/cluster.js</caption>
+   * region_tag:bigtable_cluster_list_backups
+   */
+  listBackups(


Suggesting to implement getBackups in the Instance class to get all backups across all clusters on the instance by specifying reqOpts.parent as projects/{project}/instances/{instance}/clusters/- and have cluster.getBackups delegate to instance.getBackupswith specific cluster name as reqOpts.parent

AVaksman · 2020-07-14T19:41:42Z

src/cluster.ts

+    id: string,
+    fields: ModifiableBackupFields,
+    optionsOrCallback?: UpdateBackupOptions | UpdateBackupCallback,
+    cb?: UpdateBackupCallback


Pattern-wise it seems we do not have similar methods in other classes, just object.setMetadata

Otherwise would propose to bundle up the params in one options object.

AVaksman · 2020-07-14T19:45:15Z

src/cluster.ts

+ *
+ * These methods can be auto-paginated.
+ */
+paginator.extend(Cluster, ['listBackups']);


I believe gapic supports auto-pagination now. I do not think this is needed?

I went through pagination thoroughly and recall finding that gapic was incompatible with what was expected from the backups RPC, at least per the generated types and protobufs. The implementation as it stands using the paginator library works as expected.

AVaksman · 2020-07-14T20:26:22Z

src/instance.ts

+        reqOpts,
+        gaxOpts: options.gaxOptions,
+      },
+      callback // TODO cast as Table


should splice in a table Object in the callback.

crwilcox · 2020-07-16T21:02:23Z

src/backup.ts

+
+  /**
+   * @param {Bigtable} bigtable
+   * @param {google.bigtable.admin.v2.IBackup} backup A Backup or


Seems this comment is incomplete?

crwilcox · 2020-07-16T23:15:41Z

samples/backups.js

+
+  // Alternatively, if a backup object is available, use the `delete` helper.
+  const backupPartial = cluster.asBackup({
+    name: backupFromTable.name, // used from another example


I don't understand this comment?

crwilcox · 2020-07-16T23:17:10Z

samples/document-snippets/cluster.js

+    cluster
+      .deleteBackup(backupId, {
+        gaxOptions: {
+          timeout: 50 * 1000, // this op takes a good 30-40 seconds


nit: time cannot itself be good or bad ;) also, probably shouldn't abbreviate op

crwilcox · 2020-07-16T23:18:29Z

src/backup.ts

+  sourceTable?: string | null;
+  expireTime?: google.protobuf.ITimestamp | null;
+  startTime?: google.protobuf.ITimestamp | null;
+  endTime?: google.protobuf.ITimestamp | null;
+  sizeBytes?: number | Long | string | null;
+  state?:


@AVaksman I don't quite follow this comment. Could you explain a bit? Or maybe this comment is showing in the wrong place?

crwilcox · 2020-07-16T23:21:40Z

src/cluster.ts

+  /**
+   * The ITimestamp (Date or PreciseDate will be converted) representing
+   * when the backup will automatically be deleted. This must be at a
+   * minimum 6 hours from the time of the backup request and a maximum of 30


this min/max is mentioned but I didn't see anything validating this.

from what i found, this min/max was not documented. the grpc response will respond with an error outside of these bounds. i opted to not perform client-side validation in case this changes in the future.

crwilcox · 2020-07-16T23:22:56Z

src/cluster.ts

+    );
+  }
+
+  listBackups(options?: ListBackupsOptions): Promise<ListBackupsResponse>;


are the other underlying rpcs similarly listTable and not getTables? Either way this may make sense for interface consistency.

…lete timeouts, expireTime is required

…he cluster

…expire to field encapsulation for flexibility

…precise-date for backup timestamps

…e dupe proto import, import reorder

…for backup timestamps, show Backup usage in sample

…ck bug

…nsider promise wrapping

…le name instead of reference, orig backup as public metadata

…sting helper and update sample accordingly

codecov · 2020-07-28T12:17:38Z

Codecov Report

Merging #761 into master will increase coverage by 0.04%.
The diff coverage is 99.81%.

@@            Coverage Diff             @@
##           master     #761      +/-   ##
==========================================
+ Coverage   99.14%   99.18%   +0.04%     
==========================================
  Files          17       18       +1     
  Lines       15880    16938    +1058     
  Branches      955     1039      +84     
==========================================
+ Hits        15744    16800    +1056     
- Misses        133      135       +2     
  Partials        3        3

Impacted Files	Coverage Δ
src/cluster.ts	`99.79% <99.63%> (-0.21%)`	⬇️
src/backup.ts	`100.00% <100.00%> (ø)`
src/index.ts	`100.00% <100.00%> (ø)`
src/instance.ts	`100.00% <100.00%> (ø)`
src/table.ts	`99.64% <100.00%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c694ae5...1be71e2. Read the comment docs.

JustinBeckwith · 2020-07-28T21:33:48Z

package.json

    "api-documenter": "api-documenter yaml --input-folder=temp"
  },
  "dependencies": {
+    "@google-cloud/paginator": "^3.0.0",


@google-cloud/paginator was explicitly removed here:
8a2a74e

@AVaksman want to work with @zamnuts on what we're supposed to be doing here now?

@google-cloud/paginator was explicitly removed here:
8a2a74e

@AVaksman want to work with @zamnuts on what we're supposed to be doing here now?

I noticed that commit. I've successfully refactored the stream portion, but am still having trouble understanding the pagination.

Sure,

add gapic method name (listBackupsStream: true) to gapicStreamingMethods

pipe, rather pumpify received objects through a Transform stream that will convert proto objects into this library Backup objects. Similar reference below.

nodejs-bigtable/src/instance.ts

Lines 727 to 744 in 2917d17

const self = this;

const transformToAppProfile = (

chunk: google.bigtable.admin.v2.IAppProfile,

enc: string,

callback: Function

) => {

const appProfile = self.appProfile(chunk.name!.split('/').pop()!);

appProfile.metadata = chunk;

callback(null, appProfile);

};

return pumpify.obj([

this.bigtable.request({

client: 'BigtableInstanceAdminClient',

method: 'listAppProfilesStream',

reqOpts,

gaxOpts: gaxOptions,

}),

new Transform({objectMode: true, transform: transformToAppProfile}),

JustinBeckwith · 2020-07-28T21:34:23Z

samples/backups.js

+const uuid = require('uuid');
+const {inspect} = require('util');
+
+function generateBackupId() {


These almost always belong in the samples test, and are passed in as a parameter to the main function (will keep reading though)

JustinBeckwith · 2020-07-28T21:35:25Z

samples/backups.js

+  return `gcloud-tests-${uuid.v4()}`.substr(0, 48);
+}
+
+async function runBackupOperations(


It's important that we have an outer main function, with an inner async function due to the way these get embedded in cloudsite. Please follow the pattern here:
https://github.com/googleapis/nodejs-bigtable/blob/master/samples/quickstart.js

JustinBeckwith · 2020-07-28T21:36:06Z

samples/backups.js

+  });
+  console.log(
+    'Started a table backup operation:',
+    inspect(backupOperation.latestResponse.name, {depth: null, colors: true})


Typically we don't drop down to util.inspect, but just drop individual properties in the logs. Since this is a name, is it just a string type?

JustinBeckwith · 2020-07-28T21:36:36Z

samples/backups.js

+  // [END bigtable_delete_backup]
+}
+
+require('yargs')


In our modern samples we do not use yargs, and instead use the model found in https://github.com/googleapis/nodejs-bigtable/blob/master/samples/quickstart.js

JustinBeckwith · 2020-07-28T21:37:27Z

samples/document-snippets/cluster.js

    // [END bigtable_cluster_set_meta]
  },
+
+  createBackup: (instanceId, clusterId, tableId, backupId) => {


These snippets are tech debt :/ I believe we don't have tests for many of them. For any new sample code, please create a new file, and follow https://github.com/googleapis/nodejs-bigtable/blob/master/samples/quickstart.js as a guide. One per file!

Thanks for reviewing the samples. There were competing patterns, and I wasn't sure what to implement. I'll revise based on your feedback.

JustinBeckwith · 2020-07-28T21:39:34Z

samples/test/backups.test.js

+
+describe('backups', () => {
+  before(async () => {
+    await instance.create({


I seem to remember there being a fairly low cap on the number of clusters you can have at a time. I created samples/test/util.js to help do a check-in/out re-use thing to keep the number of clusters we create down if possible.

JustinBeckwith · 2020-07-28T21:40:21Z

samples/test/cluster.js

  });

+  it('should create a backup of a table', () => {
+    clusterSnippets.createBackup(INSTANCE_ID, CLUSTER_ID, TABLE_ID, BACKUP_ID);


Are these all async functions? Do we need any assertions to make sure they ran successfully? Is there a chance these tests are all skipped in the describe block?

AVaksman · 2020-07-30T03:54:42Z

src/backup.ts

+   * @example <caption>include:samples/document-snippets/cluster.js</caption>
+   * region_tag:bigtable_cluster_delete_backup
+   */
+  delete(options?: DeleteBackupOptions): Promise<DeleteBackupResponse> {


You should keep the overloads as is

delete(gaxOptions?: CallOptions): Promise<DeleteBackupResponse>; delete(callback: DeleteBackupCallback): void; delete(gaxOptions: CallOptions, callback: DeleteBackupCallback): void;

and use async for the function implementation

async delete( optionsOrCallback?: CallOptions | DeleteBackupCallback ): void | Promise<DeleteIBackupResponse> {

Provide callback support via callbackifyAll

kolea2 · 2020-07-30T15:56:17Z

src/instance.ts

+export interface RestoreTableOptions {
+  gaxOptions?: CallOptions;
+}
+export type RestoreTableCallback = (


should these types be on table rather than instance?

… to interact with backups

kolea2 · 2020-08-18T17:17:21Z

@zamnuts can you take a look at the build failures? Thanks!

zamnuts · 2020-08-18T21:05:58Z

@zamnuts can you take a look at the build failures? Thanks!

Build failures are due to automated tests not being update to reflect the refactored implementation.

AVaksman · 2020-08-19T14:19:24Z

src/backup.ts

+export type DeleteBackupCallback = GenericBackupCallback<IEmpty>;
+export type DeleteBackupResponse = [Backup, IEmpty];


DeleteCallback/Response should just return IEmpty. Reference to same in Instance, Table

AVaksman · 2020-08-19T14:52:00Z

src/backup.ts

+export type CreateBackupCallback = (
+  err: ServiceError | null,
+  apiResponse?: LROperation<
+    google.bigtable.admin.v2.IBackup,
+    google.bigtable.admin.v2.ICreateBackupMetadata
+  >
+) => void;
+export type CreateBackupResponse = [
+  LROperation<
+    google.bigtable.admin.v2.IBackup,
+    google.bigtable.admin.v2.ICreateBackupMetadata
+  >
+];


Create backup callback/response should follow the following pattern

nodejs-bigtable/src/instance.ts

Lines 102 to 111 in 5e47bbc

export interface LongRunningResourceCallback<Resource> {

(

err: ServiceError | null,

resource?: Resource,

operation?: Operation,

apiResponse?: IOperation

): void;

}

export type CreateInstanceCallback = LongRunningResourceCallback<Instance>;

export type CreateInstanceResponse = [Instance, Operation, IOperation];

where an Operation is imported from google-gax and IOperation is google.longrunning.IOperation

Use case

const [backup, operation] = await backup.create(...); await operation.promise(); console.log('finished creating backup');

Or

backup.create(..., (err, backup, operation) => { if (err) {...} operation .on('error', (err) => {...}) .on('complete', () => console.log('finished creating backup')); })

AVaksman · 2020-08-19T16:10:54Z

src/backup.ts

+export type RestoreTableCallback = (
+  err: ServiceError | null,
+  table: Table | null,
+  apiResponse?: LROperation<
+    google.bigtable.admin.v2.ITable,
+    google.bigtable.admin.v2.IRestoreTableMetadata
+  >
+) => void;
+export type RestoreTableResponse = [
+  Table,
+  LROperation<
+    google.bigtable.admin.v2.ITable,
+    google.bigtable.admin.v2.IRestoreTableMetadata
+  >
+];


restoreTable callback/response should follow the same pattern as createBackup callback/response above.

AVaksman · 2020-08-19T16:24:21Z

src/backup.ts

+   *   described at https://aip.dev/132#ordering.
+   */
+  orderBy?: string;
+


should add pageSize?: number and pageToken?:string here as well.
should respect (until pageSize and pageToken are removed from CallOptions) same values if past within gaxOptions
Top level values should take precedence

reqOpts = extend( {}, { pageSize: gaxOpts.pageSize, pageToken: gaxOpts.pageToken, }, reqOpts ); delete gaxOpts.pageSize; delete gaxOpts.pageToken;

AVaksman · 2020-08-19T17:55:43Z

src/backup.ts

+export type GetBackupsResponse = [Backup[], IBackup[]];
+export type GetBackupsCallback = (
+  err: ServiceError | null,
+  backups?: Backup[],
+  apiResponse?: IBackup[]
+) => void;


GetBackupCallback/Response should follow the following pattern

export type GetBackupsCallback = ( err: ServiceError | null, backups?: Backup[], nextQuery?: GetBackupsOptions | null, apiResponse?: google.bigtable.admin.v2.IListBackupsResponse | null ) => void; export type GetBackupsResponse = [ Backup[], GetBackupsOptions; google.bigtable.admin.v2.IListBackupsResponse; ]

AVaksman · 2020-08-20T03:12:42Z

src/instance.ts

+  backup(
+    idOrName: string,
+    cluster: string | Cluster,
+    fields?: ModifiableBackupFields


As per my comment above, I do not think fields are needed in Backup constructor

AVaksman · 2020-08-20T03:17:26Z

src/instance.ts

+      if (!clusterId) {
+        throw new Error('A complete backup name (path) is required.');
+      }
+      backupFrom = new Backup(this.cluster(clusterId), backup);


Suggested change

backupFrom = new Backup(this.cluster(clusterId), backup);

backupFrom = this.backup(this.cluster(clusterId), backup);

AVaksman · 2020-08-20T03:24:43Z

src/instance.ts

+    tableId: string,
+    backup: Backup | string,


Should wrap it in options object

AVaksman · 2020-08-20T03:26:10Z

src/instance.ts

+    const tableAdminClient = this.bigtable.api[
+      'BigtableTableAdminClient'
+    ] as BigtableTableAdminClient;


this.bigtable.api may not contain a BigtableTableAdminClient at this point.

AVaksman · 2020-08-20T14:59:00Z

src/table.ts

+   * @example <caption>include:samples/document-snippets/table.js</caption>
+   * region_tag:bigtable_create_table
+   */
+  backup(


Not sure about this convenience function, as it internally sends additional RPC.

zamnuts added type: enhancement priority: p0 Highest priority. Critical issue. P0 implies highest priority. api: bigtable Issues related to the googleapis/nodejs-bigtable API. labels Jul 13, 2020

zamnuts requested review from JustinBeckwith and alexander-fenster July 13, 2020 11:43

zamnuts requested a review from a team as a code owner July 13, 2020 11:43

zamnuts self-assigned this Jul 13, 2020

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 13, 2020

AVaksman reviewed Jul 14, 2020

View reviewed changes

crwilcox suggested changes Jul 16, 2020

View reviewed changes

zamnuts added 16 commits July 28, 2020 05:10

feat(backups): implement create/get/list/delete backups with sample

d5b89e4

docs(backups): add examples, samples, extra method signatures, fix de…

8a9a7cb

…lete timeouts, expireTime is required

feat(table): add backup helper right on the table that auto selects t…

a521af3

…he cluster

feat(backups): implement update and restore backup methods, refactor …

08ff778

…expire to field encapsulation for flexibility

feat(backups): add Backup class wrapper with helper methods, include …

1a0a1d2

…precise-date for backup timestamps

feat(backups): stream list of backups, export new backup types, remov…

514b7bb

…e dupe proto import, import reorder

fix(backups): support optimize replstate during backup, support Date …

f992154

…for backup timestamps, show Backup usage in sample

fix(table): after refactoring tests to promisify fixed missing id che…

282dfee

…ck bug

fix(backups): id checks for cluster, keep cluster promise

9fd8e0f

test(backups): table backup helper

98264e6

test: keep promisify during tests, fix improper tests that did not co…

c61112a

…nsider promise wrapping

fix: after refactoring tests to promisify fixed missing id check bugs

e36a5fa

fix(backups): list pagination full impl, expiretime checks, allow tab…

f9e9659

…le name instead of reference, orig backup as public metadata

test(backups): cluster, instance, and table coverage

a15f35b

test(backups): full unit test coverage for all new backup code

a12bd65

test(backups): system tests for all backup methods, added asBackup ca…

e6420bd

…sting helper and update sample accordingly

docs(backups): fix sample test case stdout partial match

1b69be9

zamnuts force-pushed the feature/introduce-backups branch from 4d8a372 to 1b69be9 Compare July 28, 2020 12:11

JustinBeckwith suggested changes Jul 28, 2020

View reviewed changes

AVaksman suggested changes Jul 30, 2020

View reviewed changes

kolea2 reviewed Jul 30, 2020

View reviewed changes

feat(backups): Backup is the primary class representing and providing…

1be71e2

… to interact with backups

AVaksman suggested changes Aug 20, 2020

View reviewed changes

stephenplusplus assigned stephenplusplus and unassigned zamnuts Sep 1, 2020

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

stephenplusplus mentioned this pull request Sep 2, 2020

Introduce Backups #794

Merged

10 tasks

zamnuts closed this Sep 14, 2020

	export interface LongRunningResourceCallback<Resource> {
	(
	err: ServiceError \| null,
	resource?: Resource,
	operation?: Operation,
	apiResponse?: IOperation
	): void;
	}
	export type CreateInstanceCallback = LongRunningResourceCallback<Instance>;
	export type CreateInstanceResponse = [Instance, Operation, IOperation];

	const self = this;
	const transformToAppProfile = (
	chunk: google.bigtable.admin.v2.IAppProfile,
	enc: string,
	callback: Function
	) => {
	const appProfile = self.appProfile(chunk.name!.split('/').pop()!);
	appProfile.metadata = chunk;
	callback(null, appProfile);
	};
	return pumpify.obj([
	this.bigtable.request({
	client: 'BigtableInstanceAdminClient',
	method: 'listAppProfilesStream',
	reqOpts,
	gaxOpts: gaxOptions,
	}),
	new Transform({objectMode: true, transform: transformToAppProfile}),

		export type DeleteBackupCallback = GenericBackupCallback<IEmpty>;
		export type DeleteBackupResponse = [Backup, IEmpty];

	backupFrom = new Backup(this.cluster(clusterId), backup);
	backupFrom = this.backup(this.cluster(clusterId), backup);

Conversation

zamnuts commented Jul 13, 2020

Uh oh!

zamnuts commented Jul 13, 2020

Uh oh!

AVaksman commented Jul 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

codecov bot commented Jul 28, 2020 •

edited

Loading