Skip to content

Conversation

@jerryshao
Copy link
Contributor

What changes were proposed in this pull request?

In the current YARNHadoopDelegationTokenManager, FileSystem to which to get tokens are created out of KDC logged UGI, using these FileSystem to get new tokens will lead to exception. The main thing is that Spark code trying to get new tokens from the FS created with token auth-ed UGI, but Hadoop can only grant new tokens in kerberized UGI. To fix this issue, we should lazily create these FileSystem within KDC logged UGI.

How was this patch tested?

Manual verification in secure cluster.

CC @vanzin @mgummelt please help to review, thanks!

@SparkQA
Copy link

SparkQA commented Jul 14, 2017

Test build #79601 has finished for PR 18633 at commit 9a4f012.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@jerryshao
Copy link
Contributor Author

Jenkins, retest this please.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache the result of this call since it might be used again below?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this closure needs to take a Configuration as paramater.

If you look at the old code, when renewing tokens, the file system list uses AMCredentialRenewer.freshHadoopConf as the configuration, not the ApplicationMaster configuration as your code is doing. That sounds more correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, I see, that looks like another issue, let me change the code.

@SparkQA
Copy link

SparkQA commented Jul 15, 2017

Test build #79626 has finished for PR 18633 at commit 9a4f012.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Change-Id: I8b35ac9c946ae4396aa26ca4ba3107f4b37ac3d6
Change-Id: I256f2fd9fe666fe66606d5c2fa7dd8cf36346d7b
@SparkQA
Copy link

SparkQA commented Jul 18, 2017

Test build #79684 has finished for PR 18633 at commit 95988c1.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@jiangxb1987
Copy link
Contributor

LGTM

@vanzin
Copy link
Contributor

vanzin commented Jul 18, 2017

Merging to master.

@asfgit asfgit closed this in cde64ad Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants