Skip to content

Conversation

@maxenglander
Copy link
Collaborator

@maxenglander maxenglander commented Apr 3, 2025

Prev: #135

This PR surfaces errors encountered in p.Looger.Flush() and p.Logger.Record(). Also, implement proper checkpointing so that we only update table cursor position after a successful flushing of records to Airbyte.

Next: #137

Signed-off-by: Max Englander <max@planetscale.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR surfaces errors encountered during logger flushing and recording, while also ensuring that table cursor positions get properly updated only after successful record flushing to Airbyte. Key changes include:

  • Refactoring of the cursor variable from currentPosition to tc for improved checkpointing.
  • Enhanced error handling in the flushing and recording methods.
  • Updates to mock and logger implementations to return errors and support queue fullness checks.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
cmd/internal/planetscale_edge_database_test.go Adds verbose logging of log entries during tests.
cmd/internal/planetscale_edge_database.go Refactors checkpointing logic and error handling with updated variables.
cmd/internal/mock_types.go Updates test logger methods to return errors and adds a QueueFull method.
cmd/internal/logger.go Improves logger error handling by returning errors and adding message buffering.
Comments suppressed due to low confidence (1)

cmd/internal/mock_types.go:27

  • The test logger’s QueueFull method returns true as soon as there is any record, which differs from the production logger logic that checks against MaxBatchSize. Consider updating it to use the same threshold to avoid unexpected behavior in tests.
func (tal *testAirbyteLogger) QueueFull() bool { return len(tal.records) > 0 }

Copy link
Contributor

@nickvanw nickvanw left a comment

Choose a reason for hiding this comment

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

This looks good to me on first read, one question about this loop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants