Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20770 )
Change subject: IMPALA-12426: Query History Table ...................................................................... Patch Set 12: (5 comments) http://gerrit.cloudera.org:8080/#/c/20770/10/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/20770/10/be/src/service/impala-server.h@1829 PS10, Line 1829: int32_t max_per_host_thread_reservation; > Changed to an int32_t. Ack http://gerrit.cloudera.org:8080/#/c/20770/10/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/20770/10/be/src/service/workload-management.cc@478 PS10, Line 478: *ctx->sql_ << "'"; > Done Ack http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@186 PS12, Line 186: /// Functions used to generate the SQL DML to insert records into the completed queries : /// table. : static std::list<std::tuple<std::string, std::string, WMFieldParser>> fields_; > Can the static members and functions of workload-management.cc contained in What benefit is there to putting this in a class? Right now it's constrained to this compilation unit and won't produce an external symbol. http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@598 PS12, Line 598: : auto iter = fields_.cbegin(); : DCHECK(iter != fields_.cend()); : create_table_sql << std::get<0>(*iter) << " " << std::get<1>(*iter); : iter++; : : while(iter != fields_.cend()) { : create_table_sql << "," << std::get<0>(*iter) << " " << std::get<1>(*iter); : iter++; : } > What happen if content of fileds_ changed in the future? Do you have some idea how to version it? We will need to be able to handle identifying changes and altering the table layout in the future. I guess requiring a new table name would be another option. http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/util/runtime-profile.h@257 PS12, Line 257: mutable CounterMap counter_map_; How about updating the implementation of GetCounter to use const accessors instead? Something like if (auto it = counter_map_.find(name); it != counter_map_.end()) { return it->second; } Almost that exact example is used in https://en.cppreference.com/w/cpp/container/map/find. -- 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: 12 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, 23 Jan 2024 17:49:01 +0000 Gerrit-HasComments: Yes
