zhanglistar commented on code in PR #10815:
URL:
https://github.com/apache/incubator-gluten/pull/10815#discussion_r2415375631
##########
cpp-ch/local-engine/Common/QueryContext.cpp:
##########
@@ -27,6 +27,8 @@
#include <Common/ThreadStatus.h>
#include <Common/formatReadable.h>
#include <Common/logger_useful.h>
+#include <Parser/LocalExecutor.h>
+#include <iostream>
Review Comment:
Should not use iostream
##########
cpp-ch/local-engine/Common/QueryContext.cpp:
##########
@@ -200,14 +216,18 @@ void QueryContext::finalizeQuery(int64_t id)
size_t currentThreadGroupMemoryUsage()
{
if (!CurrentThread::getGroup())
- throw DB::Exception(ErrorCodes::LOGICAL_ERROR, "Thread group not
found, please call initializeQuery first.");
+ {
Review Comment:
Add a log to indicate the exception?
##########
cpp-ch/local-engine/Common/QueryContext.cpp:
##########
@@ -200,14 +216,18 @@ void QueryContext::finalizeQuery(int64_t id)
size_t currentThreadGroupMemoryUsage()
{
if (!CurrentThread::getGroup())
- throw DB::Exception(ErrorCodes::LOGICAL_ERROR, "Thread group not
found, please call initializeQuery first.");
+ {
+ return 0;
+ }
return CurrentThread::getGroup()->memory_tracker.get();
}
double currentThreadGroupMemoryUsageRatio()
{
if (!CurrentThread::getGroup())
- throw DB::Exception(ErrorCodes::LOGICAL_ERROR, "Thread group not
found, please call initializeQuery first.");
+ {
Review Comment:
The same as above.
##########
cpp-ch/local-engine/Common/QueryContext.cpp:
##########
@@ -71,7 +74,16 @@ void QueryContext::resetGlobal()
void QueryContext::reset()
{
+ for (auto it = active_executors_.begin(); it != active_executors_.end();
++it)
+ {
+ auto * executor = *it;
+ executor->cancel();
Review Comment:
Should we check the result status? maybe fail or something else.
##########
cpp-ch/local-engine/Common/QueryContext.cpp:
##########
@@ -71,7 +74,16 @@ void QueryContext::resetGlobal()
void QueryContext::reset()
{
+ for (auto it = active_executors_.begin(); it != active_executors_.end();
++it)
Review Comment:
Here we manipulate the `active_executors_`, lock must be added.
##########
cpp-ch/local-engine/Common/QueryContext.cpp:
##########
@@ -71,7 +74,16 @@ void QueryContext::resetGlobal()
void QueryContext::reset()
{
+ for (auto it = active_executors_.begin(); it != active_executors_.end();
++it)
+ {
+ auto * executor = *it;
+ executor->cancel();
+ std::cerr << "Not closed LocalExecutor:\n" << executor->dumpPipeline()
<< std::endl;
+ delete executor;
Review Comment:
Useless, since `active_executors_.clear();` will call dtor.
##########
cpp-ch/local-engine/Common/QueryContext.cpp:
##########
@@ -71,7 +74,16 @@ void QueryContext::resetGlobal()
void QueryContext::reset()
{
+ for (auto it = active_executors_.begin(); it != active_executors_.end();
++it)
+ {
+ auto * executor = *it;
+ executor->cancel();
+ std::cerr << "Not closed LocalExecutor:\n" << executor->dumpPipeline()
<< std::endl;
Review Comment:
Use log instead of std::cerrr if need.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
