Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16833 )
Change subject: IMPALA-10496: SAML implementation in Impala ...................................................................... Patch Set 25: (11 comments) http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/rpc/authentication.h File be/src/rpc/authentication.h: http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/rpc/authentication.h@58 PS24, Line 58: /// Returns the authentication provider to use for "external" communication with > brief comment Done http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/rpc/authentication.cc@125 PS24, Line 125: > nit: allows Done http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/rpc/authentication.cc@573 PS24, Line 573: } > Brief comment about the parameters here. Done http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/rpc/authentication.cc@629 PS24, Line 629: est > nit: we prefer nullptr, here and elsewhere Done http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/rpc/authentication.cc@683 PS24, Line 683: > probably worth explicitly saying what still needs to be done here I added some comments that explain why did I do this. http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/transport/THttpServer.h File be/src/transport/THttpServer.h: http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/transport/THttpServer.h@85 PS24, Line 85: purposes > typo: purposes Done http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/transport/THttpServer.cpp File be/src/transport/THttpServer.cpp: http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/transport/THttpServer.cpp@228 PS24, Line 228: // Try authenticating with cookies first. > nit: unnecessary whitespace Done http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/transport/THttpServer.cpp@252 PS24, Line 252: throw TTransportException("HTTP auth - SAML redirection."); : } : } else if (!auth_value_ > might be nice to put this logic in some sort of resetConnection() helper fu Done http://gerrit.cloudera.org:8080/#/c/16833/24/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/16833/24/common/thrift/metrics.json@1315 PS24, Line 1315: emon that > This maybe gets a little confusing, since usually when we talk about the "w Done http://gerrit.cloudera.org:8080/#/c/16833/24/common/thrift/metrics.json@1329 PS24, Line 1329: I > typo: space Done http://gerrit.cloudera.org:8080/#/c/16833/23/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClient.java File fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClient.java: http://gerrit.cloudera.org:8080/#/c/16833/23/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClient.java@76 PS23, Line 76: > This path is already parsed in the backend, so I think that it is easier to Added validation to authentication.cc . This also revealed a test issue that the port is only correct on the first Impalad. For now the fix was to change the tests to use a 1 sized cluster. -- To view, visit http://gerrit.cloudera.org:8080/16833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia0c026cba1b90e7ff6ec5ae49be78b0d1edd8dfa Gerrit-Change-Number: 16833 Gerrit-PatchSet: 25 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Wed, 17 Feb 2021 17:03:12 +0000 Gerrit-HasComments: Yes
