Skip to content

Set the record offset (specifically 'snapshot' and 'snapshot_completed' fields) explicitly based on the incoming source record for the SQL server source.#149

Merged
sagarkapare merged 2 commits intodevelopfrom
fix/set-offset-correctly
Mar 16, 2021
Merged

Set the record offset (specifically 'snapshot' and 'snapshot_completed' fields) explicitly based on the incoming source record for the SQL server source.#149
sagarkapare merged 2 commits intodevelopfrom
fix/set-offset-correctly

Conversation

@sagarkapare
Copy link
Copy Markdown
Contributor

Also made SqlServerOffset class immutable.

Following specific scenario was failing without the fix:

  1. Replicate the source table containing 1 record.
  2. Stop the replication pipeline.
  3. Add more records to source table.
  4. Start replication pipeline.

Pipeline generated DDL events (DROP and CREATE tables) again after step 4 because ddlEventSent field was not persisted at the end of step 1.

@google-cla
Copy link
Copy Markdown

google-cla Bot commented Mar 15, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

SqlServerOffset dmlRecordOffset = new SqlServerOffset(sourceRecord.sourceOffset(), ddlEventSent);
Long ingestTime = val.get("ts_ms");
DMLEvent.Builder dmlBuilder = DMLEvent.builder()
.setOffset(latestOffset)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

latestOffset before fix didn't have updated ddlEventSent set, when only 1 record was sent as a part of snapshot and pipeline was stopped after.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

good catch here :)

// if (!sqlServerOffset.isSnapshot() && sqlServerOffset.isBeforeOrAt(latestOffset)) {
// LOG.debug("Got duplicated event {} ", sourceRecord);
// return;
// }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

uncomment the codes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

// send the ddl events only if we see the table at the first time
// Note: the delta app itself have prevented adding CREATE_TABLE operation into DDL blacklist for all the tables.
if (!ddlEventSent.contains(sourceTableId)) {
ddlEventSent.add(sourceTableId);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

adding table id here may cause some problem , because there are three DDL event sent below :

  1. DROP TABLE
  2. CREATE DATABSE
  3. CREATE TALBE

what if target failed before committing offset of 3 and after committing offset of 1 and 2.

since 1 and 2 already has the offset containing the table id, we won't generate CREATE TABLE event again after resuming.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

my suggestion would be adding it to the offset of first dml event.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

@google-cla
Copy link
Copy Markdown

google-cla Bot commented Mar 15, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

…d' fields) explicitly based on the incoming source record for the SQL server source.
@sagarkapare sagarkapare force-pushed the fix/set-offset-correctly branch from 6634a47 to 798106e Compare March 16, 2021 01:15
@google-cla
Copy link
Copy Markdown

google-cla Bot commented Mar 16, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@sagarkapare sagarkapare merged commit 8fa72e6 into develop Mar 16, 2021
@sagarkapare sagarkapare deleted the fix/set-offset-correctly branch March 16, 2021 01:17
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