diff --git a/src/Disks/ObjectStorages/DiskObjectStorage.cpp b/src/Disks/ObjectStorages/DiskObjectStorage.cpp index 8a5ebea29ec..3ad1c777dfb 100644 --- a/src/Disks/ObjectStorages/DiskObjectStorage.cpp +++ b/src/Disks/ObjectStorages/DiskObjectStorage.cpp @@ -122,10 +122,10 @@ DiskObjectStorage::Metadata DiskObjectStorage::readUpdateAndStoreMetadata(const } -DiskObjectStorage::Metadata DiskObjectStorage::readUpdateStoreMetadataAndRemove(const String & path, bool sync, DiskObjectStorage::MetadataUpdater updater) +void DiskObjectStorage::readUpdateStoreMetadataAndRemove(const String & path, bool sync, DiskObjectStorage::MetadataUpdater updater) { std::unique_lock lock(metadata_mutex); - return Metadata::readUpdateStoreMetadataAndRemove(remote_fs_root_path, metadata_disk, path, sync, updater); + Metadata::readUpdateStoreMetadataAndRemove(remote_fs_root_path, metadata_disk, path, sync, updater); } DiskObjectStorage::Metadata DiskObjectStorage::readOrCreateUpdateAndStoreMetadata(const String & path, WriteMode mode, bool sync, DiskObjectStorage::MetadataUpdater updater) @@ -174,8 +174,13 @@ void DiskObjectStorage::getRemotePathsRecursive(const String & local_path, std:: } catch (const Exception & e) { - if (e.code() == ErrorCodes::FILE_DOESNT_EXIST) + /// Unfortunately in rare cases it can happen when files disappear + /// or can be empty in case of operation interruption (like cancelled metadata fetch) + if (e.code() == ErrorCodes::FILE_DOESNT_EXIST || + e.code() == ErrorCodes::ATTEMPT_TO_READ_AFTER_EOF || + e.code() == ErrorCodes::CANNOT_READ_ALL_DATA) return; + throw; } } @@ -186,6 +191,15 @@ void DiskObjectStorage::getRemotePathsRecursive(const String & local_path, std:: { it = iterateDirectory(local_path); } + catch (const Exception & e) + { + /// Unfortunately in rare cases it can happen when files disappear + /// or can be empty in case of operation interruption (like cancelled metadata fetch) + if (e.code() == ErrorCodes::FILE_DOESNT_EXIST || + e.code() == ErrorCodes::ATTEMPT_TO_READ_AFTER_EOF || + e.code() == ErrorCodes::CANNOT_READ_ALL_DATA) + return; + } catch (const fs::filesystem_error & e) { if (e.code() == std::errc::no_such_file_or_directory) diff --git a/src/Disks/ObjectStorages/DiskObjectStorage.h b/src/Disks/ObjectStorages/DiskObjectStorage.h index d89c00a5567..e7e8869bff0 100644 --- a/src/Disks/ObjectStorages/DiskObjectStorage.h +++ b/src/Disks/ObjectStorages/DiskObjectStorage.h @@ -65,7 +65,7 @@ public: Metadata readMetadata(const String & path) const; Metadata readMetadataUnlocked(const String & path, std::shared_lock &) const; Metadata readUpdateAndStoreMetadata(const String & path, bool sync, MetadataUpdater updater); - Metadata readUpdateStoreMetadataAndRemove(const String & path, bool sync, MetadataUpdater updater); + void readUpdateStoreMetadataAndRemove(const String & path, bool sync, MetadataUpdater updater); Metadata readOrCreateUpdateAndStoreMetadata(const String & path, WriteMode mode, bool sync, MetadataUpdater updater); diff --git a/src/Disks/ObjectStorages/DiskObjectStorageMetadata.cpp b/src/Disks/ObjectStorages/DiskObjectStorageMetadata.cpp index deeb7f2b64a..e863d811101 100644 --- a/src/Disks/ObjectStorages/DiskObjectStorageMetadata.cpp +++ b/src/Disks/ObjectStorages/DiskObjectStorageMetadata.cpp @@ -10,6 +10,10 @@ namespace ErrorCodes { extern const int UNKNOWN_FORMAT; extern const int PATH_ACCESS_DENIED; + extern const int FILE_DOESNT_EXIST; + extern const int ATTEMPT_TO_READ_AFTER_EOF; + extern const int CANNOT_READ_ALL_DATA; + extern const int CANNOT_OPEN_FILE; } DiskObjectStorageMetadata DiskObjectStorageMetadata::readMetadata(const String & remote_fs_root_path_, DiskPtr metadata_disk_, const String & metadata_file_path_) @@ -44,16 +48,38 @@ DiskObjectStorageMetadata DiskObjectStorageMetadata::createUpdateAndStoreMetadat return result; } -DiskObjectStorageMetadata DiskObjectStorageMetadata::readUpdateStoreMetadataAndRemove(const String & remote_fs_root_path_, DiskPtr metadata_disk_, const String & metadata_file_path_, bool sync, DiskObjectStorageMetadataUpdater updater) +void DiskObjectStorageMetadata::readUpdateStoreMetadataAndRemove(const String & remote_fs_root_path_, DiskPtr metadata_disk_, const String & metadata_file_path_, bool sync, DiskObjectStorageMetadataUpdater updater) { - DiskObjectStorageMetadata result(remote_fs_root_path_, metadata_disk_, metadata_file_path_); - result.load(); - if (updater(result)) - result.save(sync); - metadata_disk_->removeFile(metadata_file_path_); + /// Very often we are deleting metadata from some unfinished operation (like fetch of metadata) + /// in this case metadata file can be incomplete/empty and so on. It's ok to remove it in this case + /// because we cannot do anything better. + try + { + DiskObjectStorageMetadata metadata(remote_fs_root_path_, metadata_disk_, metadata_file_path_); + metadata.load(); + if (updater(metadata)) + metadata.save(sync); - return result; + metadata_disk_->removeFile(metadata_file_path_); + } + catch (Exception & ex) + { + /// If we have some broken half-empty file just remove it + if (ex.code() == ErrorCodes::ATTEMPT_TO_READ_AFTER_EOF + || ex.code() == ErrorCodes::CANNOT_READ_ALL_DATA + || ex.code() == ErrorCodes::CANNOT_OPEN_FILE) + { + LOG_INFO(&Poco::Logger::get("ObjectStorageMetadata"), "Failed to read metadata file {} before removal because it's incomplete or empty. " + "It's Ok and can happen after operation interruption (like metadata fetch), so removing as is", metadata_file_path_); + metadata_disk_->removeFile(metadata_file_path_); + } + /// If file already removed, than nothing to do + if (ex.code() == ErrorCodes::FILE_DOESNT_EXIST) + return; + + throw; + } } DiskObjectStorageMetadata DiskObjectStorageMetadata::createAndStoreMetadataIfNotExists(const String & remote_fs_root_path_, DiskPtr metadata_disk_, const String & metadata_file_path_, bool sync, bool overwrite) diff --git a/src/Disks/ObjectStorages/DiskObjectStorageMetadata.h b/src/Disks/ObjectStorages/DiskObjectStorageMetadata.h index ba6b7f952fc..d6e791bd53f 100644 --- a/src/Disks/ObjectStorages/DiskObjectStorageMetadata.h +++ b/src/Disks/ObjectStorages/DiskObjectStorageMetadata.h @@ -47,7 +47,7 @@ struct DiskObjectStorageMetadata static DiskObjectStorageMetadata readMetadata(const String & remote_fs_root_path_, DiskPtr metadata_disk_, const String & metadata_file_path_); static DiskObjectStorageMetadata readUpdateAndStoreMetadata(const String & remote_fs_root_path_, DiskPtr metadata_disk_, const String & metadata_file_path_, bool sync, Updater updater); - static DiskObjectStorageMetadata readUpdateStoreMetadataAndRemove(const String & remote_fs_root_path_, DiskPtr metadata_disk_, const String & metadata_file_path_, bool sync, Updater updater); + static void readUpdateStoreMetadataAndRemove(const String & remote_fs_root_path_, DiskPtr metadata_disk_, const String & metadata_file_path_, bool sync, Updater updater); static DiskObjectStorageMetadata createAndStoreMetadata(const String & remote_fs_root_path_, DiskPtr metadata_disk_, const String & metadata_file_path_, bool sync); static DiskObjectStorageMetadata createUpdateAndStoreMetadata(const String & remote_fs_root_path_, DiskPtr metadata_disk_, const String & metadata_file_path_, bool sync, Updater updater); diff --git a/tests/queries/0_stateless/01281_group_by_limit_memory_tracking.sh b/tests/queries/0_stateless/01281_group_by_limit_memory_tracking.sh index 1807da6948a..2f4164ee0d1 100755 --- a/tests/queries/0_stateless/01281_group_by_limit_memory_tracking.sh +++ b/tests/queries/0_stateless/01281_group_by_limit_memory_tracking.sh @@ -1,5 +1,5 @@ #!/usr/bin/env bash -# Tags: no-replicated-database, no-parallel, no-fasttest, no-tsan, no-asan, no-random-settings +# Tags: no-replicated-database, no-parallel, no-fasttest, no-tsan, no-asan, no-random-settings, no-s3-storage # Tag no-fasttest: max_memory_usage_for_user can interfere another queries running concurrently # Regression for MemoryTracker that had been incorrectly accounted