impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3200: more buffer pool end-to-end tests
Date Wed, 02 Aug 2017 21:30:00 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message