[SPARK-19626][YARN]Using the correct config to set credentials update time #16955
Conversation
jerryshao
left a comment
jerryshao
left a comment
There was a problem hiding this comment.
Thanks for the fix, only one comment about the changes.
| credentialUpdater.schedule(credentialUpdaterRunnable, remainingTime, TimeUnit.MILLISECONDS) | ||
| } | ||
| val startTime = sparkConf.get(CREDENTIALS_UPDATE_TIME) | ||
| val remainingTime = Math.max(1, startTime - System.currentTimeMillis()) |
There was a problem hiding this comment.
The logic looks different compared to the original code, also 1 is minute not millisecond.
There was a problem hiding this comment.
OK, let me change it back
|
cc again @jerryshao |
|
There's no detail in the JIRA or PR. I can't tell whether you are solving the problem you set out to solve. Are you saying this is the entire problem, that the wrong config is being queried? |
|
Agreed with @srowen , please describe the problem and fix both here in PR and in JIRA, also changing the title to be more meaningful. It would be better for others without the context to understand the issue fast. |
yaooqinn
changed the title
|
@srowen @jerryshao Thanks for your comments. I have added some descriptions both in the JIRA and here, please check whether OK or not. |
|
ping @srowen, would you plz verify this patch again? thanks. |
|
(I don't know enough about this to review) |
|
ok to test |
|
Test build #73068 has finished for PR 16955 at commit
|
|
@vanzin Thanks, all tests passed |
|
Merging to master / 2.1. |
…e time ## What changes were proposed in this pull request? In #14065, we introduced a configurable credential manager for Spark running on YARN. Also two configs `spark.yarn.credentials.renewalTime` and `spark.yarn.credentials.updateTime` were added, one is for the credential renewer and the other updater. But now we just query `spark.yarn.credentials.renewalTime` by mistake during CREDENTIALS UPDATING, where should be actually `spark.yarn.credentials.updateTime` . This PR fixes this mistake. ## How was this patch tested? existing test cc jerryshao vanzin Author: Kent Yao <[email protected]> Closes #16955 from yaooqinn/cred_update. (cherry picked from commit 7363dde) Signed-off-by: Marcelo Vanzin <[email protected]>
…e time ## What changes were proposed in this pull request? In apache#14065, we introduced a configurable credential manager for Spark running on YARN. Also two configs `spark.yarn.credentials.renewalTime` and `spark.yarn.credentials.updateTime` were added, one is for the credential renewer and the other updater. But now we just query `spark.yarn.credentials.renewalTime` by mistake during CREDENTIALS UPDATING, where should be actually `spark.yarn.credentials.updateTime` . This PR fixes this mistake. ## How was this patch tested? existing test cc jerryshao vanzin Author: Kent Yao <[email protected]> Closes apache#16955 from yaooqinn/cred_update.
5 participants
What changes were proposed in this pull request?
In #14065, we introduced a configurable credential manager for Spark running on YARN. Also two configs
spark.yarn.credentials.renewalTimeandspark.yarn.credentials.updateTimewere added, one is for the credential renewer and the other updater. But now we just queryspark.yarn.credentials.renewalTimeby mistake during CREDENTIALS UPDATING, where should be actuallyspark.yarn.credentials.updateTime.This PR fixes this mistake.
How was this patch tested?
existing test
cc @jerryshao @vanzin