Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/20770 )
Change subject: IMPALA-12426: Query History Table ...................................................................... Patch Set 13: (31 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/ Good idea. Renamed. 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? Yes. This boolean is used to guard against spurious wakeups of the completed_queries_thread_. I added a separate comment on the completed_queries_ticker_ and completed_queries_ready_ variables. 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/impala-server.h@1861 PS12, Line 1861: > You'd need to std::move the value into place in the constructor. Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/impala-server.h@1861 PS12, Line 1861: > Making this move-only is unnecessarily specific. It could take by value and Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/impala-server.cc@3192 PS12, Line 3192: > nit: redundant "this->" here and below? Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/impala-server.cc@3433 PS12, Line 3433: return conn_id; : } : } // namespace impala > nit: unnecessary whitespace. Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/internal-server.h File be/src/service/internal-server.h: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/internal-server.h@96 PS12, Line 96: /// `persist_in_db` Optional boolean indicating if the query data should be : /// written to the completed queries table afte > nit: Clarify in the doc that `persist_in_db` is optional and default to Tru Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/internal-server.cc File be/src/service/internal-server.cc: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/internal-server.cc@117 PS12, Line 117: internal_query_id.hi != 0 && internal_query_id.lo != 0 > Move this check to uid-util.h Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/internal-server.cc@121 PS12, Line 121: session_id > Should this went through the same check as internal_query_id before closing The CloseSession function does all the necessary checks on its own. The reason the query id had to be checked is because query execution could fail early enough that a query id was never assigned. http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/query-state-record.h File be/src/service/query-state-record.h: http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/query-state-record.h@161 PS13, Line 161: private: > nit: private should be indented differently than the other lines. Previousl Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/query-state.cc File be/src/service/query-state.cc: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/query-state.cc@95 PS12, Line 95: > nit: other place use this from debug-util.h: I would prefer to leave this code as-is since it was directly moved from https://github.com/jasonmfehr/impala/blob/cdac777c51febc99500b8426c2b3aabc7e9addd7/be/src/service/impala-server.cc#L2572 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? I've thought about this some, and I think we should go with a bool flag for now. If different storage types are supported in the future, we can add a storage type flag. 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@70 PS12, Line 70: > nit: Valid value is between (0, 14400]. Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@75 PS12, Line 75: n false; > nit: than or equal to 14400 seconds (24 hours). 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 I added comments in impala-server.h for the functions implemented in workload-management.cc http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@457 PS12, Line 457: > It'd be really useful to have some of the more involved logic like this as I like having the specific logic for formatting the events timeline here, and I also agree there is value in surfacing the events timeline. I created an iterator that can be used to iterate through the events timeline. 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 No, field order is not important except to the custom cluster python test (since it assumes a particular field order, the table must be created with fields ordered the same as the test expects). That being said, it's a bad idea to rearrange these fields because then there will be differences in the table layout between Impala clusters. I added a comment to say don't rearrange these fields. http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@578 PS12, Line 578: > nit: MaxRecordsExceeded sounds better. Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@582 PS12, Line 582: ype=\"" > nit: generating Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@584 PS12, Line 584: g_writ > nit: InitDB. Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@598 PS12, Line 598: : void ImpalaServer::ShutdownWorkloadManagement() { : unique_lock<mutex> l(completed_queries_threadstate_mu_); : if (completed_queries_thread_state_ == RUNNING) { : completed_queries_thread_state_ = SHUTDOWN_REQUESTED; : completed_queries_cv_.notify_all(); : completed_queries_shutdown_cv_.wait_for(l, : std::chrono::seconds(FLAGS_query_log_shutdown_deadline_s), : []{ return completed_queries_thread_state_ == SHUTDOWN; }); : } > Yes, changing the default query_log_table_name upon schema change seems lik Modifications to the completed queries table will be handled in the future. I set the custom table property 'schema_version' to help identify if schema evolution is required or not. Modifying the table structure will require taking some sort of lock in HMS to ensure multiple coordinators are not attempting to alter the table structure at the same time. 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. I'm going to rearrange these into a different order that puts the string checks last. I wanted as much logic as possible for excluding queries from the completed queries table to be in this one spot. The only queries setting IncludedInQueryLog() to false are those that work with the completed queries table (to prevent infinite loops of inserts to the completed queries table). 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. Added. Also added ctest for this struct. http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/util/runtime-profile.h@257 PS12, Line 257: CounterMap counter_map_; > How about updating the implementation of GetCounter to use const accessors 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 Yes, it should. I was not aware any events have special characters in them. http://gerrit.cloudera.org:8080/#/c/20770/12/tests/beeswax/impala_beeswax.py File tests/beeswax/impala_beeswax.py: http://gerrit.cloudera.org:8080/#/c/20770/12/tests/beeswax/impala_beeswax.py@215 PS12, Line 215: result.runtime_profile = self.get_runtime_profile(handle) > Why this is moved after close_query? The profile is not fully complete until after the query is closed. My tests need the full query profile. http://gerrit.cloudera.org:8080/#/c/20770/12/tests/custom_cluster/test_query_log.py File tests/custom_cluster/test_query_log.py: http://gerrit.cloudera.org:8080/#/c/20770/12/tests/custom_cluster/test_query_log.py@39 PS12, Line 39: : class TestQueryLogTable(CustomClusterTestSuite): > Can you craft a custom cluster with 2 coordinator and verify that impala_qu All the clusters are already multi-coordinator. I updated a few of the tests to switch to a different coordinator when they select from the completed queries table to assert the contents of that table. http://gerrit.cloudera.org:8080/#/c/20770/12/tests/custom_cluster/test_query_log.py@633 PS12, Line 633: self.__run_sql_inject(impalad, client, sql1_str, "closing quotes", 3) : : # Try a sql inject attack with terminating quote and semicolon. : sql2_str = "select 1'); drop table {0}; select('" \ : .format(self.QUERY_TBL) : self._ > Why need to sleep? Isn't impala-server.completed_queries.written only incre Without this sleep, the test becomes flaky. Impala does not always return the rows it just inserted if it's queried too quickly after the insert. -- 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 <jf...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Mon, 29 Jan 2024 21:32:13 +0000 Gerrit-HasComments: Yes
