Skip to content

Do not fail SUBSCRIBEs that start in the past #652

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

Merged
merged 9 commits into from
Feb 3, 2025

Conversation

ianswett
Copy link
Collaborator

Fixes #595

It's difficult or possibly even impossible to prevent it.

Fixes #595 

It's difficult or possibly even impossible to prevent it.
@ianswett ianswett added the Subscribe Related to SUBSCRIBE message and subscription handling label Jan 20, 2025
@ianswett ianswett requested a review from afrind January 20, 2025 18:50
@wilaw
Copy link
Contributor

wilaw commented Jan 21, 2025

Contrary to the title, this PR is not allowing a SUBSCRIBE to start in the past. It is taking a SUBSCRIBE that wants to start in the past, and coercing it to instead start at the beginning of the current group?

@ianswett ianswett changed the title Allow SUBSCRIBE to start in the past Do not fail SUBSCRIBEs that start in the past Jan 21, 2025
@ianswett
Copy link
Collaborator Author

Contrary to the title, this PR is not allowing a SUBSCRIBE to start in the past. It is taking a SUBSCRIBE that wants to start in the past, and coercing it to instead start at the beginning of the current group?

Good point will, I retitled this PR.

Comment on lines 1251 to 1252
StartGroup is prior to the current group, the subscription starts at the
beginning of the current group.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Individual Comment:

I'd say if the specified start point is prior to the latest seen object, the subscription starts at the next object .

Copy link
Collaborator

@suhasHere suhasHere Jan 22, 2025

Choose a reason for hiding this comment

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

+1 with @afrind with minor tweaks .. "If the specified StartGroup is prior to the latest seen object, then the subscriptions starts at the latest group and object seen at the publisher ( as if the filter was Latest Object)"

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 mention the Latest Object filter now. I think it might be worth filing an issue for exactly what Object is delivered first for the Latest Object filter.

Copy link
Collaborator

@suhasHere suhasHere left a comment

Choose a reason for hiding this comment

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

few minor suggestions to clarify.

@martinduke martinduke assigned ianswett and unassigned ianswett and martinduke Jan 29, 2025
@ianswett
Copy link
Collaborator Author

ianswett commented Jan 30, 2025

I made some minor updates, but I think a larger change is likely required to clarify what Objects one expects to receive. #592

Copy link
Collaborator

@englishm englishm left a comment

Choose a reason for hiding this comment

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

This seems like a useful incremental change to get to where we want to with clarifying Subscribe and Fetch. LGTM

@ianswett ianswett merged commit ff5a182 into main Feb 3, 2025
2 checks passed
@englishm englishm mentioned this pull request Feb 25, 2025
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Subscribe Related to SUBSCRIBE message and subscription handling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is a SUBSCRIBE_UPDATE with Start Group in the past an error?
6 participants