Matthew Mulder has posted comments on this change. Change subject: IMPALA-5625: write profile when query times out ......................................................................
Patch Set 1: (8 comments) > Matt, you uploaded a new patch set, but please reply to the inline > comments with either "done" or some other explanation. I replied to them with "Done", but they show up as Draft, so I wonder if you don't see them. http://gerrit.cloudera.org:8080/#/c/7376/1//COMMIT_MSG Commit Message: Line 22: > We typically have a testing done section. Done http://gerrit.cloudera.org:8080/#/c/7376/1/tests/stress/concurrent_select.py File tests/stress/concurrent_select.py: PS1, Line 691: "{0}" > Please add query ID: to the string here. Done Line 699: raise Exception( > Can you also add support for saving profiles when incorrect results occur? Done PS1, Line 704: {0} > Please add "query ID" to the string. Done PS1, Line 748: if not (report.profile and report.query_id): : return > Do you know often this path is executed? This is executed every time the profile is attempted to be written because of a query failure. This check cowardly refuses to write the profile if there is no profile or if we fail to get the query id. This could happen if there's a communication error or some other serious problem. Line 865: LOG.debug("Query id is %s", report.query_id) > Should the comma be a % sign? The prior version uses a comma, and the test output looks correct. It looks like all of the LOG.debug function calls use a comma. It would probably work to use a %, but it's better to follow the current style without good cause to change. Line 871: report.profile = cursor.get_profile() > Do we want to set should_cancel here since time is exceeded? should_cancel indicates whether the query timeout was purposely shortened to induce a cancelation as part of the stress procedure. See line 642. Looks like I should have documented this in the function doc. PS1, Line 1704: "--result-hash-log-dir" > Now that this contains profiles, too, should this be renamed? Yeah, good point. I should update the help text as well. How about "--results-dir" which is slightly more generic, or "--artifacts-dir"? I don't want to be too specific with something like "--results-and-profiles-dir" because then it would have to be changed again when a new kind of artifact, say CSV files or charts, are added. Done -- 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: 1 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
