chore: remove paginator dependency and use gapic to return streams for paginated calls instead #762

AVaksman · 2020-07-13T17:43:22Z

gapic supports both paginated calls and its streamified versions.

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

…alls instead

codecov · 2020-07-13T17:49:19Z

Codecov Report

Merging #762 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #762   +/-   ##
=======================================
  Coverage   99.13%   99.14%           
=======================================
  Files          17       17           
  Lines       15774    15818   +44     
  Branches      951      959    +8     
=======================================
+ Hits        15638    15682   +44     
  Misses        133      133           
  Partials        3        3

Impacted Files	Coverage Δ
src/index.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 32c72c3...eeac2d5. Read the comment docs.

stephenplusplus · 2020-07-16T15:25:39Z

@AVaksman I know it's still a draft, so sorry if I'm jumping in too soon. But, I was wondering if you could describe the intentions behind this when you get the chance? Thanks!

AVaksman · 2020-07-16T17:28:51Z

gapic now supports paginated calls and its streamified versions directly.

stephenplusplus · 2020-07-16T17:50:51Z

Does it also retry requests?

AVaksman · 2020-07-16T20:10:41Z

Yes, gax supports retries for paged calls.
RetryOptions{retryCodes; backoffSettings;} can be passed via gaxOptions.retry argument.
Streamified version of paged call is essentially same paged call where results pushed one by one via an object mode stream.

stephenplusplus · 2020-07-16T15:19:17Z

src/index.ts

@@ -750,16 +750,20 @@ export class Bigtable {
      });
    };

+    const streamified = {listTablesStream: true};


Can we use a different variable name to better describe the difference between a method that is "streamified" and one that isn't? Does this list effectively hold GAPIC streaming methods? If so, maybe const gapicStreamingMethods = {...};?

stephenplusplus · 2020-07-16T15:22:05Z

src/index.ts

@@ -805,6 +809,20 @@ export class Bigtable {
          .pipe(stream);
      });
    }
+
+    function makeRequestStreamified() {


Let's rename this, too, to be more clear. Maybe makeGapicStreamRequest()?

stephenplusplus · 2020-07-16T15:24:01Z

src/instance.ts

+
+    // eslint-disable-next-line @typescript-eslint/no-this-alias
+    const self = this;
+    const transform = function (


Would this work (minus annotations and formatting)?

// name change to indicate purpose of method const transformToTable = (chunk, enc, next) => { // no "this" argument const table = this.table(chunk.name); // the Table constructor should support raw input table.metadata = chunk; next(null, table); };

Done!
Have to pass tableId only thought, Table constructor checks for the proper projectId presence.

…lates (googleapis#769) This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/5e903fff-57bb-4395-bb94-8b4d1909dbf6/targets - [ ] To automatically regenerate this PR, check this box. Source-Link: googleapis/synthtool@21f1470 Source-Link: googleapis/synthtool@388e10f

This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/5e903fff-57bb-4395-bb94-8b4d1909dbf6/targets - [ ] To automatically regenerate this PR, check this box.

stephenplusplus · 2020-07-20T13:48:48Z

Thanks!

AVaksman · 2020-07-27T15:29:40Z

@stephenplusplus I added another unit test after the approval

stephenplusplus · 2020-07-27T15:34:41Z

Nice, lgtm!

chore: drop paginator and use gapic to return streams for paginated c…

2243a22

…alls instead

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

chore: unit tests

392ecd6

chore: more tests

d28f4a1

AVaksman changed the title ~~chore: drop paginator and use gapic to return streams for paginated calls instead~~ chore: drop paginator and use gapic to return streams for paginated calls instead Jul 16, 2020

AVaksman marked this pull request as ready for review July 16, 2020 17:28

AVaksman requested a review from a team as a code owner July 16, 2020 17:28

Merge branch 'master' into drop_paginator

6b31eba

stephenplusplus suggested changes Jul 17, 2020

View reviewed changes

AVaksman and others added 6 commits July 17, 2020 15:15

refactor: better names

cd79876

refactor: minor

f9dddac

build: add Node 8 tests (googleapis#768)

919013b

This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/5e903fff-57bb-4395-bb94-8b4d1909dbf6/targets - [ ] To automatically regenerate this PR, check this box.

refactor: fixy

6b81f5e

Merge branch 'master' into drop_paginator

0b5d370

stephenplusplus approved these changes Jul 20, 2020

View reviewed changes

AVaksman mentioned this pull request Jul 20, 2020

feat: add gapic getAppProfilesStream method #771

Merged

3 tasks

AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 21, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 21, 2020

AVaksman changed the title ~~chore: drop paginator and use gapic to return streams for paginated calls instead~~ chore: remove paginator dependency and use gapic to return streams for paginated calls instead Jul 22, 2020

AVaksman and others added 4 commits July 21, 2020 22:59

chore: add error unit test

e9839fd

Merge branch 'master' into drop_paginator

d9024ac

Merge branch 'master' into drop_paginator

b1ff4de

Merge branch 'master' into drop_paginator

eeac2d5

AVaksman merged commit 8a2a74e into googleapis:master Jul 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove paginator dependency and use gapic to return streams for paginated calls instead #762

chore: remove paginator dependency and use gapic to return streams for paginated calls instead #762

AVaksman commented Jul 13, 2020 •

edited

Loading

codecov bot commented Jul 13, 2020 •

edited

Loading

stephenplusplus commented Jul 16, 2020

AVaksman commented Jul 16, 2020

stephenplusplus commented Jul 16, 2020

AVaksman commented Jul 16, 2020

stephenplusplus Jul 16, 2020

AVaksman Jul 17, 2020

stephenplusplus Jul 16, 2020

AVaksman Jul 17, 2020

stephenplusplus Jul 16, 2020

AVaksman Jul 17, 2020

stephenplusplus commented Jul 20, 2020

AVaksman commented Jul 27, 2020

stephenplusplus commented Jul 27, 2020

chore: remove paginator dependency and use gapic to return streams for paginated calls instead #762

chore: remove paginator dependency and use gapic to return streams for paginated calls instead #762

Conversation

AVaksman commented Jul 13, 2020 • edited Loading

codecov bot commented Jul 13, 2020 • edited Loading

Codecov Report

stephenplusplus commented Jul 16, 2020

AVaksman commented Jul 16, 2020

stephenplusplus commented Jul 16, 2020

AVaksman commented Jul 16, 2020

stephenplusplus Jul 16, 2020

Choose a reason for hiding this comment

AVaksman Jul 17, 2020

Choose a reason for hiding this comment

stephenplusplus Jul 16, 2020

Choose a reason for hiding this comment

AVaksman Jul 17, 2020

Choose a reason for hiding this comment

stephenplusplus Jul 16, 2020

Choose a reason for hiding this comment

AVaksman Jul 17, 2020

Choose a reason for hiding this comment

stephenplusplus commented Jul 20, 2020

AVaksman commented Jul 27, 2020

stephenplusplus commented Jul 27, 2020

AVaksman commented Jul 13, 2020 •

edited

Loading

codecov bot commented Jul 13, 2020 •

edited

Loading