Joe McDonnell 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 21: (4 comments) http://gerrit.cloudera.org:8080/#/c/17872/21/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/17872/21/be/src/service/client-request-state.cc@652 PS21, Line 652: string op_type = catalog_op_type() == TCatalogOpType::DDL ? : PrintThriftEnum(ddl_type()) : PrintThriftEnum(catalog_op_type()); : summary_profile_->AddInfoString("DDL Type", op_type); Nit: This is identical for sync/async. To avoid duplication, let's put this in ExecDdlRequest() prior to deciding whether to do async/sync. http://gerrit.cloudera.org:8080/#/c/17872/21/be/src/service/client-request-state.cc@694 PS21, Line 694: catalog_op_executor_.reset( : new CatalogOpExecutor(ExecEnv::GetInstance(), frontend_, server_profile_)); : Status status = catalog_op_executor_->Exec(exec_request_->catalog_op_request); : { : lock_guard<mutex> l(lock_); : RETURN_IF_ERROR(UpdateQueryStatus(status)); : } : : // Add newly created table to catalog cache. : RETURN_IF_ERROR(parent_server_->ProcessCatalogUpdateResult( : *catalog_op_executor_->update_catalog_result(), : exec_request_->query_options.sync_ddl)); : : // Set the results to be reported to the client. : SetResultSet(catalog_op_executor_->ddl_exec_response()); : return Status::OK(); Nit: This code should be unreachable, so replace this with a DCHECK(false); http://gerrit.cloudera.org:8080/#/c/17872/21/be/src/service/client-request-state.cc@713 PS21, Line 713: // Transition the exec state to RUNNING for any non-CTAS DDLs. For the later, the : // state is set from PENDING to RUNNING inside ExecQueryOrDmlRequest(). : if (catalog_op_type() != TCatalogOpType::DDL : || ddl_type() != TDdlType::CREATE_TABLE_AS_SELECT) { : UpdateNonErrorExecState(ExecState::RUNNING); : } Nit: I would rather go directly to RUNNING without passing through PENDING for non-CTAS. Having the state transitions for both in the same place makes it easy to understand that there is a distinction. http://gerrit.cloudera.org:8080/#/c/17872/21/be/src/service/client-request-state.cc@719 PS21, Line 719: string op_type = catalog_op_type() == TCatalogOpType::DDL ? : PrintThriftEnum(ddl_type()) : PrintThriftEnum(catalog_op_type()); : summary_profile_->AddInfoString("DDL Type", op_type); Nit: See other comment about this code. -- 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: 21 Gerrit-Owner: Qifan Chen <[email protected]> Gerrit-Reviewer: Amogh Margoor <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Comment-Date: Tue, 12 Oct 2021 20:49:35 +0000 Gerrit-HasComments: Yes
