refactor: Move retries finish making createreadstream smaller #1403
Conversation
This reverts commit c2f4dfe.
….com/danieljbruce/nodejs-bigtable into actually-refactor-createreadstream-3
This reverts commit 4817863.
…into move-retries-from-createreadstream # Conflicts: # src/table.ts # src/utils/table.ts
This reverts commit 5edaf82.
readRowsReqOpts should have an ECMAscript prefix to completely hide it from the user. Also remove a useless Filter.parse.
product-auto-label
bot
added
size: m
| @@ -37,7 +37,6 @@ export class MockServer { | |||
| `localhost:${this.port}`, | |||
| grpc.ServerCredentials.createInsecure(), | |||
| () => { | |||
| server.start(); | |||
There was a problem hiding this comment.
As stated before. start is deprecated for grpc servers.
| @@ -2049,7 +2016,7 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`); | |||
| * that a callback is omitted. | |||
| */ | |||
| promisifyAll(Table, { | |||
| exclude: ['family', 'row'], | |||
| exclude: ['family', 'row', '#readRowsReqOpts'], | |||
There was a problem hiding this comment.
This makes it so our new helper function #readRowsReqOpts works as a normal function and doesn't return a promise.
There was a problem hiding this comment.
This is fine for now, but depending on what we decide on for this eventually, I want to leave this comment open in case we need to change it later
There was a problem hiding this comment.
If we pull #readRowsReqOpts out of this class like populateAttemptHeader then we don't need this anymore and since modifying this exclude variable is not preferred, I think this is a good argument for the change you suggested above which is pulling #readRowsReqOpts out of this class like populateAttemptHeader.
danieljbruce
changed the title
src/table.ts
Outdated
| if (filter) { | ||
| reqOpts.filter = filter; | ||
| } | ||
| const reqOpts: any = this.#readRowsReqOpts(ranges, rowKeys, options); |
There was a problem hiding this comment.
Please address this any!
There was a problem hiding this comment.
Made this change.
| @@ -1640,6 +1576,40 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`); | |||
| makeNextBatchRequest(); | |||
| } | |||
|
|
|||
| #readRowsReqOpts( | |||
There was a problem hiding this comment.
nit that I'm surprised our linter didn't catch - I think you shouldn't use #private notation per our ts style guide. Does this helper function for sure need to be private? (It might - I just want to know more about your thought process! 😁 )
There was a problem hiding this comment.
Additional question - does this need to stay within the table class, or could it be a helper function similar to populateAttemptHeader down below? Tell me more! 🙂
There was a problem hiding this comment.
I think doing what populateAttemptHeader does is a good option. The con is that the function also needs to be able to accept table so needs a fourth argument. The pro is that we don't have to have private ECMA script function syntax that is hard to debug. It's a good suggestion overall that I was wondering about.
We definitely should hide this function from the user either way.
| @@ -2049,7 +2016,7 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`); | |||
| * that a callback is omitted. | |||
| */ | |||
| promisifyAll(Table, { | |||
| exclude: ['family', 'row'], | |||
| exclude: ['family', 'row', '#readRowsReqOpts'], | |||
There was a problem hiding this comment.
This is fine for now, but depending on what we decide on for this eventually, I want to leave this comment open in case we need to change it later
This PR is written to reduce the amount of code in createreadstream so it is easier to work with when moving the retries. Before merging to main we can always revert these changes if we prefer the code to be inline.
Code from createreadstream is pulled out into 3 functions: