Tim Armstrong has posted comments on this change. Change subject: IMPALA-3200: more buffer pool end-to-end tests ......................................................................
Patch Set 5: (7 comments) If I understood correctly, the concern is that someone will skip it on core without understanding the consequences, right? I could move the tests to functional-query to avoid that, but it seems like that goes against the idea of associating tests with workloads. http://gerrit.cloudera.org:8080/#/c/7552/5/testdata/workloads/functional-query/queries/QueryTest/spilling-sorts-exhaustive.test File testdata/workloads/functional-query/queries/QueryTest/spilling-sorts-exhaustive.test: PS5, Line 86: from (select cast(l_comment as char(200)) char_col > I realize this is a move, but is a varchar(200) test similar to this one ne It was originally necessary to have two char(n) tests because there were two different representations of char, with the switchover happening at 128 bytes. That is no longer the case after "IMPALA-5560: always store CHAR(N) inline in tuple". We could probably remove this test and not lose meaningful coverage. varchar(n) only has a single representation so the single test should be fine. http://gerrit.cloudera.org:8080/#/c/7552/5/testdata/workloads/targeted-perf/targeted-perf_dimensions.csv File testdata/workloads/targeted-perf/targeted-perf_dimensions.csv: PS5, Line 1: file_format: text,seq,rc,avro,parquet,kudu > Thanks for adding this extra coverage here and elsewhere. Have you verified We didn't have any pre-existing targeted-perf tests, except for experiments/test_targeted_perf.py, which seems to have already been broken. I ran with it --collect-only and it showed tests for: text/none seq/gzip/block seq/snap/block rc/none avro/none avro/snap/block parquet/none I ran TestTpchPrimitivesMemLimitError and confirmed that it ran with Parquet (the other formats are filtered out for that test). http://gerrit.cloudera.org:8080/#/c/7552/5/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: PS5, Line 321: impalad_args=impalad_admission_ctrl_flags(1, 1, 10 * 1024 * 1024, : 1024 * 1024 * 1024) > Nit: There are so many numbers here it might be nice to consider kwargs-sty Done PS5, Line 332: assert re.search("Failed to get minimum memory reservation", str(ex)) > Micro-optimization nit: You can use the in operator in python for substring Done http://gerrit.cloudera.org:8080/#/c/7552/5/tests/query_test/test_mem_usage_scaling.py File tests/query_test/test_mem_usage_scaling.py: PS5, Line 92: return 'tpch' > Clarification: If someone later attempts to skip these tests except when in Added comments with warnings about IMPALA-3947 to the applicable test classes. PS5, Line 108: print str(result) > Remove? Done PS5, Line 254: return 'targeted-perf' > Clarification: If someone later attempts to skip these tests except when in Added a warning in a comment. -- To view, visit http://gerrit.cloudera.org:8080/7552 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I554aa5ddfef4f8e75295596e720a14eee1afa17f Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
