MCOL-4869: More detailed error report about BufferedFile open failure. #3575
Conversation
mariadb-LeonidFedorov
changed the title
utils/idbdatafile/BufferedFile.cpp
Outdated
|
|
||
| if (m_fp == NULL) | ||
| { | ||
| throw std::runtime_error("unable to open Buffered file "); | ||
| static string message = "D 35 CAL0002: Failed to open file: "; |
There was a problem hiding this comment.
You don't need to add any numbers here.
it should be "unable to open Buffered file" with filename and errno explanation
There was a problem hiding this comment.
Done
tests/BufferedFile.cpp
Outdated
|
|
||
| TEST(BufferedFileTest, NoError) | ||
| { | ||
| mkdir("/data", 0777); |
There was a problem hiding this comment.
this wil requre root permissions, unittests are not supposed to have
There was a problem hiding this comment.
The remote system setup is that this is impossible to create directory in home directory (/home/ubuntu). Also, I can not write to files in /var/lib/mysql/test.
There was a problem hiding this comment.
unitests should work on any machine
https://unix.stackexchange.com/questions/352107/generic-way-to-get-temp-path
There was a problem hiding this comment.
Done
tests/BufferedFile.cpp
Outdated
| try { | ||
| idbdatafile::BufferedFile bf(fname, "r", 0); | ||
| } catch(...) { | ||
| catched = true; |
There was a problem hiding this comment.
EXPECT_NO_THROW instead
There was a problem hiding this comment.
Done
tests/BufferedFile.cpp
Outdated
| idbdatafile::BufferedFile bf(fname, "r", 0); | ||
| } catch(const std::runtime_error &e) { | ||
| catched = true; | ||
| EXPECT_EQ(e.what(), message(fname, ENOENT)); |
There was a problem hiding this comment.
EXPECT_THROW
There was a problem hiding this comment.
Done
tests/BufferedFile.cpp
Outdated
| idbdatafile::BufferedFile bf(fname, "r", 0); | ||
| } catch(const std::runtime_error &e) { | ||
| catched = true; | ||
| EXPECT_EQ(std::string(e.what()), message(fname, ENOTDIR)); |
There was a problem hiding this comment.
EXPECT_THROW
There was a problem hiding this comment.
Done
utils/idbdatafile/BufferedFile.cpp
Outdated
| throw std::runtime_error("unable to open Buffered file "); | ||
| static string message = "unable to open file: "; | ||
|
|
||
| throw std::runtime_error(message+fname+", exception: "+strerror(err)); |
There was a problem hiding this comment.
Add spaces around '+'
We are following Google Codestyle for c++
https://google.github.io/styleguide/cppguide.html#Horizontal_Whitespace
You can also use clang-format or as we have config for it in the root of codebase
There is also a clang-format extension for VSCode
There was a problem hiding this comment.
Done
tests/CMakeLists.txt
Outdated
| @@ -101,6 +101,10 @@ if(WITH_UNITTESTS) | |||
| columnstore_link(bytestream ${ENGINE_LDFLAGS} ${ENGINE_WRITE_LIBS} ${CPPUNIT_LIBRARIES} cppunit) | |||
| add_test(NAME columnstore:bytestream COMMAND bytestream) | |||
|
|
|||
| add_executable(BufferedFile BufferedFile.cpp) | |||
There was a problem hiding this comment.
please set a test executable_name
idbdatafile_test
There was a problem hiding this comment.
Done
mariadb-NedeljkoStefanovic
force-pushed
the
MCOL-4869
branch
2 times, most recently
from
591d143 to
eaeb99b
Compare
mariadb-NedeljkoStefanovic
force-pushed
the
MCOL-4869
branch
from
eaeb99b to
6cd5d5c
Compare
The BufferedFile constructor is improved about more descriptive error report about file open failure by using errno.