search

facebook / rocksdb

A library that provides an embeddable, persistent key-value store for fast storage.
http://rocksdb.org
GNU General Public License v2.0
27.83k stars 6.2k forks source link

Clarify GetProperty API doc #12829

Closed hx235 closed 1 day ago

hx235 commented 2 days ago

Context/Summary: as titled since https://github.com/facebook/rocksdb/blob/9eebaf11cbd875435b572f05f0378ecdb761cc74/db/internal_stats.cc#L1162.

Test: no code change

facebook-github-bot commented 2 days ago

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

facebook-github-bot commented 1 day ago

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

hx235 commented 1 day ago

The language still says it returns true if the property name is valid, so some rephrasing is needed.

Ideally we would offer Status-returning variants of these functions so that users can distinguish the kind of failure--and we can better integrate into the crash test. That is a significant refactoring, so not expecting it here & now.

Btw, I don't think we should tell the users about this subtle change (with a release note) until we have the Status-returning variants. Otherwise, we would be asking for attention on these functions when the "good" solution is not available.

Thanks for keeping our contracts in good shape

Reworded again to be "If "property" is a valid "string" pr... and the DB is able to get and fill "*value" with its current value, then return true"

facebook-github-bot commented 1 day ago

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

facebook-github-bot commented 1 day ago

@hx235 merged this pull request in facebook/rocksdb@1f589a3f730c3013b6b80373471b0cd2ae8fde1a.