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

feat: support topic sequence page #540

Merged
merged 6 commits into from
Nov 4, 2024
Merged

Conversation

malandis
Copy link
Contributor

@malandis malandis commented Oct 7, 2024

Overview

This PR updates the protos selectively to include the topic sequence page changes from this PR on the client protos repo. In the future we will update to the released protos.

There are two changes: the topic subscription logic and the topic subscribe logic.

Topic Subscription changes

Updates the topic subscription to take the sequence page into account:

  • Keeps track of last known sequence page in addition to sequence number so we can reconnect gracefully
  • Includes sequence page detail for topic item and discontinuity objects

Topic Subscribe changes

Previously the topic client Subscribe method took a TopicSubscription as input but ignored the resume at sequence number. While the sequence number was used internally on reconnects and not expected to be used externally, we feel it could be confusing if a user sets the sequence number and we ignore it.

Since we also add the sequence page, we also pass that to the subscribe RPC.

To test/confirm once service is live

  • Suppose we subscribe to a long-running topic and the sequence page is not zero. Will the first message we receive still be a heartbeat, or a discontinuity?
    • heartbeat, then discontinuity
  • Test re-subscription on a sequence page change works

Updates the topic subscription to take the sequence page into
account:
- Keeps track of last known sequence page in addition to sequence
number so we can reconnect gracefully
- Includes sequence page detail for topic item and discontinuity
objects
Previously a user could have set the sequence number in a subscribe
request and it would have been ignored. We pass it along here though
provide no guidance on how to set it; ie users should leave it unset,
but when they do set it, we should respect it.
@malandis malandis requested review from anitarua and a team October 7, 2024 23:30
@malandis
Copy link
Contributor Author

malandis commented Oct 7, 2024

Opening for early review. Once the service is live, I will update the protos and rebase.

@malandis malandis requested a review from anitarua October 9, 2024 20:47
@malandis malandis marked this pull request as ready for review November 4, 2024 22:27
@malandis malandis requested a review from a team November 4, 2024 22:27
@malandis
Copy link
Contributor Author

malandis commented Nov 4, 2024

Ready for final review.

Copy link
Contributor

@anitarua anitarua left a comment

Choose a reason for hiding this comment

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

lgtm!

@malandis malandis merged commit cae3b3d into main Nov 4, 2024
6 checks passed
@malandis malandis deleted the feat/topic-sequence-page branch November 4, 2024 22:52
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.

2 participants