You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/09/02 21:27:55 UTC
[GitHub] [kafka] ijuma opened a new pull request, #12587: MINOR: Use named class for ExpiringCredential to improve `principalLogText()` output
ijuma opened a new pull request, #12587:
URL: https://github.com/apache/kafka/pull/12587
`principalLogText()` outputs the expiring credential simple class name
and it's empty for anonymous classes.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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
Re: [PR] [WIP] MINOR: Use named class for ExpiringCredential to improve `principalLogText()` output [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #12587: [WIP] MINOR: Use named class for ExpiringCredential to improve `principalLogText()` output
URL: https://github.com/apache/kafka/pull/12587
--
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
[GitHub] [kafka] rondagostino commented on a diff in pull request #12587: [WIP] MINOR: Use named class for ExpiringCredential to improve `principalLogText()` output
Posted by GitBox <gi...@apache.org>.
rondagostino commented on code in PR #12587:
URL: https://github.com/apache/kafka/pull/12587#discussion_r963820227
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/expiring/ExpiringCredentialRefreshingLogin.java:
##########
@@ -421,17 +419,17 @@ private void reLogin() throws LoginException, ExitRefresherThreadDueToIllegalSta
throw new ExitRefresherThreadDueToIllegalStateException(String.format(
"Subject's private credentials still contains the previous, soon-to-expire instance of %s even though login() followed by logout() was invoked; exiting refresh thread",
expiringCredential.getClass().getName()));
- principalName = expiringCredential.principalName();
if (log.isDebugEnabled())
log.debug("[Principal={}]: It is an expiring credential after re-login as expected",
principalLogText());
}
}
}
- private String principalLogText() {
- return expiringCredential == null ? principalName
Review Comment:
You are correct, given the way the code is written it is not possible to have both `expiringCredential == null` and `principalName != null`. I can't remember specifically why the code was written this way, but it was probably just being defensive in case the above constraint is ever violated. But the constraint is an intuitive one and the code will almost certainly not evolve in such a way as to violate it, so I think it may just cause the question to be raised and add to confusion.
--
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
[GitHub] [kafka] rondagostino commented on pull request #12587: [WIP] MINOR: Use named class for ExpiringCredential to improve `principalLogText()` output
Posted by 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
Re: [PR] [WIP] MINOR: Use named class for ExpiringCredential to improve `principalLogText()` output [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12587:
URL: https://github.com/apache/kafka/pull/12587#issuecomment-2614790435
This PR has been closed since it has not had any activity in 120 days. If you feel like this
was a mistake, or you would like to continue working on it, please feel free to re-open the
PR and ask for a review.
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #12587: MINOR: Use named class for ExpiringCredential to improve `principalLogText()` output
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12587:
URL: https://github.com/apache/kafka/pull/12587#discussion_r962039832
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/expiring/ExpiringCredentialRefreshingLogin.java:
##########
@@ -421,17 +419,17 @@ private void reLogin() throws LoginException, ExitRefresherThreadDueToIllegalSta
throw new ExitRefresherThreadDueToIllegalStateException(String.format(
"Subject's private credentials still contains the previous, soon-to-expire instance of %s even though login() followed by logout() was invoked; exiting refresh thread",
expiringCredential.getClass().getName()));
- principalName = expiringCredential.principalName();
if (log.isDebugEnabled())
log.debug("[Principal={}]: It is an expiring credential after re-login as expected",
principalLogText());
}
}
}
- private String principalLogText() {
- return expiringCredential == null ? principalName
Review Comment:
@rondagostino Do you recall why we had this code? I don't understand how we'd have no expiring credential and a non-null principal name. It seems like the latter would always be set from the expiring credential? Or am I missing something?
--
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
Re: [PR] [WIP] MINOR: Use named class for ExpiringCredential to improve `principalLogText()` output [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12587:
URL: https://github.com/apache/kafka/pull/12587#issuecomment-2563279762
This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.
<p>
If you are having difficulty finding a reviewer, please reach out on the
[mailing list](https://kafka.apache.org/contact).
<p>
If this PR is no longer valid or desired, please feel free to close it. If no activity
occurs in the next 30 days, it will be automatically closed.
--
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