Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: first refactor of createReadStream #1118

Merged

Conversation

danieljbruce
Copy link
Contributor

@danieljbruce danieljbruce commented Jun 30, 2022

This is the first of many PRs to refactor createReadStream. We need to reduce the size of this function so that we can work with it and so that adding retry features is easier.

API surface affected:

For tests that look at the API surface:
https://github.com/googleapis/nodejs-bigtable/blob/main/test/table.ts#L539
https://github.com/googleapis/nodejs-bigtable/blob/main/system-test/read-rows.ts#L154

In this PR, we pull out code to calculate the ranges variable in createReadStream into a separate function. The main motivation for this besides reducing the createReadStream function size is to take code, which calculates ranges initially and move it into a separate function since this initial calculation has nothing to do with what happens in the rest of the function after that. This conceptually tells the code reader that only options is needed to calculate ranges initially and that all code used to calculate ranges initially can be ignored if the reader is only interested in what happens in the rest of the function.

So....
https://github.com/googleapis/nodejs-bigtable/pull/1118/files#diff-6400d8602f0855266c743f4fb031e97ac9cf07f9dcddfba03833b6f04d547a5eL750 to https://github.com/googleapis/nodejs-bigtable/pull/1118/files#diff-6400d8602f0855266c743f4fb031e97ac9cf07f9dcddfba03833b6f04d547a5eL780)

should just be a cut/paste of
https://github.com/googleapis/nodejs-bigtable/pull/1118/files#diff-2490acae1e166b5d2477465b3ee761d0857374c409d215d0363d626c665e80d6R20 to
https://github.com/googleapis/nodejs-bigtable/pull/1118/files#diff-2490acae1e166b5d2477465b3ee761d0857374c409d215d0363d626c665e80d6R48

The only other two minor changes are the changes to the test stub and the change to createPrefixRange. The test stub changes which include all the changes in test/table.ts are necessary as a result of pulling out ranges into a utility function and the way that sinon works in the tests. table.createPrefixRange delegates its functionality to TableUtils.createPrefixRange which does the exact same thing table.createPrefixRange did (we are just cut/pasting the code), but is something we need to do because of sinon stubs. Basically, sinon gets in the way of just being able to pull code out because it mocks createPrefixRange from Table, but TableUtils has a different copy of Table so while a test for prefixes should work, in this case the test needs to be slightly modified because it also demands that everything happens in Table which shouldn't be necessary. But if this is undesirable then we can consider using a private method instead of a utility class.

@danieljbruce danieljbruce requested review from a team as code owners June 30, 2022 20:59
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: bigtable Issues related to the googleapis/nodejs-bigtable API. labels Jun 30, 2022
@danieljbruce danieljbruce added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 4, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 4, 2022
@danieljbruce danieljbruce added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 8, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 8, 2022
Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs a bit more refactoring. Currently this creates a circular dependency between TableUtils and Table, which is a bad code smell

@danieljbruce
Copy link
Contributor Author

I think this needs a bit more refactoring. Currently this creates a circular dependency between TableUtils and Table, which is a bad code smell

This circular dependency is just a result of some cleanup that wasn't done. I have added a commit so that there aren't circular dependencies anymore which turned out to be a fairly simple change. If you want to change the utils class name then we can do that too.

@danieljbruce
Copy link
Contributor Author

danieljbruce commented Jul 12, 2022

More refactoring is done in the next two PRs.

@danieljbruce
Copy link
Contributor Author

If you want to see a larger refactor then take a look at this PR:

#1121

Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@danieljbruce danieljbruce added owlbot:run Add this label to trigger the Owlbot post processor. labels Jul 14, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 14, 2022
@danieljbruce danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 14, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 14, 2022
@danieljbruce danieljbruce merged commit 0dfb960 into googleapis:main Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the googleapis/nodejs-bigtable API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants