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

Remove deprecated API AdvancedColumnFamilyOptions::soft_rate_limit #9451

Closed
wants to merge 1 commit into from

Conversation

hx235
Copy link
Contributor

@hx235 hx235 commented Jan 26, 2022

Context/Summary:
AdvancedColumnFamilyOptions::soft_rate_limit has been marked as deprecated since 4.4.0 (2016-01-14) and it's time to actually remove the code.

  • Keep soft_rate_limit in cf_mutable_options_type_info to prevent throwing InvalidArgument in GetColumnFamilyOptionsFromMap when reading an option file still with soft_rate_limit (e.g, old option file generated from RocksDB before this soft_rate_limit deprecation)
  • Keep soft_rate_limit in under OptionsOldApiTest to test the case mentioned above.

Test:
Rely on my eyeball and CI

@facebook-github-bot
Copy link
Contributor

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@hx235 hx235 force-pushed the rocksdb7.0_dep_soft_rate_limit branch from dc305df to 6be2b44 Compare January 26, 2022 23:09
@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@hx235 hx235 changed the title Remove deprecated API AdvancedColumnFamilyOptions::soft_rate_limit [TechDebt][1/3] Remove deprecated API AdvancedColumnFamilyOptions::soft_rate_limit Jan 26, 2022
@siying
Copy link
Contributor

siying commented Jan 26, 2022

A small comment: be aware that PR title will become commit title after it is merged. Commit title is permanent. When later RocksDB developers blame the code or try to find history, they look at the commit title. So write your PR title and description so that any RocksDB developer could understand it after 3 years or 5 years. In my opinion, "[TechDebt]" is not very helpful there, as well as "[1/3]".

@hx235
Copy link
Contributor Author

hx235 commented Jan 26, 2022

A small comment: be aware that PR title will become commit title after it is merged. Commit title is permanent. When later RocksDB developers blame the code or try to find history, they look at the commit title. So write your PR title and description so that any RocksDB developer could understand it after 3 years or 5 years. In my opinion, "[TechDebt]" is not very helpful there, as well as "[1/3]".

Good call - fixing it!

@hx235 hx235 changed the title [TechDebt][1/3] Remove deprecated API AdvancedColumnFamilyOptions::soft_rate_limit Remove deprecated API AdvancedColumnFamilyOptions::soft_rate_limit Jan 26, 2022
@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

LGTM

HISTORY.md Outdated
@@ -4,6 +4,7 @@
* Remove HDFS support from main repo.
* Remove librados support from main repo.
* Remove deprecated API DB::AddFile from main repo.
* Remove deprecated API AdvancedColumnFamilyOptions::soft_rate_limit
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for being the first to drop "from main repo".

Copy link
Contributor Author

@hx235 hx235 Jan 26, 2022

Choose a reason for hiding this comment

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

(time to see who merge it first #9454

@hx235 hx235 force-pushed the rocksdb7.0_dep_soft_rate_limit branch from 8ad8d99 to 76f011a Compare January 27, 2022 02:05
@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

@hx235 hx235 force-pushed the rocksdb7.0_dep_soft_rate_limit branch from 76f011a to adaf586 Compare January 27, 2022 06:27
@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

@hx235
Copy link
Contributor Author

hx235 commented Jan 27, 2022

Update:

  • Keep soft_rate_limit in cf_mutable_options_type_info to prevent throwing InvalidArgument in GetColumnFamilyOptionsFromMap when reading an option file still with soft_rate_limit (e.g, old option file generated from RocksDB before this soft_rate_limit deprecation)
  • Keep soft_rate_limit in under OptionsOldApiTest to test the case mentioned above.
  • Rebased

@facebook-github-bot
Copy link
Contributor

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@hx235
Copy link
Contributor Author

hx235 commented Jan 27, 2022

Close this to combine with #9452 for less rebasing efforts

@hx235 hx235 closed this Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants