surface rather than hide error handling#135
Conversation
Signed-off-by: Max Englander <max@planetscale.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the sync process to expose errors more clearly and simplify error handling during the VStream processing. Key changes include:
- Splitting up the sync() function parameters for better readability.
- Introducing a local logf closure for consistent logging.
- Replacing panic calls in unexpected event cases with error returns.
| switch { | ||
| case ok && s.Code() == codes.DeadlineExceeded: | ||
| // No next VGTID found within deadline. | ||
| logf(LOGLEVEL_ERROR, "no new VGTID found before deadline exceeded: %v", err) |
There was a problem hiding this comment.
The DeadlineExceeded condition is likely an expected scenario; consider using an informational log level instead of LOGLEVEL_ERROR.
| logf(LOGLEVEL_ERROR, "no new VGTID found before deadline exceeded: %v", err) | |
| logf(LOGLEVEL_INFO, "no new VGTID found before deadline exceeded: %v", err) |
| logf(LOGLEVEL_ERROR, "no new VGTID found before deadline exceeded: %v", err) | ||
| case errors.Is(err, io.EOF): | ||
| // EOF is an acceptable error indicating VStream is finished. | ||
| logf(LOGLEVEL_ERROR, "encountered EOF, possibly indicating end of VStream") |
There was a problem hiding this comment.
EOF is typically an acceptable signal indicating the end of the stream; consider logging it at an informational level rather than as an error.
| logf(LOGLEVEL_ERROR, "encountered EOF, possibly indicating end of VStream") | |
| logf(LOGLEVEL_INFO, "encountered EOF, possibly indicating end of VStream") |
nickvanw
left a comment
nickvanw
left a comment
There was a problem hiding this comment.
I skipped some of the parts that are copy/pasted, but this looks good to me at first read
| isFullSync := syncMode == "full" | ||
| vtgateReq := buildVStreamRequest(tabletType, s.Name, tc.Shard, tc.Keyspace, tc.Position, tc.LastKnownPk) | ||
| p.Logger.Log(LOGLEVEL_INFO, fmt.Sprintf("%sRequesting VStream with %+v", preamble, vtgateReq)) | ||
| logf(LOGLEVEL_INFO, "Requesting VStream with %+v", vtgateReq) |
There was a problem hiding this comment.
This logging change is a very welcome change
Surface rather than hide errors that are encountered throughout the sync process. Only update table cursor position after successfully flushing records to Airbyte.
This is a lot, so splitting across multiple PRs:
sync()a bit to make subsequent refactoring a bit easier.Once all PRs are approved, I'll merge them from the bottom up, and then merge this PR into
main.