Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/14433 )
Change subject: IMPALA-7504 ParseKerberosPrincipal() should use krb5_parse_name() instead ...................................................................... Patch Set 2: (10 comments) Thanks for a first cut at this. I'm still not 100% convinced that this is the right thing to do. but whatever happens it is a good training exercise. I think the general questions are: What is the performance impact of this change. Is this code on the main path of a query? Is the new function better at dealing with bad input than the old function? http://gerrit.cloudera.org:8080/#/c/14433/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14433/2//COMMIT_MSG@8 PS2, Line 8: If you look at other Impala commit message you will see that they usually try to explain what is being done in the commit in some detail. I always imagine I am explaining what I am doing to some other developer. This is a place to discuss why as well as what you are doing, and any tradeoffs that were considered. You should also add a section explaining what testing you did http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc File be/src/util/auth-util.cc: http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@90 PS2, Line 90: krb5_context g_krb5_ctx; Maybe 'krb5_ctx' is a clearer name? http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@91 PS2, Line 91: CHECK_EQ(krb5_init_context(&g_krb5_ctx), 0); Do you need unused_realm or to call krb5_get_default_realm? http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@93 PS2, Line 93: CHECK_EQ(krb5_get_default_realm(g_krb5_ctx, &unused_realm), 0); What is the performance impact of these extra allocation/deallocations? http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@97 PS2, Line 97: krb5_error_code code; You could join with the next line: krb5_error_code code = ... http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@99 PS2, Line 99: if(code != 0) { Is there a test that generates TErrorCode::BAD_PRINCIPAL_FORMAT? http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@100 PS2, Line 100: return Status(TErrorCode::BAD_PRINCIPAL_FORMAT, principal); Maybe need to call krb5_free_context() before returning http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@105 PS2, Line 105: char* p = data->data; I think you can say *service_name = data->data; http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@111 PS2, Line 111: for(int j = 0; j < data->length; j++) { *hostname = data->data; http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@114 PS2, Line 114: krb5_free_principal(g_krb5_ctx, princ); Maybe need to call krb5_free_context() before returning -- To view, visit http://gerrit.cloudera.org:8080/14433 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24 Gerrit-Change-Number: 14433 Gerrit-PatchSet: 2 Gerrit-Owner: Xiaomeng Zhang <xiaom...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Comment-Date: Tue, 15 Oct 2019 23:16:35 +0000 Gerrit-HasComments: Yes
