[GitHub] [incubator-doris] vagetablechicken commented on a change in pull request #3286: unregister fragment mem tracker in close()

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

vagetablechicken commented on a change in pull request #3286: unregister fragment mem tracker in close()
URL: https://github.com/apache/incubator-doris/pull/3286#discussion_r406581630
 
 

 ##########
 File path: be/src/runtime/plan_fragment_executor.cpp
 ##########
 @@ -556,10 +556,12 @@ void PlanFragmentExecutor::close() {
         }
     }
      
-    // _mem_tracker init failed
+    // fragment mem tracker needs unregister
     if (_mem_tracker.get() != nullptr) {
-        _mem_tracker->release(_mem_tracker->consumption());
+        _mem_tracker->unregister_from_parent();
+        _mem_tracker->close();
 
 Review comment:
   I searched all usages of `_runtime_state->fragment_mem_tracker()`. Where else?
   How about move this piece to PlanFragmentExecutor dtor? I think it's a better place.
   
   > By the way, this tracker can be substituted by other tracker. Maybe it can be done in another issue.
   
   Can't agree more. fragment mem tracker's label is "fragment mem-limit", it's not very useful. And one plan has two level1 nodes, I think it's not a good design.
   Let's create a new issue to discuss it.

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