surface rather than hide error handling #135
Conversation
Signed-off-by: Max Englander <[email protected]>
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) |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| 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") |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| @@ -306,17 +317,17 @@ func (p PlanetScaleEdgeDatabase) sync(ctx context.Context, syncMode string, tc * | |||
|
|
|||
| 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.