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/03/07 06:39:05 UTC
[GitHub] [incubator-doris] caiconghui opened a new pull request #3051:
Support sharding tablet_map_lock into more small map locks to make good
performance for tablet manage task
caiconghui opened a new pull request #3051: Support sharding tablet_map_lock into more small map locks to make good performance for tablet manage task
URL: https://github.com/apache/incubator-doris/pull/3051
This PR is to enhance the peformance for tablet manage task, when there are so many tablet in doris be, the only one tablet_map_lock may cause poor performance, and now we split it into many small locks.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request
#3051: Support sharding tablet_map_lock into more small map locks to make
good performance for tablet manage task
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3051: Support sharding tablet_map_lock into more small map locks to make good performance for tablet manage task
URL: https://github.com/apache/incubator-doris/pull/3051#discussion_r389253244
##########
File path: be/src/olap/tablet_manager.cpp
##########
@@ -172,24 +183,29 @@ OLAPStatus TabletManager::_add_tablet_to_map_unlocked(TTabletId tablet_id, Schem
RETURN_NOT_OK_LOG(tablet->register_tablet_into_dir(), Substitute(
"fail to register tablet into StorageEngine. data_dir=$0",
tablet->data_dir()->path()));
-
- _tablet_map[tablet_id].table_arr.push_back(tablet);
- _tablet_map[tablet_id].table_arr.sort(_cmp_tablet_by_create_time);
- _partition_tablet_map[tablet->partition_id()].insert(tablet->get_tablet_info());
+ tablet_map_t& tablet_map = _get_tablet_map(tablet_id);
+ tablet_map[tablet_id].table_arr.push_back(tablet);
+ tablet_map[tablet_id].table_arr.sort(_cmp_tablet_by_create_time);
+ {
+ ReadLock rlock(&_partition_tablet_map_lock);
+ _partition_tablet_map[tablet->partition_id()].insert(tablet->get_tablet_info());
Review comment:
here should be wlock， to protect _partition_tablet_map，_add_tablet_to_map_unlocked is that thread not obtained _tablet_map_lock
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] chaoyli merged pull request #3051: Support
sharding tablet_map_lock into more small map locks to make good performance
for tablet manage task
Posted by GitBox <gi...@apache.org>.
chaoyli merged pull request #3051: Support sharding tablet_map_lock into more small map locks to make good performance for tablet manage task
URL: https://github.com/apache/incubator-doris/pull/3051
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request
#3051: Support sharding tablet_map_lock into more small map locks to make
good performance for tablet manage task
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3051: Support sharding tablet_map_lock into more small map locks to make good performance for tablet manage task
URL: https://github.com/apache/incubator-doris/pull/3051#discussion_r389253303
##########
File path: be/src/olap/tablet_manager.cpp
##########
@@ -533,11 +551,12 @@ OLAPStatus TabletManager::_drop_tablet_unlocked(
OLAPStatus TabletManager::drop_tablets_on_error_root_path(
const vector<TabletInfo>& tablet_info_vec) {
OLAPStatus res = OLAP_SUCCESS;
- WriteLock wlock(&_tablet_map_lock);
for (const TabletInfo& tablet_info : tablet_info_vec) {
TTabletId tablet_id = tablet_info.tablet_id;
TSchemaHash schema_hash = tablet_info.schema_hash;
+ RWMutex& tablet_map_lock = _get_tablet_map_lock(tablet_id);
+ WriteLock wlock(&tablet_map_lock);
Review comment:
ok, I will refactor the code
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#3051: Support sharding tablet_map_lock into more small map locks to make
good performance for tablet manage task
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #3051: Support sharding tablet_map_lock into more small map locks to make good performance for tablet manage task
URL: https://github.com/apache/incubator-doris/pull/3051#discussion_r389238849
##########
File path: be/src/olap/tablet_manager.cpp
##########
@@ -533,11 +551,12 @@ OLAPStatus TabletManager::_drop_tablet_unlocked(
OLAPStatus TabletManager::drop_tablets_on_error_root_path(
const vector<TabletInfo>& tablet_info_vec) {
OLAPStatus res = OLAP_SUCCESS;
- WriteLock wlock(&_tablet_map_lock);
for (const TabletInfo& tablet_info : tablet_info_vec) {
TTabletId tablet_id = tablet_info.tablet_id;
TSchemaHash schema_hash = tablet_info.schema_hash;
+ RWMutex& tablet_map_lock = _get_tablet_map_lock(tablet_id);
+ WriteLock wlock(&tablet_map_lock);
Review comment:
When there are lots of tablet in `tablet_info_vec`, there will be many lock/unlock operation. Does it matter?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request
#3051: Support sharding tablet_map_lock into more small map locks to make
good performance for tablet manage task
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3051: Support sharding tablet_map_lock into more small map locks to make good performance for tablet manage task
URL: https://github.com/apache/incubator-doris/pull/3051#discussion_r389253395
##########
File path: be/src/olap/tablet_manager.h
##########
@@ -190,8 +192,12 @@ class TabletManager {
typedef std::unordered_map<int64_t, TableInstances> tablet_map_t;
// Protect _tablet_map, _partition_tablet_map, _shutdown_tablets
- RWMutex _tablet_map_lock;
- tablet_map_t _tablet_map;
+ int32_t _tablet_map_lock_shard_size;
+ RWMutex *_tablet_map_lock_array;
+ tablet_map_t *_tablet_map_array;
+
+ RWMutex _partition_tablet_map_lock;
+ RWMutex _shutdown_tablets_lock;
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#3051: Support sharding tablet_map_lock into more small map locks to make
good performance for tablet manage task
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #3051: Support sharding tablet_map_lock into more small map locks to make good performance for tablet manage task
URL: https://github.com/apache/incubator-doris/pull/3051#discussion_r389239500
##########
File path: be/src/olap/tablet_manager.h
##########
@@ -190,8 +192,12 @@ class TabletManager {
typedef std::unordered_map<int64_t, TableInstances> tablet_map_t;
// Protect _tablet_map, _partition_tablet_map, _shutdown_tablets
- RWMutex _tablet_map_lock;
- tablet_map_t _tablet_map;
+ int32_t _tablet_map_lock_shard_size;
+ RWMutex *_tablet_map_lock_array;
+ tablet_map_t *_tablet_map_array;
+
+ RWMutex _partition_tablet_map_lock;
+ RWMutex _shutdown_tablets_lock;
Review comment:
And comment of these 2 locks: `_partition_tablet_map_lock` and `_shutdown_tablets_lock`.
Especially notice the lock order of these locks, to avoid dead lock.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request
#3051: Support sharding tablet_map_lock into more small map locks to make
good performance for tablet manage task
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3051: Support sharding tablet_map_lock into more small map locks to make good performance for tablet manage task
URL: https://github.com/apache/incubator-doris/pull/3051#discussion_r389253244
##########
File path: be/src/olap/tablet_manager.cpp
##########
@@ -172,24 +183,29 @@ OLAPStatus TabletManager::_add_tablet_to_map_unlocked(TTabletId tablet_id, Schem
RETURN_NOT_OK_LOG(tablet->register_tablet_into_dir(), Substitute(
"fail to register tablet into StorageEngine. data_dir=$0",
tablet->data_dir()->path()));
-
- _tablet_map[tablet_id].table_arr.push_back(tablet);
- _tablet_map[tablet_id].table_arr.sort(_cmp_tablet_by_create_time);
- _partition_tablet_map[tablet->partition_id()].insert(tablet->get_tablet_info());
+ tablet_map_t& tablet_map = _get_tablet_map(tablet_id);
+ tablet_map[tablet_id].table_arr.push_back(tablet);
+ tablet_map[tablet_id].table_arr.sort(_cmp_tablet_by_create_time);
+ {
+ ReadLock rlock(&_partition_tablet_map_lock);
+ _partition_tablet_map[tablet->partition_id()].insert(tablet->get_tablet_info());
Review comment:
here should be wlock of _partition_tablet_map_lock， to protect _partition_tablet_map，_add_tablet_to_map_unlocked is that thread not obtained _tablet_map_lock
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on issue #3051: Support
sharding tablet_map_lock into more small map locks to make good performance
for tablet manage task
Posted by GitBox <gi...@apache.org>.
caiconghui commented on issue #3051: Support sharding tablet_map_lock into more small map locks to make good performance for tablet manage task
URL: https://github.com/apache/incubator-doris/pull/3051#issuecomment-596054328
for #3052
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request
#3051: Support sharding tablet_map_lock into more small map locks to make
good performance for tablet manage task
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3051: Support sharding tablet_map_lock into more small map locks to make good performance for tablet manage task
URL: https://github.com/apache/incubator-doris/pull/3051#discussion_r389253760
##########
File path: be/src/olap/tablet_manager.cpp
##########
@@ -533,11 +551,12 @@ OLAPStatus TabletManager::_drop_tablet_unlocked(
OLAPStatus TabletManager::drop_tablets_on_error_root_path(
const vector<TabletInfo>& tablet_info_vec) {
OLAPStatus res = OLAP_SUCCESS;
- WriteLock wlock(&_tablet_map_lock);
for (const TabletInfo& tablet_info : tablet_info_vec) {
TTabletId tablet_id = tablet_info.tablet_id;
TSchemaHash schema_hash = tablet_info.schema_hash;
+ RWMutex& tablet_map_lock = _get_tablet_map_lock(tablet_id);
+ WriteLock wlock(&tablet_map_lock);
Review comment:
I think it may slow the clear task, but it is for the clear task, which is not so important for the performance, but there still something to do to reduce times of the lock and unlock operation
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#3051: Support sharding tablet_map_lock into more small map locks to make
good performance for tablet manage task
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #3051: Support sharding tablet_map_lock into more small map locks to make good performance for tablet manage task
URL: https://github.com/apache/incubator-doris/pull/3051#discussion_r389238513
##########
File path: be/src/olap/tablet_manager.cpp
##########
@@ -172,24 +183,29 @@ OLAPStatus TabletManager::_add_tablet_to_map_unlocked(TTabletId tablet_id, Schem
RETURN_NOT_OK_LOG(tablet->register_tablet_into_dir(), Substitute(
"fail to register tablet into StorageEngine. data_dir=$0",
tablet->data_dir()->path()));
-
- _tablet_map[tablet_id].table_arr.push_back(tablet);
- _tablet_map[tablet_id].table_arr.sort(_cmp_tablet_by_create_time);
- _partition_tablet_map[tablet->partition_id()].insert(tablet->get_tablet_info());
+ tablet_map_t& tablet_map = _get_tablet_map(tablet_id);
+ tablet_map[tablet_id].table_arr.push_back(tablet);
+ tablet_map[tablet_id].table_arr.sort(_cmp_tablet_by_create_time);
+ {
+ ReadLock rlock(&_partition_tablet_map_lock);
+ _partition_tablet_map[tablet->partition_id()].insert(tablet->get_tablet_info());
Review comment:
This method name is `_add_tablet_to_map_unlocked()`, why you add a rlock here?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org