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

Disable fault injection for TestGetProperty #12825

Closed hx235 closed 3 days ago

hx235 commented 3 days ago

Context/Summary: See titled; along with one more minor fix to other disabling

Test: CI won't show Failed to get DB property: rocksdb.aggregated-table-properties

facebook-github-bot commented 3 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 3 days ago

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

facebook-github-bot commented 3 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 3 days ago

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

facebook-github-bot commented 3 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 3 days ago

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

facebook-github-bot commented 3 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 3 days ago

@hx235 merged this pull request in facebook/rocksdb@69ad597b4600d17085672d93993ff9e538ed4663.

pdillinger commented 2 days ago

I believe the crash test failure revealed an API problem: @hx235

// If "property" is a valid "string" property understood by this DB // implementation (see Properties struct above for valid options), fills // "*value" with its current value and returns true. Otherwise, returns // false.

It doesn't say the function can return false because of IO failures. So IMHO this PR is just suppressing a failure, not a fix to the underlying problem. From the PR description, it wasn't clear if that was known / intended.

hx235 commented 2 days ago

I believe the crash test failure revealed an API problem: @hx235

// If "property" is a valid "string" property understood by this DB // implementation (see Properties struct above for valid options), fills // "*value" with its current value and returns true. Otherwise, returns // false.

It doesn't say the function can return false because of IO failures. So IMHO this PR is just suppressing a failure, not a fix to the underlying problem. From the PR description, it wasn't clear if that was known / intended.

I see .. let me fix the API doc too.