Gergely Farkas has posted comments on this change. ( http://gerrit.cloudera.org:8080/20591 )
Change subject: IMPALA-12505: Add flag for trusted domain check to use 'origin' if xff header is not set ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/20591/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20591/2//COMMIT_MSG@7 PS2, Line 7: Add flag for trusted domain check to use 'origin' : if xff header is not set : > It would be nice to have a shorter first line, e.g. "Add flag for trusted d Sure, I've updated it. Thanks for the suggestion and the review! http://gerrit.cloudera.org:8080/#/c/20591/2//COMMIT_MSG@11 PS2, Line 11: use_ > The flag name could be mentioned You are right! http://gerrit.cloudera.org:8080/#/c/20591/2/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java File fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java: http://gerrit.cloudera.org:8080/#/c/20591/2/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@506 PS2, Line 506: > Can you add some negative tests too (also in webserver tests)? I added those testcases to hs2 and webserver tests. Thanks! http://gerrit.cloudera.org:8080/#/c/20591/2/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@518 PS2, Line 518: verifyMetrics(0, 0); > This could be also verified to be 0 at the beginning. Done -- To view, visit http://gerrit.cloudera.org:8080/20591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58e5d1119527139eafaa411b55517b10bf394bb2 Gerrit-Change-Number: 20591 Gerrit-PatchSet: 3 Gerrit-Owner: Gergely Farkas <gfar...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Gergely Farkas <gfar...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Tue, 14 Nov 2023 10:42:10 +0000 Gerrit-HasComments: Yes
