Add tests in ASSERT_STATUS_CHECKED #7793

akankshamahajan15 · 2020-12-21T18:50:16Z

Summary: add io_tracer_parser_test and prefetch_test under
ASSERT_STATUS_CHECKED

Test Plan: ASSERT_STATUS_CHECKED=1 make check -j64

Reviewers:

Subscribers:

Tasks:

Tags:

mrambacher · 2020-12-21T22:48:47Z

trace_replay/io_tracer.cc

@@ -216,13 +216,21 @@ void IOTracer::EndIOTrace() {
 }

 Status IOTracer::WriteIOOp(const IOTraceRecord& record) {
+  Status s;


I am not sure I understand the need for any of the changes in this method. This method looks like it should have passed the ASC without these changes. Can you revert them and tell me what test is failing?

io_tracer_parser_test fails because currently, I am ignoring the status of IOTracer::WriteIOOp in file_system_tracer.cc. I am planning to either log or return that status but haven't figured it out what to do with status. I added TODO as well. Planning to work on this in next half with few more additions related to io_tracer.

interesting. I wonder if this approach relies on return-value optimization. I'd expect if a different Status were constructed in the client with the result, that one would be unchecked.

Confirmed that is the case. This is likely not the only test that is optimization-dependent (I've run DEBUG_LEVEL=2 ASSERT_STATUS_CHECKED=1 before and seen other failures), but it'd be good to fix still. Can we call PermitUncheckedError() in the clients instead of inside the function returning the Status?

$ make clean && ASSERT_STATUS_CHECKED=1 EXTRA_CXXFLAGS="-fno-elide-constructors" make -j48 io_tracer_parser_test $ ./io_tracer_parser_test [==========] Running 4 tests from 1 test case. [2/1853] [----------] Global test environment set-up. [----------] 4 tests from IOTracerParserTest [ RUN ] IOTracerParserTest.InvalidArguments Failed to check Status 0x7ffe392aa5b0 #0 ./io_tracer_parser_test() [0x69fc3f] rocksdb::port::PrintStack(int) /data/users/andrewkr/rocksdb/port/stack_trace.cc:121 #1 ./io_tracer_parser_test() [0x470a39] rocksdb::Status::~Status() /data/users/andrewkr/rocksdb/./include/rocksdb/status.h:42 #2 ./io_tracer_parser_test() [0x510135] rocksdb::SanitizeOptions(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::DBOptions const&) /data/users/andrewkr/rocksdb/db/db_impl/db_impl_open.cc:166 (discriminator 1) #3 ./io_tracer_parser_test() [0x4c4392] rocksdb::DBImpl::DBImpl(rocksdb::DBOptions const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, bool) /data/users/andrewkr/rocksdb/db/db_impl/db_impl.cc:153 #4 ./io_tracer_parser_test() [0x512b0e] rocksdb::DBImpl::Open(rocksdb::DBOptions const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<rocksdb::ColumnFamilyDescriptor, std::allocator<rocksdb::ColumnFamilyDescriptor> > const&, std::vector<ro ksdb::ColumnFamilyHandle*, std::allocator<rocksdb::ColumnFamilyHandle*> >*, rocksdb::DB**, bool, bool) /data/users/andrewkr/rocksdb/db/db_impl/db_impl_open.cc:1519 #5 ./io_tracer_parser_test() [0x514a98] rocksdb::DB::Open(rocksdb::DBOptions const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<rocksdb::ColumnFamilyDescriptor, std::allocator<rocksdb::ColumnFamilyDescriptor> > const&, std::vector<rocksd ::ColumnFamilyHandle*, std::allocator<rocksdb::ColumnFamilyHandle*> >*, rocksdb::DB**) /data/users/andrewkr/rocksdb/db/db_impl/db_impl_open.cc:1450 #6 ./io_tracer_parser_test() [0x514c98] rocksdb::DB::Open(rocksdb::Options const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::DB**) /data/users/andrewkr/rocksdb/db/db_impl/db_impl_open.cc:1428 #7 ./io_tracer_parser_test() [0x4746a7] rocksdb::IOTracerParserTest::IOTracerParserTest() /data/users/andrewkr/rocksdb/tools/io_tracer_parser_test.cc:42 #8 ./io_tracer_parser_test() [0x47599b] rocksdb::IOTracerParserTest_InvalidArguments_Test::IOTracerParserTest_InvalidArguments_Test() /data/users/andrewkr/rocksdb/tools/io_tracer_parser_test.cc:108 #9 ./io_tracer_parser_test() [0x4a4e3d] testing::Test* testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::TestFactoryBase, testing::Test*>(testing::internal::TestFactoryBase*, testing::Test* (testing::internal::TestFactoryBase::*)(), char const*) /data/users/andrewkr/rocksdb/third-party/gtest-1.8.1/fused-src/gtest/gtest-all.cc:3899 ...

Actually the above failure I showed is indeed a different optimization-dependent Status check. I guess there is a whole project to be done to get rid of optimization dependencies. Please feel free to go ahead then and not block on this.

@ajkr I will be submitting a separate PR shortly that addresses the "-fno-elide-constructors".

jay-zhuang

LGTM.

jay-zhuang · 2020-12-21T23:11:53Z

Makefile

 	        io_tracer_test \
-		merge_helper_test \
+		io_tracer_parser_test \
+		prefetch_test \
+	        merge_helper_test \


nit: change the spaces to tabs

https://giphy.com/gifs/producthunt-silicon-valley-spaces-tabs-l0IylSajlbPRFxH8Y

facebook-github-bot

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

facebook-github-bot · 2020-12-21T23:33:44Z

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

akankshamahajan15 · 2020-12-21T23:58:07Z

Updated the code to change return type to Void in IOTracer::WriteIOOp and later on handle the status of IOTracer::WriteIOOp in future.

facebook-github-bot · 2020-12-22T00:00:21Z

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

facebook-github-bot · 2020-12-22T00:20:04Z

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

facebook-github-bot

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

Summary: add io_tracer_parser_test and prefetch_test under ASSERT_STATUS_CHECKED Test Plan: ASSERT_STATUS_CHECKED=1 make check -j64 Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2020-12-22T00:43:37Z

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

facebook-github-bot

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

facebook-github-bot · 2020-12-22T19:39:26Z

@akankshamahajan15 merged this pull request in fbac1b3.

Summary: add io_tracer_parser_test and prefetch_test under ASSERT_STATUS_CHECKED Pull Request resolved: facebook#7793 Test Plan: ASSERT_STATUS_CHECKED=1 make check -j64 Reviewed By: jay-zhuang Differential Revision: D25673464 Pulled By: akankshamahajan15 fbshipit-source-id: 50e0b6f17160ddda206a521a7b47ee33e699a2d4

Summary: add io_tracer_parser_test and prefetch_test under ASSERT_STATUS_CHECKED Pull Request resolved: facebook/rocksdb#7793 Test Plan: ASSERT_STATUS_CHECKED=1 make check -j64 Reviewed By: jay-zhuang Differential Revision: D25673464 Pulled By: akankshamahajan15 fbshipit-source-id: 50e0b6f17160ddda206a521a7b47ee33e699a2d4 Signed-off-by: Changlong Chen <levisonchen@live.cn>

facebook-github-bot added the CLA Signed label Dec 21, 2020

akankshamahajan15 requested a review from jay-zhuang December 21, 2020 18:53

mrambacher reviewed Dec 21, 2020

View reviewed changes

jay-zhuang approved these changes Dec 21, 2020

View reviewed changes

akankshamahajan15 force-pushed the assert_status_check branch from 2078b52 to 2da034f Compare December 21, 2020 23:18

facebook-github-bot reviewed Dec 21, 2020

View reviewed changes

akankshamahajan15 force-pushed the assert_status_check branch from 357889a to 053086d Compare December 22, 2020 00:00

akankshamahajan15 force-pushed the assert_status_check branch from 053086d to ce3efac Compare December 22, 2020 00:20

facebook-github-bot reviewed Dec 22, 2020

View reviewed changes

Add tests in ASSERT_STATUS_CHECKED

6cafe37

Summary: add io_tracer_parser_test and prefetch_test under ASSERT_STATUS_CHECKED Test Plan: ASSERT_STATUS_CHECKED=1 make check -j64 Reviewers: Subscribers: Tasks: Tags:

akankshamahajan15 force-pushed the assert_status_check branch from ce3efac to 6cafe37 Compare December 22, 2020 00:43

facebook-github-bot reviewed Dec 22, 2020

View reviewed changes

facebook-github-bot closed this in fbac1b3 Dec 22, 2020

facebook-github-bot added the Merged label Dec 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests in ASSERT_STATUS_CHECKED #7793

Add tests in ASSERT_STATUS_CHECKED #7793

akankshamahajan15 commented Dec 21, 2020

mrambacher Dec 21, 2020

akankshamahajan15 Dec 21, 2020

ajkr Dec 21, 2020

ajkr Dec 21, 2020 •

edited

Loading

ajkr Dec 21, 2020

mrambacher Dec 21, 2020

jay-zhuang left a comment

jay-zhuang Dec 21, 2020

pdillinger Dec 22, 2020

facebook-github-bot left a comment

facebook-github-bot commented Dec 21, 2020

akankshamahajan15 commented Dec 21, 2020

facebook-github-bot commented Dec 22, 2020

facebook-github-bot commented Dec 22, 2020

facebook-github-bot left a comment

facebook-github-bot commented Dec 22, 2020

facebook-github-bot left a comment

facebook-github-bot commented Dec 22, 2020

Add tests in ASSERT_STATUS_CHECKED #7793

Add tests in ASSERT_STATUS_CHECKED #7793

Conversation

akankshamahajan15 commented Dec 21, 2020

mrambacher Dec 21, 2020

Choose a reason for hiding this comment

akankshamahajan15 Dec 21, 2020

Choose a reason for hiding this comment

ajkr Dec 21, 2020

Choose a reason for hiding this comment

ajkr Dec 21, 2020 • edited Loading

Choose a reason for hiding this comment

ajkr Dec 21, 2020

Choose a reason for hiding this comment

mrambacher Dec 21, 2020

Choose a reason for hiding this comment

jay-zhuang left a comment

Choose a reason for hiding this comment

jay-zhuang Dec 21, 2020

Choose a reason for hiding this comment

pdillinger Dec 22, 2020

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 21, 2020

akankshamahajan15 commented Dec 21, 2020

facebook-github-bot commented Dec 22, 2020

facebook-github-bot commented Dec 22, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 22, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 22, 2020

ajkr Dec 21, 2020 •

edited

Loading