You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2020/06/25 01:18:29 UTC
[GitHub] [incubator-doris] xy720 opened a new pull request #3945: Add file cache
xy720 opened a new pull request #3945:
URL: https://github.com/apache/incubator-doris/pull/3945
fix #3944
This commit make segment V1 and V2 share on same file cache, so that segment V2's file descriptors stored in cache can be cleaned up as V1 do.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman merged pull request #3945: [BUG]Make segment V1 and V2 share same file cache
Posted by GitBox <gi...@apache.org>.
morningman merged pull request #3945:
URL: https://github.com/apache/incubator-doris/pull/3945
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] xy720 commented on a change in pull request #3945: [BUG]Make segment V1 and V2 share same file cache
Posted by GitBox <gi...@apache.org>.
xy720 commented on a change in pull request #3945:
URL: https://github.com/apache/incubator-doris/pull/3945#discussion_r446637336
##########
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:
Thanks for advices! For problem 1, we can further check weather StorageEngine::instance()->file_cache() == nullptr.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] xy720 commented on a change in pull request #3945: [BUG]Make segment V1 and V2 share same file cache
Posted by GitBox <gi...@apache.org>.
xy720 commented on a change in pull request #3945:
URL: https://github.com/apache/incubator-doris/pull/3945#discussion_r446636765
##########
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 it above the _file_cache field.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] xy720 commented on a change in pull request #3945: [BUG]Make segment V1 and V2 share same file cache
Posted by GitBox <gi...@apache.org>.
xy720 commented on a change in pull request #3945:
URL: https://github.com/apache/incubator-doris/pull/3945#discussion_r446636538
##########
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:
ok
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] xy720 commented on a change in pull request #3945: [BUG]Make segment V1 and V2 share same file cache
Posted by GitBox <gi...@apache.org>.
xy720 commented on a change in pull request #3945:
URL: https://github.com/apache/incubator-doris/pull/3945#discussion_r446636558
##########
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:
done
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a change in pull request #3945: [BUG]Make segment V1 and V2 share same file cache
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org