-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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 ObjectLibrary::Register() (and Regex public API) #9439
Conversation
Summary: Regexes are considered potentially problematic for use in registering RocksDB extensions, so we are removing ObjectLibrary::Register() and the Regex public API it depended on (now unused). Why? * The power of Regexes can make it hard to reason about which extension will match what. (The replacement API isn't perfect, but we are at least "holding the line" on patterns we have seen in practice.) * It is easy to make regexes that don't quite mean what you think they mean, such as forgetting that the . in "foo.bar" can match any character or that matching is nondeterministic, as in "a:b:42" matching ".*:[0-9]+". * Some regexes and implementations can have disastrously bad performance. This might not be much practical concern for ObjectLibray here, but we don't want to encourage potentially dangerous further use in production code. (Testing code is fine. See TestRegex.) Test Plan: CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
… API) (facebook#9439)" This reverts commit 449029f.
… API) (facebook#9439)" This reverts commit 449029f.
Summary: See facebook/rocksdb#9439 update-submodule: rocksdb Reviewed By: yizhang82 Differential Revision: D33832993 fbshipit-source-id: c3ac4682493
Summary: Regexes are considered potentially problematic for use in
registering RocksDB extensions, so we are removing
ObjectLibrary::Register() and the Regex public API it depended on (now
unused).
In reference to #9389
Why?
will match what. (The replacement API isn't perfect, but we are at least
"holding the line" on patterns we have seen in practice.)
mean, such as forgetting that the
.
infoo.bar
can match any characteror that matching is nondeterministic, as in
a:b:42
matching.*:[0-9]+
.performance. This might not be much practical concern for ObjectLibray
here, but we don't want to encourage potentially dangerous further use
in production code. (Testing code is fine. See TestRegex.)
Test Plan: CI