Add plain_table_db_test to ASSERT_STATUS_CHECKED list #7482

zhichao-cao · 2020-09-30T23:19:59Z

summary: Add plain_table_db_test to ASSERT_STATUS_CHECKED list
test plan: ASSERT_STATUS_CHECKED=1 make -j48 plain_table_db_test

facebook-github-bot

@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

riversand963 · 2020-09-30T23:26:08Z

db/db_iter.h

@@ -132,6 +132,7 @@ class DBIter final : public Iterator {
    ResetInternalKeysSkippedCounter();
    local_stats_.BumpGlobalStatistics(statistics_);
    iter_.DeleteIter(arena_mode_);
+    status_.PermitUncheckedError();


If we call status_.PermitUncheckedError() here, we won't benefit from the enforcement of status check, right?

Yes. But status_ is mostly used as a "carrier" of the status and return it to the user, when status() is called. So, it is reasonable to call PermitUncheckedError() in the destructor

iiuc, if it's not returned to the user, adding PermitUncheckedError() will bypass the check. I am not sure if this is what we would like to achieve.

iiuc, if it's not returned to the user, adding PermitUncheckedError() will bypass the check. I am not sure if this is what we would like to achieve.

It is a little problematic. We have some classes that use Status or IOStatus as member. In most case, they are just used to carry the status through the life time of the class instance. And usually, user call a function to get the status when they want. In those cases, we are not able to know if user get the status or not.

I agree with @riversand963 that, this thing will defeat the whole purpose of the status check for iterators.
I would rather fix those unit tests than do it here.

If you want to remove the PermitUncheckedErrors in destructors, you need a means of telling if the Status was ever checked or set. I see two potential solutions to this. One is that an "empty Status" (e.g. Status s;) always passes the check. Another is that Status::OK() always passes. If choosing the latter, you would need to explicitly initialize the Status to OK and to change OK to be a static Status (rather than one created every time).

I think there are likely going to be places where an Iterator is created and the status is never set that are causing some of the assertions, and those are very hard to track down.

table/table_factory.cc

db/plain_table_db_test.cc

db/table_cache.cc

table/table_factory.cc

facebook-github-bot · 2020-10-01T17:54:55Z

@zhichao-cao has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying · 2020-10-02T00:35:14Z

db/db_iter.h

@@ -132,6 +132,7 @@ class DBIter final : public Iterator {
    ResetInternalKeysSkippedCounter();
    local_stats_.BumpGlobalStatistics(statistics_);
    iter_.DeleteIter(arena_mode_);
+    status_.PermitUncheckedError();


I agree with @riversand963 that, this thing will defeat the whole purpose of the status check for iterators.
I would rather fix those unit tests than do it here.

facebook-github-bot · 2020-10-02T17:55:54Z

@zhichao-cao has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-10-02T17:55:54Z

@zhichao-cao has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mrambacher · 2020-10-02T18:24:52Z

table/table_factory.cc

@@ -37,9 +37,10 @@ Status TableFactory::CreateFromString(const ConfigOptions& config_options_in,
    factory->reset(new CuckooTableFactory());
 #endif  // ROCKSDB_LITE
  } else {
-    return Status::NotSupported("Could not load table factory: ", name);
+    status = Status::NotSupported("Could not load table factory: ", name);
+    return status;


I do not think you need to return here with the line 43 change...

facebook-github-bot · 2020-10-02T20:51:17Z

@zhichao-cao has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

riversand963

LGTM.

facebook-github-bot · 2020-10-07T05:14:34Z

@zhichao-cao has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying · 2020-10-12T19:16:37Z

My comment of DBIter has been addressed. Thanks!

facebook-github-bot · 2020-10-12T19:22:37Z

@zhichao-cao has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-10-13T20:43:50Z

@zhichao-cao merged this pull request in 16bff53.

Summary: Add plain_table_db_test to ASSERT_STATUS_CHECKED list Pull Request resolved: facebook#7482 Test Plan: ASSERT_STATUS_CHECKED=1 make -j48 plain_table_db_test Reviewed By: riversand963 Differential Revision: D24034987 Pulled By: zhichao-cao fbshipit-source-id: e61c937d55ded0947cc8936937362dafed572a60

zhichao-cao requested review from mrambacher and riversand963 September 30, 2020 23:19

facebook-github-bot added the CLA Signed label Sep 30, 2020

facebook-github-bot reviewed Sep 30, 2020

View reviewed changes

riversand963 reviewed Sep 30, 2020

View reviewed changes

mrambacher reviewed Sep 30, 2020

View reviewed changes

db/plain_table_db_test.cc Show resolved Hide resolved

db/table_cache.cc Outdated Show resolved Hide resolved

table/table_factory.cc Outdated Show resolved Hide resolved

zhichao-cao force-pushed the status_plain branch from 175c4a1 to f1691d1 Compare October 1, 2020 17:54

facebook-github-bot reviewed Oct 1, 2020

View reviewed changes

siying suggested changes Oct 2, 2020

View reviewed changes

zhichao-cao force-pushed the status_plain branch from f1691d1 to 4f029b0 Compare October 2, 2020 17:55

facebook-github-bot reviewed Oct 2, 2020

View reviewed changes

mrambacher reviewed Oct 2, 2020

View reviewed changes

zhichao-cao force-pushed the status_plain branch from 4f029b0 to aab35ea Compare October 2, 2020 20:51

facebook-github-bot reviewed Oct 2, 2020

View reviewed changes

riversand963 approved these changes Oct 2, 2020

View reviewed changes

zhichao-cao force-pushed the status_plain branch from aab35ea to 133ddef Compare October 7, 2020 05:14

facebook-github-bot reviewed Oct 7, 2020

View reviewed changes

Add plain_table_db_test to ASSERT_STATUS_CHECKED list

893d519

zhichao-cao force-pushed the status_plain branch from 133ddef to 893d519 Compare October 12, 2020 19:22

facebook-github-bot reviewed Oct 12, 2020

View reviewed changes

facebook-github-bot closed this in 16bff53 Oct 13, 2020

facebook-github-bot added the Merged label Oct 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add plain_table_db_test to ASSERT_STATUS_CHECKED list #7482

Add plain_table_db_test to ASSERT_STATUS_CHECKED list #7482

zhichao-cao commented Sep 30, 2020

facebook-github-bot left a comment

riversand963 Sep 30, 2020

zhichao-cao Oct 1, 2020

riversand963 Oct 1, 2020

zhichao-cao Oct 1, 2020

siying Oct 2, 2020

mrambacher Oct 2, 2020

facebook-github-bot commented Oct 1, 2020

facebook-github-bot left a comment

siying Oct 2, 2020

facebook-github-bot commented Oct 2, 2020

facebook-github-bot commented Oct 2, 2020

facebook-github-bot left a comment

mrambacher Oct 2, 2020

facebook-github-bot commented Oct 2, 2020

facebook-github-bot left a comment

riversand963 left a comment

facebook-github-bot commented Oct 7, 2020

facebook-github-bot left a comment

siying commented Oct 12, 2020

facebook-github-bot commented Oct 12, 2020

facebook-github-bot left a comment

facebook-github-bot commented Oct 13, 2020

Add plain_table_db_test to ASSERT_STATUS_CHECKED list #7482

Add plain_table_db_test to ASSERT_STATUS_CHECKED list #7482

Conversation

zhichao-cao commented Sep 30, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 1, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 2, 2020

facebook-github-bot commented Oct 2, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 2, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

riversand963 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 7, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying commented Oct 12, 2020

facebook-github-bot commented Oct 12, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 13, 2020