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

chore: Refactor ReplicationClient into a state machine #247

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

alco
Copy link
Member

@alco alco commented Aug 2, 2024

Seeing how the setup code in Electric.Postgres.ReplicationClient has been getting more complicated, I've made an attempt to refactor it into a separate module to make it clear which query follows which before the connection is switched into the logical streaming mode.

In the end I have to say, that the amount of boilerplate has exceed my expectations, not in a good way. Still, I would like another person's opinion on whether this change makes it easier to reason about the replication client and its two operating modes: 1) connection setup and 2) logical streaming.

Ideally, I would have loved to use e.g. epgsql for the connection setup, then pass the connection socket over to Postgrex.ReplicationConnection for streaming. That would have made the code much simpler.

@alco alco marked this pull request as ready for review August 3, 2024 07:33
@alco alco changed the title Refactor ReplicationClient into a state machine chore: Refactor ReplicationClient into a state machine Aug 3, 2024
Copy link
Contributor

@icehaunter icehaunter left a comment

Choose a reason for hiding this comment

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

I like the split, but I think start/1 and other public functions in ConnectionSetup could use a @spec

Copy link
Contributor

@robacourt robacourt left a comment

Choose a reason for hiding this comment

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

Yeah, that seems easier to reason about to me. Nice one!

@alco alco force-pushed the alco/replication-client-refactor branch from 6cb0c2b to f5e66d0 Compare August 5, 2024 15:05
@alco alco merged commit 156bef9 into main Aug 5, 2024
14 checks passed
@alco alco deleted the alco/replication-client-refactor branch August 5, 2024 15:10
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