Integration of log storage engines with IDisk interface#8356
Integration of log storage engines with IDisk interface#8356alexey-milovidov merged 20 commits intoClickHouse:masterfrom
Conversation
alesapin
left a comment
alesapin
left a comment
There was a problem hiding this comment.
Almost ok, need to rename some methods. and merge with master :(
Also maybe we need some lightweight class for Path representation? Very often you wrap your String path into Poco::Path.
When dealing with paths and IDisk interface, Poco::Path is currently used only for getting file name from path and getting parent path. So it would probably better to just add two utility functions for these use cases. |
|
Build failed, but it's CI error. |
| : storage(storage_), | ||
| lock(storage.rwlock), | ||
| marks_stream(storage.marks_file.path(), 4096, O_APPEND | O_CREAT | O_WRONLY) | ||
| marks_stream(fullPath(storage.disk, storage.marks_file_path), 4096, O_APPEND | O_CREAT | O_WRONLY) |
There was a problem hiding this comment.
All right. Only StorageTinyLog is fully integrated. It's left a few places in StorageLog and StorageStripeLog that require additional changes in buffer classes. I'd prefer to cover it in a separate PR.
| } | ||
| else | ||
| Poco::File(tmp_files_info_path).renameTo(files_info_path); | ||
| disk->moveFile(tmp_files_info_path, files_info_path); |
There was a problem hiding this comment.
Fixed by extending IDisk with replaceFile() method.
|
Looks like unit tests was broken here: Previous mergecommit in master (c2370b2): |
But this may cause troubles, another option will be to split finalize() into finalize(bool sync) and finalizeImpl() This was introduced in ClickHouse#8356
5 participants
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Changelog category (leave one):
Changelog entry (up to few sentences, required except for Non-significant/Documentation categories):
Detailed description (optional):
These are followup changes for #7946 that integrate log storage engines with IDisk interface.