[GitHub] [hadoop] steveloughran commented on a change in pull request #2178: HADOOP-17164 UGI loginUserFromKeytab doesn't set the last login time

[GitBox <gi...@apache.org>]

steveloughran commented on a change in pull request #2178:
URL: https://github.com/apache/hadoop/pull/2178#discussion_r463232170



##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
##########
@@ -528,6 +528,10 @@ private HadoopLoginContext getLogin() {
   private void setLogin(LoginContext login) {
     user.setLogin(login);
   }
+  
+  private void setLastLogin(long now) {

Review comment:
       add javadoc, note unit of time. Milliseconds, right?

##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUGILoginFromKeytab.java
##########
@@ -100,13 +101,34 @@ public void stopMiniKdc() {
       executor.shutdownNow();
     }
   }
+  
+  /**
+   * Login from keytab using the MiniKDC
+   */
+  @Test
+  public void testUGILoginFromKeytab() throws Exception {
+    long beforeLogin = Time.now();
+    String principal = "foo";
+    File keytab = new File(workDir, "foo.keytab");
+    kdc.createPrincipal(keytab, principal);
+
+    UserGroupInformation.loginUserFromKeytab(principal, keytab.getPath());
+    UserGroupInformation ugi = UserGroupInformation.getLoginUser();
+    Assert.assertTrue("UGI should be configured to login from keytab",
+        ugi.isFromKeytab());
+
+    User user = getUser(ugi.getSubject());
+    Assert.assertNotNull(user.getLogin());
+    Assert.assertTrue("User last login time is not set correctly",

Review comment:
       error text should include the values at fault, so we can debug better from nothing but a junit report

##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUGILoginFromKeytab.java
##########
@@ -121,6 +143,9 @@ public void testUGILoginFromKeytab() throws Exception {
     final long firstLogin = user.getLastLogin();
     final LoginContext login1 = user.getLogin();
     Assert.assertNotNull(login1);
+    
+    // Sleep for 1 sec to have a difference between first and second login
+    Thread.sleep(1000);

Review comment:
       if you really want >1s of interval, make it a 2s, delay. There's too much risk of clock/sleep granularity issues




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org