Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21059 )
Change subject: IMPALA-12426: QueryStateRecord Refactor ...................................................................... Patch Set 11: (10 comments) http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record-test.cc File be/src/service/query-state-record-test.cc: http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record-test.cc@94 PS11, Line 94: auto should this be "const auto&" ? http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.h File be/src/service/query-state-record.h: http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.h@174 PS11, Line 174: struct PerHostState { nit: Add comment for this struct. Where is it set from? http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.h@176 PS11, Line 176: fragment_instances Is this total of fragment instance in specific host? If so, fragment_instance_count might be a better name. http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.h@247 PS11, Line 247: apache::hive::service::cli::thrift::TProtocolVersion::type hiveserver2_protocol_version; What is the initial value for this if not connected using HS2? http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.h@280 PS11, Line 280: /// Events Timeline Empty : bool events_timeline_empty() const; : : /// Events Timeline Iterator : EventsTimelineIterator EventsTimeline() const; nit: feels like it is better to move these at the end, after executor_groups. http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.cc File be/src/service/query-state-record.cc: http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.cc@62 PS11, Line 62: boost::algorithm::trim_if(plan, boost::algorithm::is_any_of("\n")); This is new. What is it intended for? http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.cc@267 PS11, Line 267: auto cntr = profile->GetCounter("ScannerIoWaitTime"); cntr != NULL I think it is better to move it before the if for readability. Also, use nullptr instead of NULL. Same comment for similar patterns below. http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.cc@287 PS11, Line 287: auto should be "const auto&" here and few places below? http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.cc@344 PS11, Line 344: return base_state->event_sequence.labels.empty() || : base_state->event_sequence.timestamps.empty(); DCHECK that both has same size. http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.cc@385 PS11, Line 385: bool EventsTimelineIterator::operator==(const EventsTimelineIterator& other) const { : return cur_ == other.cur_; : } : : bool EventsTimelineIterator::operator!=(const EventsTimelineIterator& other) const { : return !(*this == other); : } Can it be wrong if they don't point to the same labels_ and timestamps_ vector? -- To view, visit http://gerrit.cloudera.org:8080/21059 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I57d470db6fea71ec12e43f86e3fd62ed6259c83a Gerrit-Change-Number: 21059 Gerrit-PatchSet: 11 Gerrit-Owner: Jason Fehr <[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: Wed, 28 Feb 2024 05:18:48 +0000 Gerrit-HasComments: Yes
