Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17872 )
Change subject: IMPALA-10811 RPC to submit query getting stuck for AWS NLB forever ...................................................................... Patch Set 17: (7 comments) Address review comments. http://gerrit.cloudera.org:8080/#/c/17872/15/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/17872/15/be/src/service/client-request-state.cc@256 PS15, Line 256: LOG_AND_RETURN_IF_ERROR(ExecDdlRequest()); : break; : } : case TStmtType::LOAD: { : DCHECK(exec_request_->__isset.load_data_request); : TLoadDataResp response; : RETURN_IF_ERROR( : > Style point: Done. Moved the decision to run the logic synchronously or asynchronously into the method ExecDdlRequest(). http://gerrit.cloudera.org:8080/#/c/17872/15/be/src/service/client-request-state.cc@366 PS15, Line 366: // A NULL pattern means match all tables. However, Thrift string types can't : // be NULL in C++, so we have to test if it's set rather than just blindly : // using the value. : const string* table_name = : > Most of the actions in this function could require metadata operations, but Done http://gerrit.cloudera.org:8080/#/c/17872/15/be/src/service/client-request-state.cc@655 PS15, Line 655: if (ddl_type() == TDdlType::COMPUTE_STATS) { : TComputeStatsParams& compute_stats_params = : exec_request_->catalog_op_request.ddl_params.compute_stats_params; : RuntimeProfile* child_profile = : RuntimeProfile::Create(&profile_pool_, "Child Queries"); : > See my comment in ExecLocalCatalogOp() about USE. It would be nice to avoid Done. http://gerrit.cloudera.org:8080/#/c/17872/15/be/src/service/client-request-state.cc@662 PS15, Line 662: vector<ChildQuery> child_queries; : if (compute_stats_params.__isset.tbl_stats_query) { : RuntimeProfile* profile = : RuntimeProfile::Create(&profile_pool_, "Table Stats Query"); : child_profile->AddChild(profile); : child_queries.emplace_back(compute_stats_params.tbl_stats_query, this, : parent_server_, profile, &profile_pool_); : } : if (compute_stats_params.__isset.col_stats_query) { : RuntimeProfile* profile = : RuntimeProfile::Create(&profile_pool_, "Column Stats Query"); : child_profile->AddChild(profile); : child_queries.emplace_back(compute_stats_params.col_stats_query, this, : parent_server_, profile, &profile_pool_); : } : : if (child_queries.size() > 0) { : RETURN_IF_ERROR(child_query_executor_->ExecAsync(move(child_queries))); : } else { : SetResultSet({"No partitions selected for incremental stats update."}); : } : return Status::OK(); : } : : catalog_op_executor_.reset( : new CatalogOpExecutor(ExecEnv::GetInstance(), frontend_, server_profile_)); : RETURN_IF_ERROR( : DebugAction(exec_request_->query_options, "TIMED_WAIT_BEFORE_CATALOG_OP_EXEC")); : Status status = catalog_op_executor_->Exec(exec_request_->catalog_op_request); : { > I think we can skip going async for the compute stats case. It is starting Done http://gerrit.cloudera.org:8080/#/c/17872/15/be/src/service/client-request-state.cc@720 PS15, Line 720: RETURN_IF_ERROR(ExecAsyncQueryOrDmlRequest(exec_request_->query_exec_request)); : } : : // Set the results to be reported to the client. : SetResultSet(catalog_op_executor_->ddl_exec_response()); : return Status::OK(); : } : : Sta > To handle CTAS, one option is to have ExecAsyncQueryOrDmlRequest() have a m Decided to keep this particular case (CREATE_TABLE_AS_SELECT) to run (the exec DDL part) in the same thread as the caller, as the modification to ExecAsyncQueryOrDmlRequest() can be complex, and that in IMPALA-10811, the slow DDLs are those like Rename, Alter Table Recover partition that can take time on existing tables with many partitions. We can file a new JIRA if there is a need for it. http://gerrit.cloudera.org:8080/#/c/17872/15/be/src/service/client-request-state.cc@738 PS15, Line 738: { > ABORT_IF_ERROR will crash Impala if the Status is not OK. not-OK status for Done http://gerrit.cloudera.org:8080/#/c/17872/15/be/src/service/client-request-state.cc@743 PS15, Line 743: // Add newly created table to catalog cache. > For DDLs, today we skip the PENDING state, so I think it makes sense to go Done -- To view, visit http://gerrit.cloudera.org:8080/17872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib57e86926a233ef13d27a9ec8d9c36d33a88a44e Gerrit-Change-Number: 17872 Gerrit-PatchSet: 17 Gerrit-Owner: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Comment-Date: Mon, 11 Oct 2021 16:16:30 +0000 Gerrit-HasComments: Yes
