impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4467: Add support for DML statements in stress test
Date Fri, 16 Dec 2016 21:47:54 GMT
Michael Brown has posted comments on this change.

Change subject: IMPALA-4467: Add support for DML statements in stress test

Patch Set 9:

Commit Message:

Line 14: 
It would be helpful to have 1-2 examples of a full call with DML statement-related
options included.
File tests/stress/

PS9, Line 1682:       help="If True, databases will be reset to their original state after
the binary"
              :       " search.")
On L1971 you say it "may be a good idea" to use this option. I think it might help more if
this help option says that too, or uses even stronger language like "it is suggested to use
this option if you plan on running other tests on the same data", or something similar.

PS9, Line 1871:   def populate_all_queries(queries):
It's odd for a function of this size to live in a scope of its size. Can you factor this out
into a top level function instead? My main objection to the design is that queries_with_runtime_info_by_db
is needed in this function but isn't referenced. The function and parent scope are sufficiently
long to make it harder to read.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2aafdc6851cc0e1677a3c668d3350e47c4bfe40
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message