Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/20770 )
Change subject: IMPALA-12426: Query History Table ...................................................................... Patch Set 14: (18 comments) 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: /// The QueryDriverMap maps query ids to QueryDrivers. The QueryDrivers are owned by the : /// ImpalaServer and QueryDriverMap references them using shared_ptr to allow : /// asynchronous deletion. : /// > Mention in comment if they are synchronize using completed_queries_lock_. Done http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/impala-server.h@1653 PS13, Line 1653: }; > nit: unnecessary added whitespace Done 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@183 PS13, Line 183: class EventsTimelineIterator { > Missing code comments outlining the purpose of this class, and what each fu Holding on this as we are moving towards defining individual fields for specific events in the timeline and only storing those specific events. http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/query-state-record.h@192 PS13, Line 192: EventsTimelineIterator(const std::vector<std::string>* labels, > These would make more sense as const&. Passing nullptr is invalid, and we'r Holding on this as we are moving towards defining individual fields for specific events in the timeline and only storing those specific events. http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/query-state-record.h@206 PS13, Line 206: iter_t operator*() const; > This class is serving two purposes that might be better served as two separ Holding on this as we are moving towards defining individual fields for specific events in the timeline and only storing those specific events. http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/query-state-record.h@212 PS13, Line 212: EventsTimelineIterator cbegin(); > If you called these begin/end, I think you could use this iterator as part Putting this on hold for now as we are moving towards a defined list of events store in individual fields instead of storing the entire events timeline. http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/query-state-record.h@287 PS13, Line 287: }; // struct QueryStateExpanded > nit: it looks strange to prefix things with C just to say they're const. Holding on this as we are moving towards defining individual fields for specific events in the timeline and only storing those specific events. http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/query-state-record.cc File be/src/service/query-state-record.cc: http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/query-state-record.cc@182 PS13, Line 182: ->GetInfoString(AdmissionController::PROFILE_INFO_KEY_EXECUTOR_GROUP); > This and compute_scan_range_assignment are only initialized if summary_prof Both these fields are being dropped from this table. http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/query-state-record.cc@252 PS13, Line 252: > nit: you don't need to use cbegin/cend on const objects. See https://en.cpp Holding on this as we are moving towards defining individual fields for specific events in the timeline and only storing those specific events. 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@461 PS13, Line 461: > Can we remove these when we store it instead? Done http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/workload-management.cc@615 PS13, Line 615: || !FLAGS_enable_workload_mgmt){ : return; : } : > This might not catch query such as: Good catch. This code does miss use/show sql statements that start with comments. I can use the existing catalog_op_type to ignore the use/show sqls. http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/CMakeLists.txt File be/src/util/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/CMakeLists.txt@272 PS13, Line 272: ADD_UNIFIED_BE_LSAN_TEST(string-util-test "TruncateDownTest.*:TruncateUpTest.*:CommaSeparatedContainsTest.*:FindUtf8PosForwardTest.*:FindUtf8PosBackwardTest.*:RandomFindUtf8PosTest.*:ToSnakeCaseTest.*:StringStreamPopTest.*") > typo: StringStreamPopTest Done http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/string-util-test.cc File be/src/util/string-util-test.cc: http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/string-util-test.cc@319 PS13, Line 319: fixture.move_back(); > typo: StringStreamPopTest Done http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/string-util.h File be/src/util/string-util.h: http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/string-util.h@105 PS13, Line 105: void ToSnakeCase(const std::string& in, std::stringstream* out); > nit: ToSnakeCase to match other function names. Done http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/string-util.h@119 PS13, Line 119: /// last character of the stream. The purpose of this additional functionality is to > This is pretty easy and probably more performant as Switched to boost::algorithm::trim_if to remove both leading and trailing newlines. http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/string-util.cc File be/src/util/string-util.cc: http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/string-util.cc@198 PS13, Line 198: > str() can be expensive, it makes a copy of the underlying string. 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 UUIDEmpty(const TUniqueId& id) { > Seems like this should be IsUUIDEmpty to match other functions in this head Done http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/uid-util.h@134 PS13, Line 134: inline bool UUIDEmpty(const TUniqueId& id) { > Code in ImpalaServer::ExecuteIgnoreResults has not changed to use this. 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: 14 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: Tue, 30 Jan 2024 00:39:05 +0000 Gerrit-HasComments: Yes
