morningman commented on a change in pull request #3945:
URL: https://github.com/apache/incubator-doris/pull/3945#discussion_r446165162
##########
File path: be/src/olap/file_helper.cpp
##########
@@ -44,9 +45,19 @@ FileHandler::FileHandler() :
_is_using_cache(false),
_cache_handle(NULL) {
static std::once_flag once_flag;
- std::call_once(once_flag, [] {
- _s_fd_cache = new_lru_cache(config::file_descriptor_cache_capacity);
- });
+ #ifdef BE_TEST
+ std::call_once(once_flag, [] {
+ _s_fd_cache =
new_lru_cache(config::file_descriptor_cache_capacity);
+ });
+ #else
+ // storage engine may not be opened when doris try to read and write
+ // temp file under the storage root path. So we need to check it.
+ if (StorageEngine::instance() != nullptr) {
Review comment:
2 problems:
1. You should check that the storage engine is truly "opened", not just
created, or the `_file_cache` in storage engine is sill null.
2. If `StorageEngine::instance() == nullptr`, the `_s_fd_cache` is null
either. This could cause problems.
For problem 2, I suggest that you can modify the `open_with_cache()` method
as below:
```
OLAPStatus FileHandler::open_with_cache(const string& file_name, int flag) {
if (_cache == nullptr) {
return open(file_name, flag);
}
....
}
```
##########
File path: be/src/util/file_cache.h
##########
@@ -104,8 +104,16 @@ class FileCache {
// the number of files open at any given time.
FileCache(const std::string& cache_name, int max_open_files);
+ // Creates a new file cache with given cache.
+ //
+ // The 'cache_name' is used to disambiguate amongst other file cache
+ // instances.
+ FileCache(const std::string& cache_name, std::shared_ptr<Cache> cache);
Review comment:
Here are 2 constructors of `FileCache`, and the difference is the first
own the `cache`, the second does not own the `cache`.
So you should first add new field `_is_cache_owned` to indicate whether it
owns the cache.
Secondly, to modify the deconstructor of `FileCache`, only reset the cache
if `_is_cache_owned` is true.
##########
File path: be/src/olap/storage_engine.cpp
##########
@@ -157,6 +158,8 @@ Status StorageEngine::_open() {
_index_stream_lru_cache =
new_lru_cache(config::index_stream_cache_capacity);
+ _file_cache.reset(new_lru_cache(config::file_descriptor_cache_capacity));
Review comment:
Add comment to emphasize that the `_file_cache` must be created before
`FileBlockManager`.
##########
File path: be/src/olap/storage_engine.cpp
##########
@@ -503,7 +507,8 @@ void StorageEngine::clear_transaction_task(const
TTransactionId transaction_id,
void StorageEngine::_start_clean_fd_cache() {
VLOG(10) << "start clean file descritpor cache";
- FileHandler::get_fd_cache()->prune();
+ // FileHandler::get_fd_cache()->prune();
Review comment:
remove the unused code.
##########
File path: be/src/olap/storage_engine.h
##########
@@ -297,6 +301,8 @@ class StorageEngine {
Cache* _file_descriptor_lru_cache;
Cache* _index_stream_lru_cache;
+ std::shared_ptr<Cache> _file_cache;
Review comment:
and comment for this field
##########
File path: be/src/olap/file_helper.cpp
##########
@@ -44,9 +45,19 @@ FileHandler::FileHandler() :
_is_using_cache(false),
_cache_handle(NULL) {
static std::once_flag once_flag;
- std::call_once(once_flag, [] {
- _s_fd_cache = new_lru_cache(config::file_descriptor_cache_capacity);
- });
+ #ifdef BE_TEST
+ std::call_once(once_flag, [] {
+ _s_fd_cache =
new_lru_cache(config::file_descriptor_cache_capacity);
+ });
+ #else
+ // storage engine may not be opened when doris try to read and write
+ // temp file under the storage root path. So we need to check it.
+ if (StorageEngine::instance() != nullptr) {
Review comment:
And also, other place where `_s_fd_cache` is used should also be taken
care of. The `release` method can be private, so it will only be called by
`close()`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
