Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16406 )
Change subject: IMPALA-9229: impala-shell 'profile' to show original and retried queries ...................................................................... Patch Set 5: Code-Review+1 (10 comments) > Patch Set 3: > > (12 comments) > > I updated the patch to include your suggestions from > https://issues.apache.org/jira/browse/IMPALA-9870?focusedCommentId=17188243&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17188243 > so the format of the profile command is: > > PROFILE [ALL | LATEST | ORIGINAL] > > The core functionality is done, I haven't added tests yet. Just looking for > any feedback first to make sure the implementation is correct, then I will > add tests. Thanks for implementing the extended PROFILE command! The patch looks good to me. Just have some minor comments. Do you plan to let anyone else to take a look? If not, I can bump to +2 after tests are added. http://gerrit.cloudera.org:8080/#/c/16406/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16406/2//COMMIT_MSG@27 PS2, Line 27: Beeswax is being : deprecated soon. > I know there is a review out for changing the default: https://gerrit.cloud Thanks for the info! http://gerrit.cloudera.org:8080/#/c/16406/5/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/16406/5/be/src/service/impala-hs2-server.cc@1042 PS5, Line 1042: return_val.__isset.failed_thrift_profiles = true; nit: this is already done in __set_failed_thrift_profiles() http://gerrit.cloudera.org:8080/#/c/16406/5/be/src/service/impala-hs2-server.cc@1052 PS5, Line 1052: return_val.__isset.failed_profiles = true; nit: this is already done in __set_failed_profiles() http://gerrit.cloudera.org:8080/#/c/16406/5/be/src/service/impala-hs2-server.cc@1056 PS5, Line 1056: const TGetRuntimeProfileReq& get_profile_req nit: only the 'format' of the request is used. We can refactor it to a TRuntimeProfileFormat::type parameter. http://gerrit.cloudera.org:8080/#/c/16406/5/be/src/service/impala-hs2-server.cc@1121 PS5, Line 1121: nit: 4 spaces indention http://gerrit.cloudera.org:8080/#/c/16406/5/be/src/service/impala-hs2-server.cc@1132 PS5, Line 1132: if (request.format == TRuntimeProfileFormat::THRIFT) { : SetFailedProfile(&thrift_profile, return_val); : } else if (request.format == TRuntimeProfileFormat::JSON) { : JsonProfileToStringProfile(json_profile, &ss); : SetFailedProfile(&ss, return_val); : } else { : DCHECK(request.format == TRuntimeProfileFormat::STRING : || request.format == TRuntimeProfileFormat::BASE64); : SetFailedProfile(&ss, return_val); : } nit: the 3 if branches have the same conditions as SetProfile() at line 1055. Maybe it's better to refactor them as one method (e.g. by adding a "is_failed_attempt" parameter. http://gerrit.cloudera.org:8080/#/c/16406/5/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/16406/5/shell/impala_shell.py@110 PS5, Line 110: the typo then? http://gerrit.cloudera.org:8080/#/c/16406/5/shell/impala_shell.py@111 PS5, Line 111: the typo then? http://gerrit.cloudera.org:8080/#/c/16406/5/shell/impala_shell.py@1004 PS5, Line 1004: nit: 2 spaces indention http://gerrit.cloudera.org:8080/#/c/16406/5/shell/impala_shell.py@1071 PS5, Line 1071: print("Invalid value for query profile display mode: \'" + : profile_display_mode + "\'") nit: may be helpful to print the three valid values as well. -- To view, visit http://gerrit.cloudera.org:8080/16406 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89cee02947b311e7bf9c7274f47dfc7214c1bb65 Gerrit-Change-Number: 16406 Gerrit-PatchSet: 5 Gerrit-Owner: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Comment-Date: Tue, 15 Sep 2020 01:55:39 +0000 Gerrit-HasComments: Yes
