From 8a0081639612b2d2221991976615b8cbf882f551 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Tue, 19 Jan 2021 22:22:58 +0300 Subject: [PATCH] Do not mark file for distributed send as broken on EOF - the sender will got ATTEMPT_TO_READ_AFTER_EOF (added in 946c275dfbe901cfec87deecc845f72215350b9d) when the client just go away, i.e. server had been restarted, and this is incorrect to mark the file as broken in this case. - since #18853 the file will be checked on the sender locally, and in case the file was truncated CANNOT_READ_ALL_DATA will be thrown. But before #18853 the sender will not receive ATTEMPT_TO_READ_AFTER_EOF from the client in case of file was truncated on the sender, since the client will just wait for more data, IOW just hang. - and I don't see how ATTEMPT_TO_READ_AFTER_EOF can be received while reading local file. --- src/Storages/Distributed/DirectoryMonitor.cpp | 35 ++++++++++++------- src/Storages/Distributed/DirectoryMonitor.h | 1 - 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/Storages/Distributed/DirectoryMonitor.cpp b/src/Storages/Distributed/DirectoryMonitor.cpp index 2a29f2559b6..1b0b78ba0d9 100644 --- a/src/Storages/Distributed/DirectoryMonitor.cpp +++ b/src/Storages/Distributed/DirectoryMonitor.cpp @@ -155,6 +155,27 @@ namespace return header; } + + /// remote_error argument is used to decide whether some errors should be + /// ignored or not, in particular: + /// + /// - ATTEMPT_TO_READ_AFTER_EOF should not be ignored + /// if we receive it from remote (receiver), since: + /// - the sender will got ATTEMPT_TO_READ_AFTER_EOF when the client just go away, + /// i.e. server had been restarted + /// - since #18853 the file will be checked on the sender locally, and + /// if there is something wrong with the file itself, we will receive + /// ATTEMPT_TO_READ_AFTER_EOF not from the remote at first + /// and mark batch as broken. + bool isFileBrokenErrorCode(int code, bool remote_error) + { + return code == ErrorCodes::CHECKSUM_DOESNT_MATCH + || code == ErrorCodes::TOO_LARGE_SIZE_COMPRESSED + || code == ErrorCodes::CANNOT_READ_ALL_DATA + || code == ErrorCodes::UNKNOWN_CODEC + || code == ErrorCodes::CANNOT_DECOMPRESS + || (!remote_error && code == ErrorCodes::ATTEMPT_TO_READ_AFTER_EOF); + } } @@ -571,7 +592,7 @@ struct StorageDistributedDirectoryMonitor::Batch } catch (const Exception & e) { - if (isFileBrokenErrorCode(e.code())) + if (isFileBrokenErrorCode(e.code(), e.isRemoteException())) { tryLogCurrentException(parent.log, "Failed to send batch due to"); batch_broken = true; @@ -801,16 +822,6 @@ void StorageDistributedDirectoryMonitor::processFilesWithBatching(const std::map } } -bool StorageDistributedDirectoryMonitor::isFileBrokenErrorCode(int code) -{ - return code == ErrorCodes::CHECKSUM_DOESNT_MATCH - || code == ErrorCodes::TOO_LARGE_SIZE_COMPRESSED - || code == ErrorCodes::CANNOT_READ_ALL_DATA - || code == ErrorCodes::UNKNOWN_CODEC - || code == ErrorCodes::CANNOT_DECOMPRESS - || code == ErrorCodes::ATTEMPT_TO_READ_AFTER_EOF; -} - void StorageDistributedDirectoryMonitor::markAsBroken(const std::string & file_path) const { const auto last_path_separator_pos = file_path.rfind('/'); @@ -837,7 +848,7 @@ void StorageDistributedDirectoryMonitor::markAsBroken(const std::string & file_p bool StorageDistributedDirectoryMonitor::maybeMarkAsBroken(const std::string & file_path, const Exception & e) const { /// mark file as broken if necessary - if (isFileBrokenErrorCode(e.code())) + if (isFileBrokenErrorCode(e.code(), e.isRemoteException())) { markAsBroken(file_path); return true; diff --git a/src/Storages/Distributed/DirectoryMonitor.h b/src/Storages/Distributed/DirectoryMonitor.h index bc897136786..c73b79761ca 100644 --- a/src/Storages/Distributed/DirectoryMonitor.h +++ b/src/Storages/Distributed/DirectoryMonitor.h @@ -70,7 +70,6 @@ private: void processFile(const std::string & file_path); void processFilesWithBatching(const std::map & files); - static bool isFileBrokenErrorCode(int code); void markAsBroken(const std::string & file_path) const; bool maybeMarkAsBroken(const std::string & file_path, const Exception & e) const;