Michael Brown has posted comments on this change. Change subject: IMPALA-4467: Add support for DML statements in stress test ......................................................................
Patch Set 9: (3 comments) http://gerrit.cloudera.org:8080/#/c/5093/9//COMMIT_MSG Commit Message: Line 14: It would be helpful to have 1-2 examples of a full concurrent_select.py call with DML statement-related options included. http://gerrit.cloudera.org:8080/#/c/5093/9/tests/stress/concurrent_select.py File tests/stress/concurrent_select.py: 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 http://gerrit.cloudera.org:8080/5093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia2aafdc6851cc0e1677a3c668d3350e47c4bfe40 Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-HasComments: Yes
