search

liftbridge-io / go-liftbridge

Go client for Liftbridge. https://github.com/liftbridge-io/liftbridge
Apache License 2.0
68 stars 18 forks source link

Potential resubscription bug. #69

Closed ruseinov closed 3 years ago

ruseinov commented 4 years ago

I've been looking through the client code while implementing error handling in subscriptions for the Rust client and stumbled upon this: https://github.com/liftbridge-io/go-liftbridge/blob/f9c0b883e5343c343bc0722e2ff3652665375393/client.go#L1259 .

What happens if a client gets an error response on the first Recv? After re-subscription we would have incremented the offset by 1.

The other scenario is when the client subscribes with subscriptionOptions to a particular offset, in that case instead of starting with that offset on re-subscription we would start with 1.

Both of those scenarios would result in somewhat unexpected behaviour for the client. I'm in the process of making a decision about handling this in Rust client too. The one option I could think of is storing SubscriptionOptions to be able to resubscribe at a specified offset and starting with a -1 initial offset to be able to distinguish whether we managed to receive at least one message or not.

tylertreat commented 4 years ago

Yeah, in fact you're not the first to notice this issue: https://github.com/liftbridge-io/go-liftbridge/issues/51 :)

I think your suggested approach sounds like a reasonable alternative.

ruseinov commented 4 years ago

@tylertreat this has been implemented on the rust driver/client side already. I might take this on unless you want to do it.

tylertreat commented 4 years ago

Happy to accept PRs.

stephane-moreau commented 3 years ago

I encountered a similar issue a few days ago, after a couple of months of run for a proccess subscribed to various partitions of a stream a connection loss induced a reconnection to partition 0 instead of keeping the initial partition it was subscribed too.

Initially tool was using v1 of the liftbridge client, I switched to v2 and will submit a PR for a fix proposal

tylertreat commented 3 years ago

Should be fixed with #110.