search

liftbridge-io / liftbridge-api

Protobuf definitions for the Liftbridge gRPC API. https://github.com/liftbridge-io/liftbridge
Apache License 2.0
15 stars 13 forks source link

Add stop position to SubscribeRequest #44

Closed alexrudd closed 4 years ago

alexrudd commented 4 years ago

Hey, this is related to https://github.com/liftbridge-io/liftbridge/pull/282 and https://github.com/liftbridge-io/liftbridge/pull/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 commented 4 years ago

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.

alexrudd commented 4 years ago

@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 commented 4 years ago

@alexrudd Yeah, that makes sense to me.