Allow table/sst_file_reader_test.cc to use custom Env #6536
Conversation
There was a problem hiding this comment.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
akankshamahajan15
force-pushed
the
bootcamp_task
branch
from
0315192
to
bf3c9cb
Compare
|
@akankshamahajan15 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
Thanks @akankshamahajan15 for working on this!
Given that options_:Options has a member variable env, let's add a member env_ to the class. The env_ will be loaded in the constructor and assigned to options.env and used throughout the test.
Hi @riversand963 , I wanted to ask is Loading part using Env::LoadEnv inside getenv(WsEnviroment) correct. In DBTestBase, they have a seperate class foreg MockEnv or NewEncryptedEnv which set the objects based on getenv(type_of_enviroment). And then that is set to options.env |
We can disregard mock env and encryption env here. |
akankshamahajan15
force-pushed
the
bootcamp_task
branch
from
bf3c9cb
to
6d2a70f
Compare
|
@akankshamahajan15 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
akankshamahajan15
force-pushed
the
bootcamp_task
branch
from
6d2a70f
to
ff87829
Compare
|
@akankshamahajan15 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Good job. One last piece is adding something like (https://github.com/facebook/rocksdb/blob/master/db/db_basic_test.cc#L2204:L2210) before the main function of sst_file_reader_test.cc, and call the function accordingly, similar to https://github.com/facebook/rocksdb/blob/master/db/db_basic_test.cc#L2215.
Nit: call ROCKSDB_NAMESPACE::port::InstallStackTraceHandler(); at the beginning of main(), too.
akankshamahajan15
force-pushed
the
bootcamp_task
branch
from
ff87829
to
509fa4d
Compare
|
@akankshamahajan15 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
table/sst_file_reader_test.cc
Outdated
| } | ||
|
|
||
| ~SstFileReaderTest() { | ||
| Status s = Env::Default()->DeleteFile(sst_name_); | ||
| Status s = env_->DeleteFile(sst_name_); | ||
| assert(s.ok()); |
There was a problem hiding this comment.
Nit: substitute assert(s.ok()) for EXPECT_OK(s).
akankshamahajan15
force-pushed
the
bootcamp_task
branch
from
509fa4d
to
5b1fa55
Compare
|
@akankshamahajan15 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@akankshamahajan15 merged this pull request in a8149ae. |
Allowing table/sst_file_reader_test.cc to use custom Env specified by TEST_ENV_URI.