Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9374 )
Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java: http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@61 PS2, Line 61: private String realUser; probably worth a comment. Why is this nullable, considering it's initialized with the system property user.name? Is it possible that is null? If it were null would other stuff just crash later anyway with an NPE? http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java: http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java@73 PS2, Line 73: try (KuduClient client = new KuduClient.KuduClientBuilder(miniCluster.getMasterAddresses()).build()) { wrap http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java@77 PS2, Line 77: NotAuthorized yea, probably.... guess it's worth a JIRA http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java@82 PS2, Line 82: try (KuduClient client = new KuduClient.KuduClientBuilder(miniCluster.getMasterAddresses()).build()) { mind wrapping this? http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-internal.h@233 PS2, Line 233: rpc::UserCredentials user_credentials_; can you comment that this never changes after construction? ie it's effectively const? was a bit nervous about thread-safety but that appears to be the case, right? http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-test.cc@5362 PS2, Line 5362: // TODO(dan): should this be NotAuthorized? yea seems like probably should. maybe put this in the same JIRA as the other one in the Java side? -- To view, visit http://gerrit.cloudera.org:8080/9374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45 Gerrit-Change-Number: 9374 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 23 Feb 2018 20:49:17 +0000 Gerrit-HasComments: Yes
