Xiaomeng Zhang 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 3: (8 comments) Patch set 3 has jenkins test failure "Bad SAM flags in obtain_sam_padata" on CHECK_EQ(krb5_get_default_realm(krb5_ctx, &unused_realm), 0); 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 krb5_ctx; > Maybe 'krb5_ctx' is a clearer name? Yes, it should be. http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@93 PS2, Line 93: CHECK_EQ(krb5_get_default_realm(krb5_ctx, &unused_realm), 0); > What is the performance impact of these extra allocation/deallocations? It has negative impact on perf. I added this initialization based on comments in init.cc InitKrb5Ctx "Work around the lack of thread safety in krb5_parse_name() by implicitly initializing g_krb5_ctx->default_realm once." http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@97 PS2, Line 97: krb5_error_code code = krb5_parse_name(krb5_ctx, principal.c_str(), &princ); > You could join with the next line: Done http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@99 PS2, Line 99: *realm = princ->realm.data; > Is there a test that generates TErrorCode::BAD_PRINCIPAL_FORMAT? No, I will add one. http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@100 PS2, Line 100: krb5_data* data = princ->data; > Maybe need to call Done http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@105 PS2, Line 105: krb5_free_principal(krb5_ctx, princ); > I think you can say Good to know! http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@111 PS2, Line 111: > *hostname = data->data; Done http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@114 PS2, Line 114: > Maybe need to call Done -- 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: 3 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-Reviewer: Xiaomeng Zhang <xiaom...@cloudera.com> Gerrit-Comment-Date: Mon, 21 Oct 2019 20:14:12 +0000 Gerrit-HasComments: Yes
