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

validator: Cleanup deprecated and dead AccountsDb arguments #4958

Merged

Conversation

steviez
Copy link

@steviez steviez commented Feb 12, 2025

Summary of Changes

Remove deprecated AccountsDb args from the deprecated list to reduce clutter.

I think the first two are obviously fair game to rip out. I also see the third one as valid to pull out since this change will first land in v2.3 which will have given full v2.1 + full v2.2 cycle for validators to adapt. But, I'm open to opinions on this. EDIT: We'll keep --accounts-index-memory-limit-mb in place until v3.0

@steviez steviez force-pushed the validator_rm_deprecated_accounts_db_args branch from 40d093d to 1ddfe0b Compare February 12, 2025 20:58
@steviez
Copy link
Author

steviez commented Feb 12, 2025

Added @brooksprumo & @HaoranYi as subject matter experts on AccountsDb; added @willhickey to weigh in on the more process side of it

brooksprumo
brooksprumo previously approved these changes Feb 13, 2025
Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Thanks for the 🧹

HaoranYi
HaoranYi previously approved these changes Feb 13, 2025
Copy link

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Thanks!

@willhickey
Copy link

willhickey commented Feb 13, 2025

I think the first two are obviously fair game to rip out. I also see the third one as valid to pull out since this change will first land in v2.3 which will have given full v2.1 + full v2.2 cycle for validators to adapt. But, I'm open to opinions on this.

As I understand our policy we can remove deprecated features that were deprecated:

  1. In a earlier major version; and
  2. At least 1 full minor cycle earlier

So #2 would prevent this:

Deprcate in: 2.<last minor release>
Remove in:   3.0

In that case the removal would have to wait until 3.1

I'm not sure if this policy is written down anywhere 👀

@willhickey
Copy link

The policy we've published is a little different from what I said above, but it still prohibits removing features in the same major release that they were deprecated.
https://docs.anza.xyz/backwards-compatibility#deprecation-process

@steviez steviez dismissed stale reviews from HaoranYi and brooksprumo via c21db07 February 13, 2025 19:43
@steviez
Copy link
Author

steviez commented Feb 13, 2025

As I understand our policy we can remove deprecated features that were deprecated:

  1. In a earlier major version; and
  2. At least 1 full minor cycle earlier

So item 2 would prevent this:

Deprcate in: 2.<last minor release>
Remove in:   3.0

In that case the removal would have to wait until 3.1

I'm a little confused by your example. Using concrete numbers, let's suppose that v2.3 will be the last minor on the v2.2 line. Suppose I deprecate some argument in tip of master today. That deprecation will land in v2.3.0 and all subsequent v2.3 tags when we cut those. In that case, the deprecation warning would ride for the entire v2.3 cycle. And then in my mind, it would be fair game to rip this out in v3.0. I'm not sure I follow why we should / would have to wait until v3.1 ...

@steviez
Copy link
Author

steviez commented Feb 13, 2025

In any case, these deprecated items are in a fairly narrow corner of the codebase, so I'm cool with letting the one that was deprecated in v2.1 to live until we cut v3.0. I added a 4th commit that reverts the 3rd commit

@steviez steviez merged commit 9d6ca21 into anza-xyz:master Feb 14, 2025
47 checks passed
@steviez steviez deleted the validator_rm_deprecated_accounts_db_args branch February 14, 2025 05:32
@willhickey
Copy link

willhickey commented Feb 14, 2025

I'm a little confused by your example. Using concrete numbers, let's suppose that v2.3 will be the last minor on the v2.2 line. Suppose I deprecate some argument in tip of master today. That deprecation will land in v2.3.0 and all subsequent v2.3 tags when we cut those. In that case, the deprecation warning would ride for the entire v2.3 cycle. And then in my mind, it would be fair game to rip this out in v3.0. I'm not sure I follow why we should / would have to wait until v3.1 ...

The spirit of rule 2 is to introduce a minimum amount of time between deprecation and removal,. This is probably the most detailed conversation that has happened on this topic, so there are likely multiple interpretations of what "at least 1 minor cycle" means. I'm comfortable relaxing my interpretation from above. How about this:

We can remove deprecated features that were deprecated:

  1. In a earlier major version; and
  2. In all tagged releases of the previous minor version

Continuing your concrete example, something merged in master before v2.3.0 is branched/tagged would be eligible for removal in v3.0, but a deprecation that's backported to v2.3 after v2.3.0 wouldn't be eligible until v3.1. In general we should avoid backporting deprecations anyway, but I think the rule is warranted to make it clear that we will never:

Deprecate in v2.3.<last patch> and then remove in v3.0

If those rules sound reasonable I'll update https://docs.anza.xyz/backwards-compatibility#deprecation-process

@steviez
Copy link
Author

steviez commented Feb 14, 2025

Continuing your concrete example, something merged in master before v2.3.0 is branched/tagged would be eligible for removal in v3.0, but a deprecation that's backported to v2.3 after v2.3.0 wouldn't be eligible until v3.1. In general we should avoid backporting deprecations anyway, but I think the rule is warranted to make it clear that we will never:

Ahh ok, I see; you're trying to protect against someone slipping something in at the last minute for the sake of checking of a box. Yeah, that seems reasonable. If you want to update the policy, a PR is a good way to facilitate conversation and we can continue it there & get others involved

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.

4 participants