etcd-io / raft

Raft library for maintaining a replicated state machine
Apache License 2.0
666 stars 164 forks source link

deprecate `RawNode.TickQuiesced()` #62

Closed erikgrinaker closed 1 year ago

erikgrinaker commented 1 year ago

TickQuiesced() was originally added for use with range quiescence in CockroachDB, but hasn't been in use since 2018. This patch marks it as deprecated, to be removed in a future release.

See https://github.com/cockroachdb/cockroach/pull/24956.

erikgrinaker commented 1 year ago

cc @tbg @pavelkalinnikov

pav-kv commented 1 year ago

@erikgrinaker Please add a Signed-off-by footer: git commit --amend --signoff.

@tbg Please approve/merge, as I can't.

spzala commented 1 year ago

@erikgrinaker thanks, and please take care of the DCO.

tbg commented 1 year ago

This technically breaks drop-in backwards compatibility. I still think we should remove this, as it's quite crufty. I used Github code search and there are 200+ hits, but as far as I can tell they're all in

Would appreciate some advice from the other maintainers. Personally, I think the benefit of removing this far outweigh the risks (in fact, I'd be concerned about someone using this).

cc @ahrtr @spzala

spzala commented 1 year ago

This technically breaks drop-in backwards compatibility. I still think we should remove this, as it's quite crufty. I used Github code search and there are 200+ hits, but as far as I can tell they're all in

* old forks of cockroach repos

* forks of etcd/raft

* some possibly machine-written versions of etcd/raft in, for example, D and Rust (exact matching comments).

Would appreciate some advice from the other maintainers. Personally, I think the benefit of removing this far outweigh the risks (in fact, I'd be concerned about someone using this).

cc @ahrtr @spzala

Thanks for the investigation @tbg I think we should at least mention this in the Changelog. This repo is still new so seems like we don't have the Changelog file created but it's time to do so now :) cc @ahrtr

ahrtr commented 1 year ago

If we decide to remove a public API, we need to add a breaking change notice at least. Creating a changelog (thx @spzala for the reminder) is a good idea.

I am also thinking to create a milestone as a goal to release next raft version, e.g. 3.6.0. Detailed proposals:

erikgrinaker commented 1 year ago

Thanks for the code search @tbg! I'd be surprised and concerned if someone was using this.

I don't know what our guarantees are around backwards compatibility, but it makes sense to leave this for either 3.6 or 4.0. This is not a priority at all, just an opportunistic cleanup. A milestone and changelog sounds good, let me know if we can help set that up.

please take care of the DCO

ahrtr commented 1 year ago

Usually a best practice is to deprecate a public API firstly (e.g in 3.6) to discourage users from using it, and then remove it in a major release (e.g. 4.0). So two options:

erikgrinaker commented 1 year ago

Sure, updated the PR to deprecate it and opened #66 to track the removal in the next major release.

erikgrinaker commented 1 year ago

TFTRs! Maybe we should add a 4.0 milestone too, and track #66 in it?

ahrtr commented 1 year ago

TFTRs! Maybe we should add a 4.0 milestone too, and track #66 in it?

Done.