-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: Move BT chunk processing into retry #806
Conversation
Bigtable stream reads are not reliable, and may fail at any time. Move the chunk processing into the retry section to reduce the number of read errors that can happen. Closes: SYNC-4508
*NOTE* `cargo audit` may fail with an "Invalid version" if the `Cargo.lock` version is set to 4. Manually changing to 3 will resolve this.
error::BigTableError::GRPC(e) | ||
| error::BigTableError::Read(e) |
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.
I don't see these 2 variants being emitted in merge
but I imagine you're guarding against them potentially happening in the future.
Being generic across BigTableError
here convinced me we might as well clean all this up by making a retryable_bigtable_error
version of retryable_error
. How does this look?
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.
Yeah, aside from the "InvalidRowResponse" it's behaving a lot like a generic GRPC error retry clause, but I don't think that it's worth the hassle of doing a stack call for just that. WFM.
Bigtable stream reads are not reliable, and may fail at any time.
Move the chunk processing into the retry section to reduce the number
of read errors that can happen.
Version and cargo update included because of an audit update.
Closes: SYNC-4508