Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21059 )
Change subject: IMPALA-12426: QueryStateRecord Refactor ...................................................................... Patch Set 13: (11 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: cons > should this be "const auto&" ? Done http://gerrit.cloudera.org:8080/#/c/21059/12/be/src/service/query-state-record.h File be/src/service/query-state-record.h: http://gerrit.cloudera.org:8080/#/c/21059/12/be/src/service/query-state-record.h@172 PS12, Line 172: int64_t EstimateSize(const QueryStateRecord* record); > Ah, I missed that earlier. Correct. The query_log_size_in_bytes only applies to QueryStateRecord objects that are saved for the recent queries returned by the Impala HTTP server. In the future I want to add similar functionality to limit the size of the completed queries queue based on memory size, so I will be leveraging this pattern for calculating the size of QueryStateExpanded. 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: /// Stores relevant i > nit: Add comment for this struct. Where is it set from? Added a comment. http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.h@176 PS11, Line 176: HostState { > Is this total of fragment instance in specific host? If so, fragment_instan Done http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.h@247 PS11, Line 247: > What is the initial value for this if not connected using HS2? It's undefined. I added comments explaining what consumers needed to do first before using this struct member. http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.h@280 PS11, Line 280: std::string redacted_sql; : : /// Exec Summary Pretty Printed : std::string exec_summary; : > nit: feels like it is better to move these at the end, after executor_group Agreed. Better to organize the code so like definitions are together. 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: // Remove any trailing newlines. > This is new. What is it intended for? It removes newlines at the end of the plan. I added a comment explaining what is happening. http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.cc@267 PS11, Line 267: quals(prof_stack[1]->name().substr(0, 8), "instance") && > I think it is better to move it before the if for readability. Also, use nu I used NULL because that is what GetCounter returns (since it's old code). However, after testing, nullptr can be used, thus I am switching to it. I want to keep the variable declarations where they are because those variables are only used inside the if block. It does make the code slightly less readable but at the same time conforms to the C++ style guide -- https://google.github.io/styleguide/cppguide.html#Local_Variables http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.cc@287 PS11, Line 287: > should be "const auto&" here and few places below? yes, it should! http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.cc@344 PS11, Line 344: executor_groups = exec_group_str.str(); : boost::algorithm::trim_if(executor_groups, boost:: > DCHECK that both has same size. Done http://gerrit.cloudera.org:8080/#/c/21059/11/be/src/service/query-state-record.cc@385 PS11, Line 385: } : : EventsTimelineIterator EventsTimelineIterator::operator++(int) { : ++cur_; : return *this; : } : > Can it be wrong if they don't point to the same labels_ and timestamps_ vec Yes, they can if the EventsTimelineIterator is used in a manner in which it is not intended. However, there is no documentation explaining how the equality operator is making an assumption that the internal labels_ and timestamps_ vectors are the same. Thus, I am adding DCHECKs to ensure iterator comparisons are only done for iterators that point to the same internal vectors. -- 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: 13 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 21:45:03 +0000 Gerrit-HasComments: Yes
