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 24: (13 comments) Many thanks to Joe and Amogh for the review 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: : if (catalog_op_type() != TCatalogOpType::DDL && : catalog_op_type() != TCatalogOpType::RESET_METADA > Nit: This is identical for sync/async. To avoid duplication, let's put this Done http://gerrit.cloudera.org:8080/#/c/17872/21/be/src/service/client-request-state.cc@694 PS21, Line 694: : void ClientRequestState::ExecDdlRequestImplAsync() { : // Transition the exec state to RUNNING for any non-CTAS DDLs. For the later, the : // state is set to RUNNING inside FinishExecQueryOrDmlRequest(). : if (catalog_op_type() != TCatalogOpType::DDL : || ddl_type() != TDdlType::CREATE_TABLE_AS_SELECT) { : UpdateNonErrorExecState(ExecState::RUNNING); : } : : catalog_op_executor_.reset( : new CatalogOpExecutor(ExecEnv::GetInstance(), frontend_, server_profile_)); : DebugActionNoFail( : exec_request_->query_options, "CRS_DELAY_BEFORE_CATALOG_OP_EXEC"); : Status status = catalog_op_executor_->Exec(exec_request_->catalog_op_request); : { : lock_guard<mutex> > Nit: This code should be unreachable, so replace this with a DCHECK(false); Done http://gerrit.cloudera.org:8080/#/c/17872/21/be/src/service/client-request-state.cc@713 PS21, Line 713: // If this is a CTAS request, there will usually be more work to do : // after executing the CREATE TABLE statement (the INSERT portion of the operation). : // The exception is if the user specified IF NOT EXISTS and the table already : // existed, in which case we do not execute the INSERT. : if (catalog_op_type() == TCatalogOpType::DDL && : > Nit: I would rather go directly to RUNNING without passing through PENDING Yes, the code to enter into the PENDING state is removed. This block of code is kept since otherwise no one is set the STATE to RUNNING for non CTAS cases and we will hit illegal transition INITIALIZED -> FINISHED http://gerrit.cloudera.org:8080/#/c/17872/21/be/src/service/client-request-state.cc@719 PS21, Line 719: !catalog_op_executor_->ddl_exec_response()->new_table_created) { : DCHECK(exec_request_->catalog_op_request. : ddl_params.create_table_params.if_not_exists); > Nit: See other comment about this code. Done http://gerrit.cloudera.org:8080/#/c/17872/22/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/17872/22/tests/hs2/test_hs2.py@356 PS22, Line 356: def test_get_operation_status_for_async_ddl(self, > Use unique_database, as that will avoid needing to think about cleanup of t Done http://gerrit.cloudera.org:8080/#/c/17872/22/tests/hs2/test_hs2.py@368 PS22, Line 368: # "create table alltypes_clone as select * from functiona > What I would do is: Did some debugging today and found that the idea of counting # of times in each state is okay. In particular, the # of times in the initialized state is a function of the length of the delay. Before the change, we will not be in the initialized state at all in the GetOperationStatus() loop. http://gerrit.cloudera.org:8080/#/c/17872/22/tests/hs2/test_hs2.py@370 PS22, Line 370: y > flake8: E703 statement ends with a semicolon Done http://gerrit.cloudera.org:8080/#/c/17872/22/tests/hs2/test_hs2.py@371 PS22, Line 371: t > flake8: E703 statement ends with a semicolon Done http://gerrit.cloudera.org:8080/#/c/17872/22/tests/hs2/test_hs2.py@379 PS22, Line 379: ) > flake8: E502 the backslash is redundant between brackets Done http://gerrit.cloudera.org:8080/#/c/17872/22/tests/hs2/test_hs2.py@383 PS22, Line 383: > flake8: E502 the backslash is redundant between brackets Done http://gerrit.cloudera.org:8080/#/c/17872/22/tests/hs2/test_hs2.py@384 PS22, Line 384: > flake8: E129 visually indented line with same indent as next logical line Done http://gerrit.cloudera.org:8080/#/c/17872/22/tests/hs2/test_hs2.py@387 PS22, Line 387: > flake8: E502 the backslash is redundant between brackets Done http://gerrit.cloudera.org:8080/#/c/17872/22/tests/hs2/test_hs2.py@388 PS22, Line 388: > flake8: E129 visually indented line with same indent as next logical line 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: 24 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: Wed, 13 Oct 2021 21:11:44 +0000 Gerrit-HasComments: Yes
