-
Notifications
You must be signed in to change notification settings - Fork 56
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: ensure that during resumption of a scan, rows that have not been observed by the caller are re-requested #1444
Merged
danieljbruce
merged 35 commits into
googleapis:main
from
danieljbruce:fix-missing-rows-with-test-and-fix-for-node-14-plus-watermark-removal
Jul 11, 2024
Merged
Changes from 30 commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
9e2c037
fix: dropping buffered rows during a retry of a scan
igorbernstein2 2a4d928
Add a test that sends rows back
danieljbruce d8e2e91
Create an instance with dummy server.
danieljbruce d803684
Add test parameters for sending back the right chu
danieljbruce a8bebb1
Omit server start
danieljbruce fdc5c0d
Run a test against the server in the old code
danieljbruce 80f4fca
Add logging to the chunk transformer
danieljbruce 580d65f
Add logging to indicate that the server received r
danieljbruce b08288a
chunk transformer logs and other logs
danieljbruce e4d9b86
Add a log
danieljbruce 9d41781
Don’t change the old server
danieljbruce 2d9b603
Add another guard against the logs
danieljbruce 5bad7c0
Add setImmediate everywhere that it needs to be
danieljbruce e61343d
Remove the logging
danieljbruce 138d105
Remove more logging
danieljbruce f35fc3e
Adjust header
danieljbruce bf74092
Add the high watermarks back in
danieljbruce ff13036
Remove the at accessor
danieljbruce 87c57a9
Eliminate the watermark adjustments
danieljbruce 09d9155
Introduce the watermarks back in
danieljbruce 89c7c53
Reduce the number of watermark removals to 1.
danieljbruce f60e790
Reverted the streamEvents pipeline
danieljbruce d07aa9e
Add some comments for introducing the new waterma
danieljbruce 537760b
Remove comments and console logs. Add TODO.
danieljbruce 96f98d7
Add TODO
danieljbruce 7b03b84
refactor the test with a helper method
danieljbruce cffcd57
Fix linting issue
danieljbruce 52675d1
Adding a comment about the mock
danieljbruce c62ed72
readable comment change
danieljbruce d78d119
Update test/readrows.ts
danieljbruce 8c0726c
It uses 150 rows not 1000 rows
danieljbruce eba2195
Add a TODO for making more specific typing
danieljbruce 0a86df7
Add some TODOs for better factoring
danieljbruce 85901e5
Merge branch 'fix-missing-rows-with-test-and-fix-for-node-14-plus-wat…
danieljbruce 88236d1
Add interface: server writable stream
danieljbruce File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this comment about backpressure still true given that the highwatermark is set to 0? if not, please remove, otherwise, feel free to resolve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still true because we want to create the scenario where there is backpressure in the chunk transformer and other streams in order to reproduce the issue that occurs when these transforms are thrown away from before the fix. Note that this fix only applies a highwatermark of 0 to the user stream.
However, this comment still does need an adjustment from 1000 to 150 :)