Matthew Mulder has posted comments on this change. Change subject: IMPALA-5625: stress test: write profile when queries fail ......................................................................
Patch Set 2: (5 comments) I rebased and posted another version with a couple minor fixes for indent and using named parameters in a format string. 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 Done 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. Done PS2, Line 818: self.results_dir = gettempdir() > I know this isn't a functional change, but check whether this gettempdir() Since this class doesn't enforce setting results_dir I think it's better to leave this here. Otherwise results_dir could be unset when it is used. PS2, Line 1668: get_profile > Would set_profile be a more descriptive, accurate name? I was debating that myself but decided that, since the major activity is fetching the query profile from Impala, get_profile seemed more appropriate. I could change it to fetch_profile if you like, but I don't think set_profile is appropriate because I don't know the profile when I call this function. 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 profi I haven't looked at performance. At first I was concerned that collecting profiles could potentially cause a cascade of further timeouts, but I thought it's more important to get the profiles even if this happens. It won't cause a test run to fail that otherwise shouldn't have. If I'm the one analyzing a test failure, I'll start with the first failure, which will not have been affected by fetching query profiles. However, if the parameters are set tight such that the system is close to the threshold of failure and the act of fetching a profile from an initial failure pushes the system into a cascade of other failures, that could potentially provide confusing results. I doubt it would happen, since the query cancellation should reduce load on the system, but it would be nice to get some empirical data. -- 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 <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Matthew Mulder <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Tim Wood <[email protected]> Gerrit-HasComments: Yes
