[
https://issues.apache.org/jira/browse/ZOOKEEPER-2469?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15367170#comment-15367170
]
Edward Ribeiro commented on ZOOKEEPER-2469:
-------------------------------------------
Nice catch [~sershe]! I am not aware of this code path, but [~arshadmohammad]
has been working with credentials on ZK so I think it's nice to have it take a
look at the patch too. :) I would like to make a few comments though.
The "is" prefix is usually used to Java boolean methods. In the case a boolean
parameter, {{immediateRetry}} seems more readable, imho.
{code}
if (!hasSufficientTimeElapsed(isImmediateRetry)) {
return;
}
{code}
can be rewritten as:
{code}
if (!immediateRetry && !hasSufficientTimeElapsed()) {
return;
}
{code}
That is, there's no need to propagate the {{immediateRetry}} boolean down
{{hasSuficientTimeElapsed()}} if it is only needed at the {{reLogin}} level.
{quote}
// TODO: should we also exit the refresh thread?
{quote}
Well, see that there's an outer try-catch block that catches the LoginException
and exit the thread. As far as I can see, this means that we would need to
re-throw the last exception {{le}} at this exact same line.
I see you trying to preserve the first LoginException, but I am asking myself
if it was not better just to spit out the {{LOG.error}} as below:
{code}
catch (LoginException le) {
LOG.error("Could not refresh TGT for principal:
{}.", principal, le);
if (retry > 0) {
// sleep for 10 seconds.
try {
Thread.sleep(10 * 1000);
} catch (InterruptedException e) {
LOG.error("Interrupted during login
retry after LoginException:", le);
throw le;
}
} else {
throw le;
}
LOG.debug("Retrying login for principal:{}...",
principal);
--retry;
}
{code}
It would be nice to provide some unit tests to exercise those scenarios. This
probably would require some method extraction tough, so it is harder and more
error prone, but maybe worth doing (idk).
Last but not least, as we are using Java 7, then we can replace
{{Thread.sleep(10 * 1000);}} by {{TimeUnit.SECONDS.sleep(10);}} (more clearer,
no need to code comment anymore).
> infinite loop in ZK re-login
> ----------------------------
>
> Key: ZOOKEEPER-2469
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2469
> Project: ZooKeeper
> Issue Type: Bug
> Reporter: Sergey Shelukhin
> Assignee: Sergey Shelukhin
> Attachments: ZOOKEEPER-2469.patch
>
>
> {noformat}
> int retry = 1;
> while (retry >= 0) {
> try {
> reLogin();
> break;
> } catch (LoginException le) {
> if (retry > 0) {
> --retry;
> // sleep for 10 seconds.
> try {
> Thread.sleep(10 * 1000);
> } catch (InterruptedException e) {
> LOG.error("Interrupted during login
> retry after LoginException:", le);
> throw le;
> }
> } else {
> LOG.error("Could not refresh TGT for
> principal: " + principal + ".", le);
> }
> }
> }
> {noformat}
> will retry forever. Should return like the one above
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
