[GitHub] [incubator-doris] morningman commented on a change in pull request #3294: Use read lock when iterate tablet_map in TabletManager::start_trash_sweep

[GitBox <gi...@apache.org>]

morningman commented on a change in pull request #3294: Use read lock when iterate tablet_map in TabletManager::start_trash_sweep
URL: https://github.com/apache/incubator-doris/pull/3294#discussion_r407061670
 
 

 ##########
 File path: be/src/olap/tablet_manager.cpp
 ##########
 @@ -960,28 +960,36 @@ OLAPStatus TabletManager::start_trash_sweep() {
     {
         std::vector<int64_t> tablets_to_clean;
         for (int32 i = 0; i < _tablet_map_lock_shard_size; i++) {
-            WriteLock rlock(&_tablet_map_lock_array[i]);
             tablet_map_t& tablet_map = _tablet_map_array[i];
-            for (auto& item : tablet_map) {
-                // try to clean empty item
-                if (item.second.table_arr.empty()) {
-                    // try to get schema change lock if could get schema change lock, then nobody
-                    // own the lock could remove the item
-                    // it will core if schema change thread may hold the lock and this thread will deconstruct lock
-                    if (item.second.schema_change_lock.trylock() == OLAP_SUCCESS) {
-                        item.second.schema_change_lock.unlock();
+            {
+                ReadLock r_lock(&_tablet_map_lock_array[i]);
+                for (auto& item : tablet_map) {
+                    // try to clean empty item
+                    if (item.second.table_arr.empty()) {
                         tablets_to_clean.push_back(item.first);
                     }
-                }
-                for (TabletSharedPtr tablet : item.second.table_arr) {
-                    tablet->delete_expired_inc_rowsets();
+                    for (TabletSharedPtr tablet : item.second.table_arr) {
 
 Review comment:
   Currently, there should be only one tablet in `tablet_arr`. 
   Only in the early Doris version, `tablet_arr` will appear multiple tablets.
   
   And in `tablet->delete_expired_inc_rowsets();`, it will hold the `meta lock`, So need to be protected by `_tablet_map`.
   
   So you can just by doing:
   
   1. do the `tablets_to_clean.push_back(item.first);` in `ReadLock`
   2. Unlock the `ReadLock`, and iterate the `tablets_to_clean` to do the `tablet->delete_expired_inc_rowsets();`
   3. Do the following `tablet_map.erase(tablet_id_to_clean);` in `WriteLock`

----------------------------------------------------------------
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