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: add topic sequence_page #298

Merged
merged 3 commits into from
Oct 7, 2024
Merged

chore: add topic sequence_page #298

merged 3 commits into from
Oct 7, 2024

Conversation

kvcache
Copy link
Collaborator

@kvcache kvcache commented Sep 30, 2024

Add a sequence_page field to topics. This will be in the discontinuities, the subscribe message, and the items next to the sequence number.

PDXKimani
PDXKimani previously approved these changes Sep 30, 2024
// `resume_at_topic_sequence_number`.
// Include this in your Subscribe() calls when you are reconnecting. The right value
// is the last sequence_stamp you received.
uint64 sequence_stamp = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: topic_sequence_stamp so that the field naming is similar to topic_sequence_number.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't really like the idea of mixing the names too closely. Stamp versus number being the only discriminator between the two seems confusing, particularly to someone with English as a second, third, or fourth language. It's already subtle enough haha

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I did have that same thought. If anything I would rather change sequence to something else instead of adding topic to it.

// If you didn't have one, then this is just telling you what the sequence stamp is expected to be.
// If you had one before, and this one is the same, then it's just telling you that you missed some messages
// in the topic. Probably your client is consuming messages a little too slowly in this case!
uint64 new_sequence_stamp = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: new_topic_sequence_stamp

Copy link
Contributor

@malandis malandis left a comment

Choose a reason for hiding this comment

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

Few clarifying questions on first read, thinking about client user experience

proto/cachepubsub.proto Show resolved Hide resolved
proto/cachepubsub.proto Outdated Show resolved Hide resolved
proto/cachepubsub.proto Outdated Show resolved Hide resolved
malandis
malandis previously approved these changes Sep 30, 2024
@kvcache kvcache dismissed stale reviews from malandis and PDXKimani via f5a8b96 October 1, 2024 00:39
@kvcache kvcache changed the title chore: add topic sequence_stamp chore: add topic sequence_page Oct 1, 2024
@kvcache kvcache requested a review from malandis October 1, 2024 00:39
@kvcache
Copy link
Collaborator Author

kvcache commented Oct 1, 2024

@malandis FYI I switched the name here after some discussion. Let me know what you think!

anp13
anp13 previously approved these changes Oct 1, 2024
Copy link
Contributor

@krispraws krispraws left a comment

Choose a reason for hiding this comment

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

LGTM. page doesn't quite feel 100% right to me but I don't have a better word to describe a time slice for a topic

// * to provide a best-effort awareness of stream contiguity, or lack thereof,
// in case you need to know.
// You can safely ignore them if none of that matters to you!
// Topic sequence numbers give an order of messages per-topic.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: They give an order per-topic-sequence-page.

@danielamiao
Copy link

LGTM. page doesn't quite feel 100% right to me but I don't have a better word to describe a time slice for a topic

I don't feel strongly about this either but just curious why switch from stamp to page? I liked stamp better actually, it gave me the right semantics about what it represents

@krispraws
Copy link
Contributor

LGTM. page doesn't quite feel 100% right to me but I don't have a better word to describe a time slice for a topic

I don't feel strongly about this either but just curious why switch from stamp to page? I liked stamp better actually, it gave me the right semantics about what it represents

I am the opposite - stamp did not convey any meaning to me. instance, version, page came closer.

@kvcache
Copy link
Collaborator Author

kvcache commented Oct 2, 2024

LGTM. page doesn't quite feel 100% right to me but I don't have a better word to describe a time slice for a topic

I don't feel strongly about this either but just curious why switch from stamp to page? I liked stamp better actually, it gave me the right semantics about what it represents

I am the opposite - stamp did not convey any meaning to me. instance, version, page came closer.

Here's my reasoning for how I got here:

It is impossible to please everyone with this name. Stamps are abstract and have no sense of meaning or order, nor even a notion of change. Page evokes a sense of order and continuity, with a clear signal that "pages turn."
It's not a perfect word, but even the perfect word, "epoch," is not universally liked and itself fails to capture the nonfunctionals. It's a more heady word, while the simplicity of "page" captures the needful. Remember, people are going to be interacting with this via http, so this is an api discussion.

@kvcache kvcache merged commit b8b17fe into main Oct 7, 2024
7 checks passed
@kvcache kvcache deleted the stamp branch October 7, 2024 18:55
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.

6 participants