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

fix: remove the watermarks #1313

Merged
merged 6 commits into from
Aug 17, 2023

Conversation

danieljbruce
Copy link
Contributor

Removing the watermarks allows data not to be stuck at the last transform so that data doesn’t get stuck due to the watermarks.

Fixes #1286

Removing the watermarks allows data not to be stuck at the last transform so that data doesn’t get stuck due to the watermarks.
@danieljbruce danieljbruce requested review from a team as code owners August 9, 2023 15:00
@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: bigtable Issues related to the googleapis/nodejs-bigtable API. labels Aug 9, 2023
@danieljbruce danieljbruce changed the title remove the watermarks fix: remove the watermarks Aug 11, 2023
@@ -252,6 +252,7 @@ describe('Bigtable/ReadRows', () => {
// Transform stream
const transform = new Transform({
objectMode: true,
writableHighWaterMark: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

why is writableHighWaterMark only set in the test and not in the implementation (in src/table.ts) similar to readableHighWaterMark ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prior to this PR, data gets trapped between this transform and the readStream transform and when the user cancels the stream the data is released resulting in extra rows. Removing the two watermarks prevents the data from getting trapped which means when the user cancels the streams no extra trapped data gets through so the right number of rows reaches the user.

@danieljbruce danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 17, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 17, 2023
@danieljbruce danieljbruce merged commit 0126a0e into googleapis:main Aug 17, 2023
15 checks passed
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: xs Pull request size is extra small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

readStream.end() called asynchronously returns 16 extra rows before stopping
3 participants