[AVaksman]

• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

#ToDo

• Add System test
• Add Samples for
  • Backup#restoreTo()
  • Instance#createTableFromBackup()

This PR proposes the following design

• Introduce an additional function Backup#restoreTo(config, callback?) which accepts a config parameter of the type

  export interface RestoreTableConfig {
    tableId: string;
    instance?: Instance | string;
    gaxOptions?: CallOptions;
  }

• Backup#restore() delegates to Backup#restoreTo() with this.cluster.instance value for config.instance.
• Instance#createTableFromBackup() now respects both
  • Backup object created on a different instance.
  • As well as a string of full backup name (path) that contains a different (form this.id) instanceId.

Other design options

!Breaking:

• Replace Backup#restore() with the proposed Backup#restoreTo() implementation
• Include the changes with current pending 4.0.0 release

Non Breaking:

• Add an optional instance parameter to Backup#restore()
  • This design might be a bit messier than desired

    restore(
      tableId: string,
      instanceOrGaxOptionsOrCallback?:
        | Instance
        | string
        | CallOptions
        | RestoreTableCallback,
      gaxOptionsOrCallback?: CallOptions | RestoreTableCallback,
      cb?: RestoreTableCallback
    ): void | Promise<RestoreTableResponse> {...}

  • This design will diverge from the established pattern

     function(OneRequiredParamIfAny, optionsOrCallback?, callback?) {...}

[codecov]

Codecov Report

Merging #830 (53d9ca5) into backup-different-instance (0ef8976) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                    Coverage Diff                     @@
##           backup-different-instance     #830   +/-   ##
==========================================================
  Coverage                      99.22%   99.22%           
==========================================================
  Files                             18       18           
  Lines                          17416    17488   +72     
  Branches                        1025     1033    +8     
==========================================================
+ Hits                           17281    17353   +72     
  Misses                           132      132           
  Partials                           3        3           

Impacted Files	Coverage Δ
src/backup.ts	100.00% <100.00%> (ø)
src/instance.ts	100.00% <100.00%> (ø)

---

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 0ef8976...53d9ca5. Read the comment docs.

[stephenplusplus]

@AVaksman is there any additional context for the reasoning of this addition?

[kolea2]

@AVaksman is there any additional context for the reasoning of this addition?

Hey @stephenplusplus! We will be working on this in all languages, here's the Java draft for reference: googleapis/java-bigtable#515

[stephenplusplus]

@kolea2 thank you!

[snippet-bot]

Here is the summary of changes.

You added 1 region tag.

• samples/backups.restore.js:20, tag bigtable_restore_backup

You deleted 1 region tag.

• samples/backups.restore.js:20, tag bigtable_restore_backups

---

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

• Refresh this comment

[kolea2]

I think we may want to leave some of these tests as is, so we don't lose coverage on .restore() functionality?

[AVaksman]

To have gRPC call implementation only in one place Backup#restore() (as more restrictive wrapper since can only restore to the same instance) delegates to Backup#restoreTo() and passing this.cluster.instance value as the config.instance param.
The current unit tests for Backup#restore() are to verify that it properly delegates to Backup#restoreTo() with and without optional gaxOptions param.

[kolea2]

got it, just want to confirm we still have at least one restore test in place for coverage?

[kolea2]

Looking good, merging into backup-different-instance as a WIP