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

Checkpointer interface breaking change in v1.3.0 #102

Open
bjwrk opened this issue May 16, 2022 · 1 comment
Open

Checkpointer interface breaking change in v1.3.0 #102

bjwrk opened this issue May 16, 2022 · 1 comment
Labels
bug Something isn't working

Comments

@bjwrk
Copy link

bjwrk commented May 16, 2022

Describe the bug

The Checkpointer interface was updated in v1.3.0 to include the following two methods:

ListActiveWorkers(map[string]*par.ShardStatus) (map[string][]*par.ShardStatus, error)
ClaimShard(*par.ShardStatus, string) error

We have a custom Postgres Checkpointer implementation and the minor version bump broke our build. We believe this change should either have been an optional interface, i.e. ShardStealingCheckpointer with only the two new methods, or a major version bump. If the optional interface would be an acceptable solution, we are happy to contribute a PR.

As far as I can tell, it's sufficient for now for us to simply return a "not implemented" error as stealShard will never be true unless EnableLeaseStealing is also true, and we are not currently using this feature, but I just wanted to confirm that this was something we could expect.

As an aside, I notice there is already a DynamoDB implementation in the repo. We would be happy to flesh out the missing methods and contribute our Postgres Checkpointer in a PR at some point if that would be something you'd see value in.

Reproduction steps

Implement the Checkpointer interface prior to v1.3.0, update to v1.3.0.

Expected behavior

Minor version bumps should not break APIs.

Additional context

No response

@bjwrk bjwrk added the bug Something isn't working label May 16, 2022
@taoj-action
Copy link
Contributor

Please flesh out the missing methods and contribute Postgres Checkpointer in a PR. Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants