Michael Brown has posted comments on this change. Change subject: IMPALA-5625: write profile when query times out ......................................................................
Patch Set 2: (8 comments) Thanks for this patch, I think this will be a great improvement. http://gerrit.cloudera.org:8080/#/c/7376/2//COMMIT_MSG Commit Message: PS2, Line 7: IMPALA-5625: write profile when query times out Please amend to say this is for the stress test, and cover general failure. IMPALA-5625: stress test: write profile when queries fail PS2, Line 22: the the "the the" PS2, Line 25: Testing: Great testing, thanks. http://gerrit.cloudera.org:8080/#/c/7376/2/tests/stress/concurrent_select.py File tests/stress/concurrent_select.py: PS2, Line 700: raise Exception(dedent("""Result hash mismatch; expected {0}, got {1} : Query ID: {2} : Query: {3}""".format(query.result_hash, : report.result_hash, : report.query_id, query.sql))) For complicated strings like this, prefer format() with a kwargs-style set of arguments. It makes the string more readable. "{descriptive_arg} ...".format(descriptive_arg=val, ...) The level of complication is subjective, but multiple args + multiple lines is probably enough to warrant it. PS2, Line 708: "Query unexpectedly timed out. Query ID: {0}".format(report.query_id)) This needs to be indented 2 more spaces to satisfy flake8. PS2, Line 818: self.results_dir = gettempdir() I know this isn't a functional change, but check whether this gettempdir() call is needed and remove if you can. PS2, Line 1668: get_profile Would set_profile be a more descriptive, accurate name? PS2, Line 1670: Producing a query profile can be somewhat expensive. A v-tune profile of : impalad showed 10% of cpu time spent generating query profiles. Thanks for preserving this comment. Since we could be collecting more profiles now, have you examined CPU or other performance differences? Do we need to be able to disable profile collection, or collect profiles based on sampling instead of always, or avoid collecting more than 1 profile at a time? -- To view, visit http://gerrit.cloudera.org:8080/7376 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1dbdf5fcf97d6c5681c9fc8fb9eb448bc459b3b0 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Mulder <mmul...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Matthew Mulder <mmul...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Tim Wood <tw...@cloudera.com> Gerrit-HasComments: Yes
