Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20770 )
Change subject: IMPALA-12426: Query History Table ...................................................................... Patch Set 13: (9 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@1653 PS13, Line 1653: nit: unnecessary added whitespace 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 function does. 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're not mutating them, which are the two use-cases for passing by pointer. 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 separate classes. It's basically https://en.cppreference.com/w/cpp/ranges/zip_view (or boost::combine) with a custom iterator implemented for the events timeline. http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/query-state-record.h@287 PS13, Line 287: EventsTimelineIterator CEventsTimeline() const; nit: it looks strange to prefix things with C just to say they're const. 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: admission_control_time_since_last_update = adm_ctrl_ptr == NULL ? 0 : This and compute_scan_range_assignment are only initialized if summary_profile is non-null. This can be pretty confusing. https://en.cppreference.com/w/cpp/language/value_initialization goes into more detail than I can. But it's safer to give primitive types an explicit default value. http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/query-state-record.cc@252 PS13, Line 252: return EventsTimelineIterator(labels_, timestamps_, labels_->cbegin(), nit: you don't need to use cbegin/cend on const objects. See https://en.cppreference.com/w/cpp/container/vector/begin for the definitions. It should be rare that you need to use the cbegin/cend versions; it has a pretty specific use-case, essentially https://stackoverflow.com/a/12001519. 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.*:RemoveLeadingNewlinesTest.*:StreamSteamPopTest.*") typo: StringStreamPopTest 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: TEST(StreamSteamPopTest, NotEmptyPopOnce) { typo: StringStreamPopTest -- 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: Fri, 26 Jan 2024 23:12:06 +0000 Gerrit-HasComments: Yes
