Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20770 )
Change subject: IMPALA-12426: Query History Table ...................................................................... Patch Set 13: (14 comments) http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/CMakeLists.txt File be/src/service/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/CMakeLists.txt@46 PS12, Line 46: query-state-re > I recommend to rename this to query-state-record.cc to not confuse with be/ Done http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/impala-server.h@1261 PS13, Line 1261: /// Ticker that wakes up the completed_queried_thread at set intervals to process the : /// queued completed queries. : std::unique_ptr<TickerSB> completed_queries_ticker_; : std::shared_ptr<bool> completed_queries_ready_; Mention in comment if they are synchronize using completed_queries_lock_. Also move this closer to completed_queries_lock_ declaration at line 1294. http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/impala-server.h@1256 PS12, Line 1256: std::unique_ptr<Thread> admission_heartbeat_thr > Is this protected by completed_queries_lock_ as well? Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/impala-server.h@1281 PS12, Line 1281: : /// Default query options in the form of TQueryOptions and b > Mention constant/flag that represent maximum number of attempts in this doc Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/impala-server.h@1797 PS12, Line 1797: : : > Is this also immutable like QueryStateRecord? Please clarify in comment. Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@50 PS12, Line 50: : DEFINE_bool(enable_workload_mgmt, false, : "Specifies if Impala will automatically write completed queries in the query log " : "table. If this value is set to true and then later removed, the query log table " : "will remain intact and accessible."); : > Since it only support iceberg table now, shall this turn to bool flag? Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@63 PS12, Line 63: return false; : }); : > Validate against empty string. Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@186 PS12, Line 186: /// table. : static std::list<std::tuple<std::string, std::string, WMFieldParser>> fields_; : > I agree generally. I think this particular instance is different since it's Ack http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@494 PS12, Line 494: fields_.push_back(MakeTuple("network_address", _clientNetworkAddress)); : fields_.push_back(MakeTuple("start_time_utc", _queryStartTime, "TIMESTAMP")); : fields_.push_back(MakeTuple("total_time_us", _queryDuration, "BIGINT")); : fields_.push_back(MakeTuple("s > Is field order matter here? If yes, please add comment to warn against reor Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@646 PS12, Line 646: { : lock_guard<mutex> l(completed_queries_threadstate_mu_); : completed_queries_thread_state_ = RUNNING; : : completed_queries_ticker_ = make_unique<TickerSB>( : std::chrono::seconds(FLAGS_query_log_write_interval_s), completed_queries_cv_, : completed_que > Add indentations and check wmEnabled() first. Done http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/workload-management.cc@615 PS13, Line 615: || (query_handle->sql_stmt().length() >= 3 : && boost::algorithm::iequals(query_handle->sql_stmt().substr(0,3), "use")) : || (query_handle->sql_stmt().length() >= 4 : && boost::algorithm::iequals(query_handle->sql_stmt().substr(0,4), "show")) This might not catch query such as: -- intentional comment show column stats customer; Probably they should be disabled right from Frontend (UseStmt.java, SetStmt.java, ShowStmt.java). http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/util/network-util.h File be/src/util/network-util.h: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/util/network-util.h@114 PS12, Line 114: const int uds_addr_compare = first.uds_address.compare(second.uds_address); : if (uds_addr_compare < 0) { > missing comparison over uds_address. Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/util/string-util.h File be/src/util/string-util.h: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/util/string-util.h@99 PS12, Line 99: /// is a space) or to the lower case of the character and writes the converted character : /// to a stringstream. : /// > Should this handle special char as well? For example, there is an event key Done http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/uid-util.h File be/src/util/uid-util.h: http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/uid-util.h@134 PS13, Line 134: inline bool is_uuid_empty(const TUniqueId& id) { > Seems like this should be IsUUIDEmpty to match other functions in this head Code in ImpalaServer::ExecuteIgnoreResults has not changed to use this. -- To view, visit http://gerrit.cloudera.org:8080/20770 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844 Gerrit-Change-Number: 20770 Gerrit-PatchSet: 13 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Sat, 27 Jan 2024 01:18:26 +0000 Gerrit-HasComments: Yes
