Add use_secondary config flag

[judahrand]

Problem

Proposed changes

Alternate implementation of #131. I'm not sure how to go about testing this without setting up quite a lot of additional infrastructure. Do you think this needs additional testing?

Types of changes

What types of changes does your code introduce to PipelineWise?
Put an x in the boxes that apply

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation Update (if none of the other choices apply)

Checklist

• Description above provides context of the change
• I have added tests that prove my fix is effective or that my feature works
• Unit tests for changes (not needed for documentation changes)
• CI checks pass with my changes
• Bumping version in setup.py is an individual PR and not mixed with feature or bugfix PRs
• Commit message/PR title starts with [AP-NNNN] (if applicable. AP-NNNN = JIRA ID)
• Branch name starts with AP-NNN (if applicable. AP-NNN = JIRA ID)
• Commits follow "How to write a good git commit message"
• Relevant documentation is updated including usage instructions

[judahrand]

@Samira-El

[Samira-El]

Hey @judahrand, I'm happy with the idea in general as we already have it implemented in PipelineWise FastSync. The issue is that I would prefer if we can rename these new parameters to something else other than replica_* as these are reserved for FastSync.

Will review the PR after renaming :)

re infrastructure, it would be great if we can spin up a replica alongside the docker image in the docker-compose to use for dev and testing purposes.

[judahrand]

Will review the PR after renaming :)

What would you prefer?

re infrastructure, it would be great if we can spin up a replica alongside the docker image in the docker-compose to use for dev and testing purposes.

This sounds painful but I'll give it a VERY quick look given the debezium image used...

[judahrand]

It's also worth adding that PipelineWise FastSync does not spin up a replica either: https://github.com/transferwise/pipelinewise/blob/206e75e630933d1a2b2ab9afb36261a55ccf0e4c/dev-project/docker-compose.yml#L63-L75

[judahrand]

I would prefer if we can rename these new parameters to something else other than replica_* as these are reserved for FastSync.

This also feels confusing to me 🤔Why can they not be used by both? Having to define the replica details twice seems even more confusing? host, port, user, password are already used in both. Why do you feel that this would be a mistake for replica details?

[Samira-El]

This also feels confusing to me thinkingWhy can they not be used by both? Having to define the replica details twice seems even more confusing? host, port, user, password are already used in both. Why do you feel that this would be a mistake for replica details?

It's because it will interfere with how we use replicas internally at Wise, temporary replicas are sometimes spun up to do initial sync or resync with FastSync to avoid overwhelming the primary, but the rest of the flow that uses Singer would use primary.

[Samira-El]

It's also worth adding that PipelineWise FastSync does not spin up a replica either: https://github.com/transferwise/pipelinewise/blob/206e75e630933d1a2b2ab9afb36261a55ccf0e4c/dev-project/docker-compose.yml#L63-L75

I know :)

[judahrand]

This also feels confusing to me thinkingWhy can they not be used by both? Having to define the replica details twice seems even more confusing? host, port, user, password are already used in both. Why do you feel that this would be a mistake for replica details?

It's because it will interfere with how we use replicas internally at Wise, temporary replicas are sometimes spun up to do initial sync or resync with FastSync to avoid overwhelming the primary, but the rest of the flow that uses Singer would use primary.

Hmmm... I see. The replica is still only used if use_replica is true. So it wouldn't actually break anything.

Do you have a preference on the naming in that case? tap_replica_*?

[Samira-El]

Ah naming! one out of the two hardest things in software engineering!

How about secondary_*?

[judahrand]

I've added the usage of the new flag to the tests. This was easiest by adding a parameterisation to an existing test but that required moving away from unittest.TestCase - I hope this is all okay.

This should be ready for review now.

[judahrand]

@Samira-El Could you please give this another look?

[Samira-El]

Looks good in general, really good stuff!
Left few comments