Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20770 )
Change subject: IMPALA-12426: Query History Table ...................................................................... Patch Set 34: (9 comments) Please consider rebasing this patch before addressing my comments. http://gerrit.cloudera.org:8080/#/c/20770/34/be/src/service/workload-management.h File be/src/service/workload-management.h: http://gerrit.cloudera.org:8080/#/c/20770/34/be/src/service/workload-management.h@57 PS34, Line 57: /// Default query options that will be provided on all queries that insert rows into the : /// completed queries table. : extern const TQueryOptions INSERT_QUERY_OPTS; Mention the default option chosen here. http://gerrit.cloudera.org:8080/#/c/20770/34/be/src/service/workload-management.h@155 PS34, Line 155: std::tuple<std::string, std::string, FieldParser> Can this type replaced by struct or class? I think it will be easier to refer the fields as col.name, col.type, and col.parser. http://gerrit.cloudera.org:8080/#/c/20770/34/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/20770/34/be/src/service/workload-management.cc@197 PS34, Line 197: std:: Can drop std:: here and below since already using namespace std? http://gerrit.cloudera.org:8080/#/c/20770/34/be/src/service/workload-management.cc@211 PS34, Line 211: const int32_t INSERT_TIMEOUT_S = (FLAGS_query_log_write_timeout_s < 1 ? : FLAGS_query_log_write_interval_s : FLAGS_query_log_write_timeout_s); This can be removed, and INSERT_QUERY_OPTS simply do the the right hand side assignment. http://gerrit.cloudera.org:8080/#/c/20770/34/be/src/service/workload-management.cc@443 PS34, Line 443: \" I think quote is unnecessary for logging here and below. http://gerrit.cloudera.org:8080/#/c/20770/34/be/src/util/sql-util.cc File be/src/util/sql-util.cc: http://gerrit.cloudera.org:8080/#/c/20770/34/be/src/util/sql-util.cc@29 PS34, Line 29: std:: Can drop std:: here and below since already using namespace std? http://gerrit.cloudera.org:8080/#/c/20770/31/tests/beeswax/impala_beeswax.py File tests/beeswax/impala_beeswax.py: http://gerrit.cloudera.org:8080/#/c/20770/31/tests/beeswax/impala_beeswax.py@185 PS31, Line 185: fetch_profile_after_close=False > For now, this parameter is only used for the TestQueryLogTable tests. It c OK http://gerrit.cloudera.org:8080/#/c/20770/34/tests/custom_cluster/test_query_log.py File tests/custom_cluster/test_query_log.py: http://gerrit.cloudera.org:8080/#/c/20770/34/tests/custom_cluster/test_query_log.py@827 PS34, Line 827: while (time() - start_time) < 10: Can this be simplifield by just looping 30 times? http://gerrit.cloudera.org:8080/#/c/20770/34/tests/custom_cluster/test_query_log.py@836 PS34, Line 836: assert 14 <= total_time <= 17 Isn't it better if total_time is as close to 10s? So maybe just asserting <= 17 is enough? Also, note that sanitized build (TSAS, ASAN, etc) can be slower than regular build. -- 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: 34 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: Thu, 07 Mar 2024 23:50:03 +0000 Gerrit-HasComments: Yes
