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

Add stop position to SubscribeRequest #44

Merged
merged 1 commit into from
Oct 28, 2020

Conversation

alexrudd
Copy link
Contributor

@alexrudd alexrudd commented Oct 28, 2020

Hey, this is related to liftbridge-io/liftbridge#282 and liftbridge-io/liftbridge#280

This wasn't as simple as I'd hoped:

protoc --gofast_out=plugins=grpc:go api.proto
api.proto:93:5: "OFFSET" is already defined in "proto".
api.proto:93:5: Note that enum values use C++ scoping rules, meaning that enum values are siblings of their type, not children of it.  Therefore, "OFFSET" must be unique within "proto", not just within "StopPosition".

to get this to compile I've added STOP_ prefix to all the StopPosition enums. Happy to accept alternatives.

I couldn't see any reference to preferred protoc tool versions, so I just installed all the latest. Think this has resulted to some generated code changes that are not related to the proto definition changes.

@tylertreat
Copy link
Member

Bummer on the enum name conflict. I suppose we could change the StartPosition values to have a START_ prefix for consistency since I believe proto field names can be safely changed.

@tylertreat tylertreat merged commit 67420de into liftbridge-io:master Oct 28, 2020
@alexrudd
Copy link
Contributor Author

alexrudd commented Oct 28, 2020

@tylertreat I just started implementing against this and realised there's a problem. There's no reliable way of checking if a stop position has not been set in a request, as the default StopPosition value is STOP_OFFSET and default StopOffset value is 0. So any request where the stop position isn't set looks identical to a request to stop after reading only the first message.

The simplest way I can think of fixing this is to change the 0 enum value to the current subscribe behaviour:

// StopPosition determines the stop-position type on a subscription.
enum StopPosition {
    STOP_ON_CANCEL = 0; // Stop when the request is cancelled
    STOP_OFFSET    = 1; // Stop at a specified offset
    STOP_LATEST    = 2; // Stop at the latest offset
    STOP_TIMESTAMP = 3; // Stop at a specified timestamp
}

Thoughts?

@tylertreat
Copy link
Member

@alexrudd Yeah, that makes sense to me.

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