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

[WPB-183] Version federation API queue notifications #3831

Merged
merged 52 commits into from
Mar 7, 2024

Conversation

mdimjasevic
Copy link
Contributor

@mdimjasevic mdimjasevic commented Jan 24, 2024

Tracked by https://wearezeta.atlassian.net/browse/WPB-183.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jan 24, 2024
@mdimjasevic mdimjasevic force-pushed the wpb-183/versioned-queue-notifications branch 4 times, most recently from d48db58 to dcbbd82 Compare January 24, 2024 15:59
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

Looks good so far! I nit-picked around a little.

@mdimjasevic mdimjasevic force-pushed the wpb-183/versioned-queue-notifications branch 14 times, most recently from f8d8b54 to 0f3664d Compare January 26, 2024 16:24
@pcapriotti pcapriotti force-pushed the wpb-183/versioned-queue-notifications branch 5 times, most recently from 9eaf699 to 73f2140 Compare February 6, 2024 09:49
@pcapriotti pcapriotti force-pushed the wpb-183/versioned-queue-notifications branch 2 times, most recently from bdf5e88 to ad683dc Compare February 9, 2024 15:48
pcapriotti and others added 13 commits March 4, 2024 11:09
We cannot parse the supported versions returned by a remote federator
using our own `Version` type, because this breaks forward compatibility.
Instead, use integers and convert later, ignoring any version that
doesn't exist locally.
Old backends are not able to parse version lists containing newer
versions. This commit changes the JSON format of the response of the
`api-version` federation endpoint, and leaves a hardcoded value for the
legacy field that old backends are able to parse.

This means that version negotiation running within an old backend will
return a bogus result, but since those old backends were not actually
making use of federation API versioning, that is not a problem.
It was introduced to accommodate older backends that were already
claiming to support V1. However, now that the version negotiation system
is bypassing the old one, there is no need for the extra version bump.
Co-authored-by: Akshay Mankar <akshay@wire.com>
@pcapriotti pcapriotti force-pushed the wpb-183/versioned-queue-notifications branch 2 times, most recently from 4da4075 to c0ac4e4 Compare March 4, 2024 13:57
This prevents the background worker from getting stuck when a remote
backend is running an incompatible or broken instance.
@pcapriotti pcapriotti force-pushed the wpb-183/versioned-queue-notifications branch from c0ac4e4 to 83dcf79 Compare March 4, 2024 14:20
@pcapriotti pcapriotti requested a review from akshaymankar March 4, 2024 16:44
@pcapriotti
Copy link
Contributor

pcapriotti commented Mar 4, 2024

@akshaymankar I think I addressed all the issues. Let me know if I missed anything.

@mdimjasevic
Copy link
Contributor Author

mdimjasevic commented Mar 6, 2024

I approve this PR, but given that I started the PR, GitHub wouldn't let me approve it through its interface. @pcapriotti , can you approve it for me?

@pcapriotti pcapriotti merged commit 163eb29 into develop Mar 7, 2024
6 of 8 checks passed
@pcapriotti pcapriotti deleted the wpb-183/versioned-queue-notifications branch March 7, 2024 13:30
pcapriotti added a commit that referenced this pull request Mar 7, 2024
Co-authored-by: Matthias Fischmann <mf@zerobuzz.net>
Co-authored-by: Paolo Capriotti <paolo@capriotti.io>
Co-authored-by: Akshay Mankar <akshay@wire.com>
pcapriotti added a commit that referenced this pull request Mar 7, 2024
Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com>
Co-authored-by: Matthias Fischmann <mf@zerobuzz.net>
Co-authored-by: Akshay Mankar <akshay@wire.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants