empiredan commented on code in PR #1248:
URL:
https://github.com/apache/incubator-pegasus/pull/1248#discussion_r1028249160
##########
src/block_service/test/fds_service_test.cpp:
##########
@@ -634,8 +635,8 @@ TEST_F(FDSClientTest, test_basic_operation)
static void
generate_file(const char *filename, unsigned long long file_size, char *block,
unsigned block_size)
{
- int fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR |
S_IRGRP | S_IWGRP);
- ASSERT_TRUE(fd > 0) << strerror(errno) << std::endl;
+ int fd = ::open(filename, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR
| S_IRGRP | S_IWGRP);
+ ASSERT_TRUE(fd > 0) << utils::safe_strerror(errno);
Review Comment:
```suggestion
ASSERT_GT(fd, 0) << utils::safe_strerror(errno);
```
##########
src/block_service/directio_writable_file.cpp:
##########
@@ -96,12 +103,16 @@ bool direct_io_writable_file::finalize()
if (_offset > 0) {
if (::write(_fd, _buffer, _buffer_size) != _buffer_size) {
Review Comment:
Once returned value by `::write()` is non-negative, which means some bytes
might have been written, should we retry remaining bytes several times until
all data is written into the file ?
##########
src/block_service/test/fds_service_test.cpp:
##########
@@ -645,9 +646,11 @@ generate_file(const char *filename, unsigned long long
file_size, char *block, u
for (int j = 0; j < batch_size; ++j) {
block[j] = (char)rand::next_u32(0, 255);
}
- write(fd, block, batch_size);
+ ASSERT_EQ(batch_size, ::write(fd, block, batch_size))
+ << "write file " << filename << " failed, err = {}" <<
utils::safe_strerror(errno);
Review Comment:
```suggestion
<< "write file " << filename << " failed, err = " <<
utils::safe_strerror(errno);
```
##########
src/utils/logging.cpp:
##########
@@ -158,13 +128,11 @@ void dsn_log(const char *file,
namespace dsn {
-std::unique_ptr<logging_provider> logging_provider::_logger =
- std::unique_ptr<logging_provider>(nullptr);
+std::unique_ptr<logging_provider> logging_provider::_logger = nullptr;
Review Comment:
```suggestion
std::unique_ptr<logging_provider> logging_provider::_logger;
```
##########
src/utils/logging_provider.h:
##########
@@ -80,10 +74,16 @@ class logging_provider
virtual void flush() = 0;
-private:
+ void deregister_commands() { _cmds.clear(); }
+
+protected:
+ logging_provider(const char *) {}
Review Comment:
```suggestion
logging_provider(const char *) = default;
```
##########
src/utils/simple_logger.h:
##########
@@ -71,22 +72,22 @@ class simple_logger : public logging_provider
{
public:
simple_logger(const char *log_dir);
- virtual ~simple_logger(void);
+ ~simple_logger(void);
Review Comment:
Should we mark `override` for those destructors whose ancestors are declared
as `virtual` ?
##########
src/block_service/test/fds_service_test.cpp:
##########
@@ -645,9 +646,11 @@ generate_file(const char *filename, unsigned long long
file_size, char *block, u
for (int j = 0; j < batch_size; ++j) {
block[j] = (char)rand::next_u32(0, 255);
}
- write(fd, block, batch_size);
+ ASSERT_EQ(batch_size, ::write(fd, block, batch_size))
+ << "write file " << filename << " failed, err = {}" <<
utils::safe_strerror(errno);
}
- close(fd);
+ ASSERT_EQ(0, ::close(fd)) << "close file " << filename << " failed, err =
{}"
Review Comment:
```suggestion
ASSERT_EQ(0, ::close(fd)) << "close file " << filename << " failed, err
= "
```
##########
src/block_service/directio_writable_file.cpp:
##########
@@ -96,12 +103,16 @@ bool direct_io_writable_file::finalize()
if (_offset > 0) {
if (::write(_fd, _buffer, _buffer_size) != _buffer_size) {
- LOG_ERROR_F(
- "Failed to write last chunk, filie_path = {}, errno = {}",
_file_path, errno);
+ LOG_ERROR_F("Failed to write last chunk, file_path = {}, err = {}",
+ _file_path,
+ utils::safe_strerror(errno));
Review Comment:
Once returned value by `::write()` is non-negative, `errno` may not be set ?
##########
src/utils/logging_provider.h:
##########
@@ -55,9 +51,7 @@ class logging_provider
typedef logging_provider *(*factory)(const char *);
public:
- logging_provider(const char *) {}
-
- virtual ~logging_provider(void) {}
+ virtual ~logging_provider() {}
Review Comment:
```suggestion
virtual ~logging_provider() = default;
```
##########
src/utils/command_manager.cpp:
##########
@@ -177,10 +178,9 @@ command_manager::command_manager()
command_manager::~command_manager()
{
_cmds.clear();
- _handlers.clear();
- // TODO(yingchun): enable this check when all commands deregister
correctly.
- // CHECK(_handlers.empty(), "All commands must be deregistered before
command_manager been
- // destroyed", _handlers.begin()->first);
+ CHECK(_handlers.empty(),
+ "All commands must be deregistered before command_manager been
destroyed",
Review Comment:
```suggestion
"All commands must be deregistered before command_manager is
destroyed, however {} is still registered",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
