Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20770 )
Change subject: IMPALA-12426: Query History Table ...................................................................... Patch Set 41: Code-Review+1 (7 comments) http://gerrit.cloudera.org:8080/#/c/20770/39/be/src/service/workload-management.h File be/src/service/workload-management.h: http://gerrit.cloudera.org:8080/#/c/20770/39/be/src/service/workload-management.h@42 PS39, Line 42: /// Name of the database where all workload management tables will be stored. > This is going to allocate storage every place the header is included. Do we Done http://gerrit.cloudera.org:8080/#/c/20770/39/be/src/service/workload-management.h@51 PS39, Line 51: /// this constant is slightly smaller to keep well away from the actual max. > This should still be extern, or it won't use the right value in all cases. Done http://gerrit.cloudera.org:8080/#/c/20770/39/be/src/service/workload-management.h@105 PS39, Line 105: FieldDefinition(const std::string db_col_name, const std::string db_col_type, > These should be `std::move`'d in to place. Right now it's making 2 copies o Done http://gerrit.cloudera.org:8080/#/c/20770/39/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/20770/39/be/src/service/workload-management.cc@206 PS39, Line 206: > This is still evaluated during static initialization, before query_log_writ Done http://gerrit.cloudera.org:8080/#/c/20770/39/be/src/service/workload-management.cc@354 PS39, Line 354: > Does this actually need to be a global? Looks like it's only used in this f Done http://gerrit.cloudera.org:8080/#/c/20770/41/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/20770/41/be/src/service/workload-management.cc@186 PS41, Line 186: // Definition of constants/variables declared in workload-management.h nit: this could all be static variables local to this file since they're not used elsewhere. http://gerrit.cloudera.org:8080/#/c/20770/41/be/src/service/workload-management.cc@368 PS41, Line 368: max_values_length = static_cast<uint32_t>(FLAGS_query_log_max_query_length) - We should use this and the batch size to SET MAX_STATEMENT_LENGTH_BYTES for the query, and only need to truncate if the total is close to 2GB. This may benefit from some testing with multiple large queries to ensure writing to the table doesn't get overwhelmed. -- 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: 41 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: Mon, 11 Mar 2024 21:23:27 +0000 Gerrit-HasComments: Yes
