Skip to content

Conversation

@dylanlott
Copy link
Contributor

@dylanlott dylanlott commented Mar 30, 2022

Description

Adds the snapshot iterator to the logrepl package.

This is the first step towards adding a snapshot that works seamlessly with CDC mode.

We define a SnapshotIterator that takes a config which allows for a SnapshotName to be passed and iterates over
the rows from the configured table at that snapshot's point in time.

We necessarily secure a transaction with READ REPEATABLE from the database for our iterator to read.

Related to #15

Quick checks:

  • I have followed the Code Guidelines.
  • There is no other pull request for the same update/change.
  • I have written unit tests.
  • I have made sure that the PR is of reasonable size and can be easily reviewed.

@dylanlott dylanlott force-pushed the dylan/snapshot-iterator branch from db10d48 to d045009 Compare March 30, 2022 23:19
@dylanlott dylanlott changed the title [WIP] snapshot iterator adds snapshot iterator Apr 1, 2022
@dylanlott dylanlott force-pushed the dylan/snapshot-iterator branch from 2344550 to f4fd084 Compare April 1, 2022 17:22
@dylanlott dylanlott marked this pull request as ready for review April 1, 2022 17:24
@dylanlott dylanlott requested a review from a team as a code owner April 1, 2022 17:24
@dylanlott dylanlott self-assigned this Apr 1, 2022
@dylanlott dylanlott marked this pull request as draft April 1, 2022 18:06
@dylanlott dylanlott force-pushed the dylan/snapshot-iterator branch from ae77df4 to 53be2c2 Compare April 1, 2022 18:11
@dylanlott dylanlott marked this pull request as ready for review April 5, 2022 03:30
@dylanlott dylanlott requested a review from lovromazgon April 5, 2022 03:30
Copy link
Contributor

@lovromazgon lovromazgon left a comment

Choose a reason for hiding this comment

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

A couple of nitpicks, but the main things are the open transaction and the unchecked context in Next, after this we should be good 👍 The error related issues will be addressed in #27 so I skipped those.

Comment on lines 88 to 94
rows, err := tx.Query(context.Background(), query)
is.NoErr(err)

var name *string
is.True(rows.Next())
err = rows.Scan(&name)
is.NoErr(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: we can simplify using QueryRow, we know there will be only one row. Also name doesn't have to be a pointer, and we can use ctx as the context.

Suggested change
rows, err := tx.Query(context.Background(), query)
is.NoErr(err)
var name *string
is.True(rows.Next())
err = rows.Scan(&name)
is.NoErr(err)
row := tx.QueryRow(ctx, query)
var name string
err = row.Scan(&name)
is.NoErr(err)

Copy link
Contributor Author

@dylanlott dylanlott Apr 7, 2022

Choose a reason for hiding this comment

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

We actually can't use QueryRow here, because it messes with our transaction when we call Scan, which handles its own connection cleanup. Will update to match the rest, though 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried it out and it works for me 🤔

Copy link
Contributor

@lovromazgon lovromazgon Apr 7, 2022

Choose a reason for hiding this comment

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

If this indeed doesn't work we should open an issue in the driver repo because this doesn't do anything out of the ordinary, just a query within a transaction.

EDIT: I realized you are talking about the closing of the rows - that is implicit in here yes, but that doesn't mean that the connection or transaction is cleaned up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I tested this before I wrote my first comment to verify that behavior still persisted, and it broke exactly as I expected it to the first time... but after you said you tested it and it worked for you, I went back and tested it again, and... it worked? 🤔 I guess I'll accept a passing test 😅

Copy link
Contributor

@lovromazgon lovromazgon left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Left two more comments, but approving already!


err = s.loadRows(ctx)
if err != nil {
if rollErr := s.tx.Rollback(ctx); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if rollErr := s.tx.Rollback(ctx); err != nil {
if rollErr := s.tx.Rollback(ctx); rollErr != nil {

Comment on lines 65 to 66
sdk.Logger(ctx).Err(err).Msg("load rows failed")
return nil, fmt.Errorf("rollback failed: %w", rollErr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I'd rather turn the logic around and just log the rollback error (same as we do in line 107)

Suggested change
sdk.Logger(ctx).Err(err).Msg("load rows failed")
return nil, fmt.Errorf("rollback failed: %w", rollErr)
sdk.Logger(ctx).Err(rollErr).Msg("rollback failed")

@dylanlott dylanlott merged commit ec2e67e into main Apr 8, 2022
@dylanlott dylanlott deleted the dylan/snapshot-iterator branch April 8, 2022 16:09
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.

3 participants