Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20770 )
Change subject: IMPALA-12426: Query History Table ...................................................................... Patch Set 12: (2 comments) 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_; > What benefit is there to putting this in a class? Right now it's constraine I think I'm just confused that some of workload-management.cc implementations are declared in impala-server.h and not on its own header file. But then I see impala-beeswax-server.cc and impala-hs2-server.cc also did the same way. It will be nice then to mention both workload-management.cc and query-state.cc in impala-server.h comment to point out where the implementation is, similarly as how impala-beeswax-server.cc and impala-hs2-server.cc is mentioned. 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++; : } > Do you have some idea how to version it? We will need to be able to handle Yes, changing the default query_log_table_name upon schema change seems like the easiest way. I suppose the other way is to read schema_version if table is already exist and do the necessary alter table is required. I guess this is also how HMS upgrade is dealt with. https://github.com/apache/impala/blob/ad0dc67482b10ef7a49a432a1bd46887d171800d/bin/create-test-configuration.sh#L188-L193 Or, should Impalad just terminate if existing schema_version does not match? -- 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 18:36:44 +0000 Gerrit-HasComments: Yes
