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 sync streams and add diffs for annotations and owner references #2728

Merged
merged 4 commits into from
Aug 14, 2024

Conversation

FxKu
Copy link
Member

@FxKu FxKu commented Aug 13, 2024

in our internal e2e tests I found the following logs:

level=warning msg="database replication slots for streams with applicationId test-app not in sync, skipping event stream sync" 
level=warning msg="database replication slots for streams with applicationId test-app-2 not in sync, skipping event stream sync"

The stream with test-app-2 uses a slot with not existing schema and table hence the expected second log line. But the stream of the first app should be fine. I have extended and improved the TestHasSlotsInSync unit test but found no bugs. After adding a logger, I found the issue:

In #2704 we removed always setting a slot to sync in order not block syncing broken streams. However, now they are only set on create, update or delete, but not on a normal sync. Therefore, hasSlotsInSync does not find the desired slots in slotsToSync and skips the stream sync. The proposed solution sets a slot if

If we find out later that the publication can be removed it is set to nil. This code already exists, too.

I'm using this PR to also introduce diffs for annotations and owner references in streams. It required some rewriting of unit tests.

The changes on maintenanceWindows function were triggered because the fake stream client was used before which wasn't working anymore after my changes. First, I choose another client from the same go files (newFakeK8sAnnotationsClient) but then I realized, it's better to just pass the maintenance windows from the manifest to this function to simplify the code.

@FxKu FxKu added the bugfix label Aug 13, 2024
@FxKu FxKu added this to the 1.13.0 milestone Aug 13, 2024
@FxKu FxKu changed the title extend and improve hasSlotsInSync unit test fix sync streams and add diffs for annotations and owner references Aug 14, 2024
@idanovinda
Copy link
Member

👍

1 similar comment
@FxKu
Copy link
Member Author

FxKu commented Aug 14, 2024

👍

@FxKu FxKu merged commit c7ee34e into master Aug 14, 2024
12 checks passed
@FxKu FxKu deleted the syncstream-fix branch August 14, 2024 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants