[GitHub] [kafka] rondagostino commented on pull request #12587: [WIP] MINOR: Use named class for ExpiringCredential to improve `principalLogText()` output

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

rondagostino commented on PR #12587:
URL: https://github.com/apache/kafka/pull/12587#issuecomment-1382319525

   Ok, I looked at this again, and I think there was a mistake combined with a poorly named variable that was causing confusion.
   
   The `private String principalName = null;` variable that you want to remove of should have originally been named `private String lastKnownPrincipalName = null;`
   
   The reason this variable exists is because of the potential for this case to occur when trying to invoke `reLogin()`:
   ```
               if (!hasExpiringCredential) {
                   /*
                    * Re-login has failed because our login() invocation has not generated a
                    * credential but has also not generated an exception. We won't exit here;
                    * instead we will allow login retries in case we can somehow fix the issue (it
                    * seems likely to be a bug, but it doesn't hurt to keep trying to refresh).
                    */
                   log.error("No Expiring Credential after a supposedly-successful re-login");
                   principalName = null;
   ```
   
   The bug is the line `principalName = null;` -- that line should not exist.  If we had named the variable correctly we would have seen that we are blanking out the last-known principal name, which we should not do.  This case *should* result in the expiring credential being null but the last known principal reflecting the principal that we last had, but with this line it doesn't happen -- we get a null credential and a null last-known name, which is pointless (and therefore I see why with the bad name you wanted to remove it).  But keeping this last-known principal name around is why the logging-related method exists as follows (with the rename from `principalName` to `lastKnownPrincipalName` included):
   
   ```
       private String principalLogText() {
           return expiringCredential == null ? lastKnownPrincipalName
                   : expiringCredential.getClass().getSimpleName() + ":" + lastKnownPrincipalName;
       }
   ```
   
   Apologies for this morass -- two mistakes compounded into something quite confusing.


-- 
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.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

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