Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/20770 )
Change subject: IMPALA-12426: Query History Table ...................................................................... Patch Set 5: (50 comments) http://gerrit.cloudera.org:8080/#/c/20770/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20770/5//COMMIT_MSG@9 PS5, Line 9: Adds the ability for users to specify that Impala creates > Nit: "to specify that Impala should create and maintain" Done http://gerrit.cloudera.org:8080/#/c/20770/5//COMMIT_MSG@10 PS5, Line 10: contains all : completed queries > Nit: contains data about all completed queries. Done http://gerrit.cloudera.org:8080/#/c/20770/5//COMMIT_MSG@13 PS5, Line 13: each completed query is queued in memory and flushed to the > Are there any queries that are not recorded? If so we could mention that he Done http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/exprs/string-functions.h File be/src/exprs/string-functions.h: http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/exprs/string-functions.h@206 PS5, Line 206: static StringVal PrettyPrintDuration(FunctionContext*, const BigIntVal& duration_us); > Add descriptions Done http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/runtime/exec-env.h@22 PS3, Line 22: #include <unordered_map> > Unnecessary change. Fixed, was left over from previous code. http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/runtime/exec-env.cc@20 PS3, Line 20: #include <vector> > Unnecessary change. Fixed, was leftover from previous code. http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/client-request-state.h@25 PS5, Line 25: #include "runtime/query-exec-params.h" > Why are these headers added? They shouldn't be needed. Fixed. I was running into compiler errors without them. I tried removing them again and the compile errors did not re-appear. http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/impala-server.h@1278 PS5, Line 1278: /// count of the number of times the completed query has attempted to be inserted into > Please explain why we need this count in the comment. Done http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/impala-server.h@1644 PS5, Line 1644: struct QueryStateRecord { > I think these would make sense to break out in a separate header now. I tried to create a new header file, but the circular dependencies made it very difficult to figure out all the necessary include/forward declaration changes that were needed across multiple files. I eventually stopped trying to figure everything out and left the struct definitions in this file. http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/impala-server.h@1761 PS5, Line 1761: /// Initialise from 'exec_state' of a completed query. 'compressed_profile' must be > Not sure what this means Done http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/impala-server.h@1766 PS5, Line 1766: /// Initialize from 'exec_state' of a running query > Not sure what this means Done http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/impala-server.h@1801 PS5, Line 1801: /// Unsure > Maybe explain where it comes from Done http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/impala-server.h@1819 PS5, Line 1819: /// Initialise from 'exec_state' of a completed query. 'compressed_profile' must be > Is this different from the one in QueryStateRecord ? Reorganized the code, this comment is not relevant in the latest patch. http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/impala-server.h@1821 PS5, Line 1821: QueryStateExpanded(const ClientRequestState& exec_state, > Is this different from the one in QueryStateRecord ? Reorganized the code, this comment is not relevant in the latest patch. http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/impala-server.h@1106 PS3, Line 1106: /// timeout values are 'expired': they will no longer accept queries. > A QueryHandle can be dereferenced to a ClientRequestState. This would be mo Done http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/impala-server.cc@1150 PS3, Line 1150: shared_ptr<QueryStateExpanded> record = nullptr; > Although the way I'm planning to borrow these data structures for impala_qu Thinking through this situation, `query_log_` is a list that holds the most recent queries (by default, 100). In a busy cluster, queries will roll off the `query_log_` list before they will be processed into the completed queries table. In a quiet cluster, the opposite happens. Thus, quiet clusters are the situation where the data in QueryStateExpanded is kept longer than needed. Of course, if the user has configured a large value for the max `query_log_` size, then the opposite could happen. My preference is to leave this code as-is, but I'm open to changing it. http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/impala-server.cc@1163 PS5, Line 1163: query_log_.push_front(move(record)); > Reusing QueryStateExpanded here may end up retaining slightly more memory, I modified the QueryStateExpanded struct to be its own struct and to contain a shared pointer to the QueryStateRecord instance. I modified this code so the query_log_ list contains just the QueryStateRecord again. http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/internal-server.h File be/src/service/internal-server.h: http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/internal-server.h@96 PS5, Line 96: query > Nit: query data? Done http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/internal-server.h File be/src/service/internal-server.h: http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/internal-server.h@96 PS3, Line 96: /// `persist_in_db` Boolean indicating if the query should be written to the > With include_in_query_log=false, the queries still show up in the coordinat I think we do still want these queries showing up in the UI. However, the terminology "query_log" matches the query_log that stores completed queries for display on the UI. I will use different terminology here. http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/query-state.cc File be/src/service/query-state.cc: http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/query-state.cc@151 PS5, Line 151: this->impala_coordinator = parent->GetExecEnv()->configured_backend_address(); > Why record this? This value is globally available and shouldn't change. Good point. Fixed. http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc@61 PS3, Line 61: > This would also be useful for writing tests. Changed to seconds. I also was able to remove the code that handled writing to the completed queries table instantly (when this value was set to 0). http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc@62 PS3, Line 62: DEFINE_string_hidden(query_log_table_name, "impala_query_log", "Specifies the name of " > I would interpret this to mean the next write will come this number of minu Expanded the help for both the "query_log_write_interval_s" and "query_log_max_queued" parameters. http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc@87 PS3, Line 87: "write (based on the time period defined in the 'query_log_write_interval_s' flag) " > "must be a valid user" Done http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc@114 PS3, Line 114: > I would implement these as an enum class and use AtomicEnum. I switched to a regular enum guarded by a mutex. The mutex is necessary when using condition variables. http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc@146 PS3, Line 146: static const std::string WM_DB = "information"; Ah the power of IDE auto-completion to propagate misspellings. Fixed http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc@187 PS3, Line 187: static std::list<std::tuple<std::string, std::string, WMFieldParser>> fields_; > Indentation is off here. Done http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc@190 PS3, Line 190: string ret; > There's lots of extra whitespace like this that would be nice to clean up. Done http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc@225 PS3, Line 225: /// Query Id > Queries are still being enqueued when workload management is disabled. Yes, I realized that as well. The fix is in the EnqueueCompletedQuery function. http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc@291 PS3, Line 291: static void _queryType(WMFieldParserContext* ctx) { > We should indent the lambda body for readability. Done http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc@318 PS3, Line 318: static void _clientNetworkAddress(WMFieldParserContext* ctx) { > That will end up breaking it into multiple files. If this is something we c I opened https://issues.apache.org/jira/browse/IMPALA-12652 to implement this. http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc@331 PS3, Line 331: *ctx->sql_ << (ctx->record_->end_time_us - ctx->record_->start_time_us); > This should all use StrAppend. Look at the comments in strcat.h. Fixed. http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc@343 PS3, Line 343: > We should log how many rows were written each time this runs, and warn with Done http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc@388 PS3, Line 388: fields_.push_back(mkTuple("impala_query_end_state", _impalaQueryEndState)); > With TNetworkAddressToString. Done http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/service/workload-management.cc@388 PS3, Line 388: fields_.push_back(mkTuple("impala_query_end_state", _impalaQueryEndState)); > This should store the TNetworkAddress, and we can format it when used. Done http://gerrit.cloudera.org:8080/#/c/20770/4/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/20770/4/be/src/service/workload-management.cc@520 PS4, Line 520: lock_guard<mutex> l(this->completed_queries_lock_); > It's not required and pretty uncommon to prefix variable access with 'this- I removed most of the "this->" prefixes. Will continue to remove them as I find them. http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/workload-management.cc@52 PS5, Line 52: iceberg > Are we still saying we might store the table in another type of table? Not making any promised, but the hope is to support Kudu tables in the future. http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/workload-management.cc@63 PS5, Line 63: completed queries will be stored > Maybe I am pedantic but it is data (or metadata) about completed queries I think data is a better term. Either way, this config option is hidden because we don't want end users to modify the table name but do want the flexibility to modify it to handle rare, unexpected situations. http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/workload-management.cc@112 PS5, Line 112: completes > Nit: "completes," Done http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/workload-management.cc@152 PS5, Line 152: static string INSERT_DML = ""; > I don't understand this string, why is it here? The first portion of the insert dml sql (everything up to and including "VALUES") stays the same from dml to dml. Since the code dynamically builds the dml, this variable is populated at first start and does not change after. http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/workload-management.cc@227 PS5, Line 227: *ctx->sql_ << "'" << PrintId(ctx->record_->id) << "'"; > nit: Trailing whitespace. Done http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/workload-management.cc@406 PS5, Line 406: > trailing space? Done http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/workload-management.cc@458 PS5, Line 458: self-introspection > Should it be just introspection? Yes. http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/workload-management.cc@474 PS5, Line 474: schema_version > How is schema_version used? Right now, it is not used. In the future we will need to add columns to the completed queries table, and this schema version will be used to determine if the table structure needs modifying. http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/workload-management.cc@474 PS5, Line 474: << "STORED AS iceberg TBLPROPERTIES ('schema_version'='1.0.0')"; > Maybe also set 'format-version'='2' to avoid a later upgrade Good catch! I was not aware of the format-version property being required for v2 tables. http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/service/workload-management.cc@597 PS5, Line 597: "\" query_id=\"" << PrintId(iter->first->id) << "\""; > Indent broken lines by 4 spaces. Done http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/util/impalad-metrics.h File be/src/util/impalad-metrics.h: http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/util/impalad-metrics.h@268 PS5, Line 268: hlog > log Done http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/util/string-util.h File be/src/util/string-util.h: http://gerrit.cloudera.org:8080/#/c/20770/3/be/src/util/string-util.h@98 PS3, Line 98: #endif > Unnecessary change. Done http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/util/ticker.h File be/src/util/ticker.h: http://gerrit.cloudera.org:8080/#/c/20770/5/be/src/util/ticker.h@72 PS5, Line 72: return Thread::Create("impala-server", "completed-queries-ticker", > These should be arguments since you're making it a generic class. Done http://gerrit.cloudera.org:8080/#/c/20770/3/tests/util/retry.py File tests/util/retry.py: http://gerrit.cloudera.org:8080/#/c/20770/3/tests/util/retry.py@22 PS3, Line 22: def retry(func, max_attempts=3, sleep_time_s=1, backoff=2, raise_immediately=False): > I think we've implemented this pattern a couple times. I'll keep an eye out It has been implemented a couple times but each has its own variation. This particular implementation is unique where it passes a boolean to func indicating if its the last iteration and also has the ability to raise exceptions immediately. http://gerrit.cloudera.org:8080/#/c/20770/5/tests/util/retry.py File tests/util/retry.py: http://gerrit.cloudera.org:8080/#/c/20770/5/tests/util/retry.py@87 PS5, Line 87: > nit: Trailing whitespace. Done -- 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: 5 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Tue, 16 Jan 2024 21:22:28 +0000 Gerrit-HasComments: Yes
