[ 
https://issues.apache.org/jira/browse/YARN-2709?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14177898#comment-14177898
 ] 

Zhijie Shen commented on YARN-2709:
-----------------------------------

[~gtCarrera], thanks for the patch. Here're some comments.

1. Any reason why TimelineClientRetryOp, TimelineClientConnectionRetry and 
TimelineJerseyRetryFilter are not private?

2. Redundant reference.
{code}
            TimelineClientImpl.this.connectionRetry.retryOn(jerseyRetryOp);
{code}

3. Not sure why can't this code be put into run() directly? At least it 
shouldn't be public.
{code}
  public Token<TimelineDelegationTokenIdentifier>
    getDelegationTokenInternal(final String renewer) throws IOException {
{code}

4. It's safer to create connectionRetry before retryFilter, because retryFilter 
may invoke retryFilter, though it won't actually in practice.
{code}
      TimelineJerseyRetryFilter retryFilter = new TimelineJerseyRetryFilter();
      client = new Client(new URLConnectionClientHandler(
          new TimelineURLConnectionFactory()), cc);
      token = new DelegationTokenAuthenticatedURL.Token();
      client.addFilter(retryFilter);
      connectionRetry = new TimelineClientConnectionRetry(conf);
{code}

5. Unnecessary import in TimelineClientImpl

6. I believe the following mock is not necessary. The reason why you want to 
tack this code is because of HADOOP-11215. Due to this issue, it will throw 
cast exception here. Please leave a comment about the mock code bellow.
{code}
    doThrow(new ConnectException("Connection refused")).when(client)
      .getDelegationTokenInternal(any(String.class));
{code}

7. It's not meaningful renewer. You can say 
UserGroupInformation.getCurrentUser().getShortUserName() here.
{code}
      Token<TimelineDelegationTokenIdentifier> token = client
          .getDelegationToken("http://localhost:8/resource?delegation=yyyy";);
{code}

> Add retry for timeline client getDelegationToken method
> -------------------------------------------------------
>
>                 Key: YARN-2709
>                 URL: https://issues.apache.org/jira/browse/YARN-2709
>             Project: Hadoop YARN
>          Issue Type: Bug
>            Reporter: Li Lu
>            Assignee: Li Lu
>         Attachments: YARN-2709-102014-1.patch, YARN-2709-102014.patch
>
>
> As mentioned in YARN-2673, we need to add retry mechanism to timeline client 
> for secured clusters. This means if the timeline server is not available, a 
> timeline client needs to retry to get a delegation token. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to