Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch replication methods #173

Merged
merged 11 commits into from
Nov 16, 2023
Merged

Switch replication methods #173

merged 11 commits into from
Nov 16, 2023

Conversation

JYOTHINARAYANSETTY
Copy link
Contributor

Description of change

Switching replication method from full_table to incremental and vice versa

QA steps

  • automated tests passing
  • manual qa steps passing (list below)

Risks

low

Rollback steps

  • revert this branch

self.assertTrue(fulltbl_primary_keys.issubset(incrmntl_primary_keys))

#verify that the last message is not a activateversion message for incremental sync
self.assertNotEqual('activate_version', incrmntl_sync_records[stream]['messages'][-1]['action'])

Choose a reason for hiding this comment

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

The activate_version message tells the loaders it is okay to go ahead and load data into the table. If we have sent all of the data, it is okay to start loading. So although this message may not be necessary, it would have no negative consequences.

Also, If they take the appropriate action and fix it so that they do not send the activate_version as the first message for a new table version for incremental, waiting until all data is ready before deleting the previous full table records, Then it would be necessary/critical to have this message be sent.

So in the final version we should assert that the only the last message is an activate version, and as a workaround we should verify that there is an activate message being sent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a qa task in the DOD of the ticket to modify the assertions based on the final implementation.

Comment on lines 80 to 81
self.assertEqual('activate_version', fulltbl_sync_records[stream]['messages'][fulltbl_sync_count+1]
['action'])

Choose a reason for hiding this comment

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

Also, the same comment as above. I think we should expect this and also when the ticket is fixed assert that there is no other message that is an activate_version message. We should probably write out that assertion that will fail and comment it out with a comment mentioning the ticket that will address the issue.

Choose a reason for hiding this comment

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

I don't see a change here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the activate version, added a qa task/note in the ticket itself in the DOD to add or m,odify the assertions based on the final implementation.

Copy link

@HarrisonMarcRose HarrisonMarcRose left a comment

Choose a reason for hiding this comment

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

Approving, but I don't see a change or comment for the activate version in the second test

#verify that the table version incremented after every sync
self.assertGreater(incrmntl_sync_records[stream]['table_version'],
fulltbl_sync_records[stream]['table_version'],
msg = "Table version is not incremented after a successful sync")
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation

@JYOTHINARAYANSETTY JYOTHINARAYANSETTY merged commit b0ce83d into master Nov 16, 2023
7 checks passed
@JYOTHINARAYANSETTY JYOTHINARAYANSETTY deleted the qa/tdl24356 branch November 16, 2023 13:46
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