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

Fix SR status broadcast on database instance registration #1687

Merged
merged 3 commits into from Aug 2, 2023

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Aug 1, 2023

Description

Fix system replication status broadcast on database instance registration.
Basically, we need all database instances to get the value for each type (primary/secondary).

I want to test this with the whole "restoration" fix for the instances (#1673)

PD: I do agree that this got complex, so querying with a api call on this events as refactor would be a good idea.
PD2: In fact, it has "forced" me to change some domain events order to make it work without horrible "data enrichment" messages to the frontend...

How was this tested?

Everything tested in the projection. I didn't add any test on the view, as the test would be the same.

@arbulu89 arbulu89 added the bug Something isn't working label Aug 1, 2023
@arbulu89 arbulu89 changed the title Fix sr status broadcast on database instance registration Fix SR status broadcast on database instance registration Aug 1, 2023
@arbulu89 arbulu89 force-pushed the fix-system-replication-status-broadcast branch from 385dff9 to 75e0166 Compare August 1, 2023 16:30
@arbulu89 arbulu89 marked this pull request as ready for review August 2, 2023 06:30
Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

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

@arbulu89 looks good to me. Just for a better understanding: what made you have to change the order of the restoration event? (not questioning it, I think it's intuitively makes more sense to me this way)

@arbulu89
Copy link
Contributor Author

arbulu89 commented Aug 2, 2023

@arbulu89 looks good to me. Just for a better understanding: what made you have to change the order of the restoration event? (not questioning it, I think it's intuitively makes more sense to me this way)

Already commented in our internal Slack channel, but here we go again 😆

In order to calculate the Active pill, we need to have all the database instances (specially the primary) present in the database.
During the restoration, we can have a corner case (or normal case, depending what you do...), where you deregister the database simply removing the Primary instance, and letting the Secondary there, without removing, but it is not showing up in the console, so it stays as "zombie".

When you restore the Primary instance, the Secondary was already present there, so it is sent to the Frontend together with the DatabaseRestore event, but as the primary was not there, the Active pill couldn't be calculated, so it is not set.
To calculate it we need the Primary.

Changing the order, maker the Primary instance read model being present in the database for the moment we do the query to do the calculation

I'm saying this is a corner case, because in a normal flow, you couldn't never have a Secondary there without previously having a Primary, but as it stayed as zombie...

@arbulu89 arbulu89 merged commit b1eafb0 into main Aug 2, 2023
18 checks passed
@arbulu89 arbulu89 deleted the fix-system-replication-status-broadcast branch August 2, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

None yet

4 participants